Paimon reuse scanner#65404
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
17d7a1e to
c273a9f
Compare
|
/review |
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); | |||
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() { |
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.
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)