[fix](orc) handle cancellation during condition cache seek - #67141
Open
Gabriel39 wants to merge 1 commit into
Open
[fix](orc) handle cancellation during condition cache seek#67141Gabriel39 wants to merge 1 commit into
Gabriel39 wants to merge 1 commit into
Conversation
Condition-cache granule skipping may perform ORC I/O before nextBatch. Keep those seeks inside the existing cancellation boundary so a stop exception returns clean EOF instead of escaping. Add a deterministic regression test for cancellation during seek. Jira: DORIS-28199
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review result: no actionable code findings. The implementation is suitable for approval, subject to the remaining required CI checks.
Critical checkpoint conclusions:
- Goal and proof: The change prevents a cancellation exception raised during a condition-cache
seekToRow()from escapingOrcReader::get_block(). The focused BE unit test enters that exact cancellation window and verifies OK status, clean EOF, zero rows, and an empty block. - Scope and clarity: The production change is narrowly limited to moving condition-cache seeking into the existing ORC stop-aware exception boundary; the added debug hook exists only to exercise that boundary in the regression test.
- Concurrency and lifecycle: No new threads, locks, shared ownership, or static initialization are introduced. On cancellation, output is cleared,
_eofbecomes terminal, successful-seek cursor/counter updates are skipped, andTableReaderdoes not publish an incompletely visited condition-cache result. - Error semantics and parallel paths: All three condition-cache seek branches now share the boundary. Only the exact production stop signal (
should_stopplus"stop") becomes clean EOF; other ORC exceptions remain non-OK failures. This matches the established v1 ORC behavior. - Compatibility and configuration: No storage format, schema, FE/BE protocol, persisted metadata, configuration, rolling-upgrade, SARG, Split/stripe, delete, or residual-predicate contract changes.
- Performance and observability: The widened
tryadds no material work. Existing filtered-row accounting changes only after successful seeks, and non-cancellation errors retain their reason. No additional metric or log is needed for the expected cancellation path. - Test and gate status: The focused unit coverage is appropriate and adjacent tests cover ordinary condition-cache skipping and pre-read cancellation. Clang Formatter and the corrected PR-title check pass. BE UT, compile, and performance checks were still pending at review time; no builds or tests were rerun in this review-only runner.
- Transactions, writes, persistence, and new FE/BE variables: Not applicable to this reader-only cancellation fix.
- User focus: No additional focus was provided. One normal full-review agent and one separate risk-focused agent both returned
NO_NEW_VALUABLE_FINDINGSin round 1 after rechecking missed paths.
Completion status: complete for code review at head cc7590f1b1e23cff10780613ec2e5817e7a8ba5c. No inline comments or critical code blockers were found; merge remains subject to the pending required CI checks.
Contributor
TPC-H: Total hot run time: 16993 ms |
Contributor
TPC-DS: Total hot run time: 82478 ms |
Contributor
ClickBench: Total hot run time: 15.01 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
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?
Issue Number: DORIS-28199
Problem Summary:
Condition-cache granule skipping can call ORC seekToRow before the existing nextBatch exception boundary. When a query is cancelled during that I/O, the ORC input stream throws a stop exception that escapes get_block and may terminate the BE process.
Release note
Fix BE process termination when an ORC scan is cancelled during condition-cache seeking.
Check List (For Author)
Test
Verification: NewOrcReaderTest.* passed 163/163 under ASAN_UT. Clang-format 16 passed for both affected C++ files.
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)