Paimon reuse scanner#65404
Open
zhangstar333 wants to merge 3 commits into
Open
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
zhangstar333
force-pushed
the
paimon-reuse-scanner
branch
from
17d7a1e to
c273a9f
Compare
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review found two blocking issues in the Paimon JNI scanner reuse.
Critical checkpoints:
- Goal/test coverage: the scanner reuse is not complete because the legacy FileScanner Paimon JNI path still depends on constructor-provided paimon_split, and the new tests do not cover that path or close-failure cleanup.
- Scope and parallel paths: the FileScannerV2 path was updated to call prepareForSplit, but the still-selectable legacy FileScanner path was not.
- Lifecycle/error cleanup: PaimonJniReader::close can return before the base JNI cleanup when split reset fails.
- Config/session propagation: the new paimon.jni.enable_file_reader_async option path was checked end to end; no additional issue found there.
- Compatibility: mixed scanner implementations and the forwarded enable_file_scanner_v2 session variable make the legacy path a compatibility requirement.
Validation: static review plus git diff --check against base 64769a1. FE/BE tests were not run because this runner lacks thirdparty/installed and thirdparty/installed/bin/protoc.
Subagent conclusions: optimizer-rewrite OR-1 was merged into M-1; tests-session-config TSC-1 became M-2; convergence round 1 ended with both subagents reporting NO_NEW_VALUABLE_FINDINGS. User focus: no additional user-provided focus points.
| @@ -132,7 +133,8 @@ public void open() throws IOException { | |||
| preExecutionAuthenticator.execute(() -> { | |||
| PaimonJdbcDriverUtils.registerDriverIfNeeded(params, classLoader); | |||
Contributor
There was a problem hiding this comment.
This breaks the legacy Paimon JNI reader path. open() no longer builds a reader from the constructor's paimon_split; it only initializes the table/projection and leaves reader null until prepareForSplit() runs. Format-v2 calls that new method, but the existing be/src/format/table/paimon_jni_reader.cpp path still passes paimon_split in the constructor params and then uses JniReader::open()/getNextBatchMeta() directly, with no prepareForSplit call. That path is still reachable whenever enable_file_scanner_v2=false (and for paths where v2 is not selected), so the first read hits Preconditions.checkState(reader != null, "Paimon split is not prepared") and the scan fails. Please keep backward compatibility by either preparing the constructor-provided split in open() when present, or teaching the legacy C++ reader to invoke prepareForSplit before reading.
| return Status::OK(); | ||
| } | ||
|
|
||
| Status PaimonJniReader::close() { |
Contributor
There was a problem hiding this comment.
close() should still run the base JNI cleanup even if resetting the current split reports an error. resetCurrentSplit() can throw when releasing an active batch or closing the Paimon reader, and Java close() is explicitly written to keep going with IOManager cleanup/metric accounting after that reset error. This C++ override returns immediately, so JniTableReader::close() never reaches the idempotent releaseTable() or Java close() call, and FileScannerV2::close() has already marked the scanner closed so it will not retry. Please preserve the first reset error but still attempt JniTableReader::close() before returning.
zhangstar333
force-pushed
the
paimon-reuse-scanner
branch
from
586b2f6 to
78de427
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29282 ms |
Contributor
TPC-DS: Total hot run time: 177652 ms |
Contributor
ClickBench: Total hot run time: 24.95 s |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29352 ms |
Contributor
TPC-DS: Total hot run time: 176910 ms |
Contributor
ClickBench: Total hot run time: 24.83 s |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Problem Summary:
beore the PaimonJniReader handle multi splits one by one: eg:
PaimonJniReader(BE) ---> split1 ----> PaimonJniScanner(FE) -----> open -------> get_next-------->close.
split mapping to PaimonJniScanner, one split will new PaimonJniScanner object and deserialize, open, get_next, close.
so those will make JVM more pressure when have lost of splits.
now add reset logical, eg:
PaimonJniReader hold PaimonJniScanner not close it, when finish first split and reset some state, continue to handle next splits.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)