Skip to content

Paimon reuse scanner#65404

Open
zhangstar333 wants to merge 3 commits into
apache:masterfrom
zhangstar333:paimon-reuse-scanner
Open

Paimon reuse scanner#65404
zhangstar333 wants to merge 3 commits into
apache:masterfrom
zhangstar333:paimon-reuse-scanner

Conversation

@zhangstar333

@zhangstar333 zhangstar333 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

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

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason
  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@zhangstar333 zhangstar333 force-pushed the paimon-reuse-scanner branch from 17d7a1e to c273a9f Compare July 9, 2026 07:20
@zhangstar333

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants