Repository navigation
[fix](orc) handle cancellation during condition cache seek - #67141
Merged
Merged
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 |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
yiguolei
approved these changes
Aug 31, 2026
github-actions Bot
pushed a commit
that referenced
this pull request
Aug 31, 2026
### 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
- [x] Regression test
- [x] Unit Test
- [ ] Manual test
- [ ] No need to test or manual test.
Verification: NewOrcReaderTest.* passed 163/163 under ASAN_UT.
Clang-format 16 passed for both affected C++ files.
- Behavior changed:
- [ ] No.
- [x] Yes. Cancellation during a condition-cache seek now returns clean
EOF instead of allowing the ORC stop exception to escape.
- Does this need documentation?
- [x] No.
- [ ] Yes.
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label
yiguolei
pushed a commit
that referenced
this pull request
Aug 31, 2026
…#67141 (#67321) Cherry-picked from #67141 Co-authored-by: Gabriel <[email protected]>
suxiaogang223
added a commit
to suxiaogang223/doris
that referenced
this pull request
Oct 23, 2026
### What problem does this PR solve? Issue Number: N/A Related PR: apache#67141 Problem Summary: The legacy ORC reader performs RowID seekToRow before the exception handlers around nextBatch. A seek can read a missing or unreadable file and throw orc::ParseError, which escapes the external row-fetch worker because its Doris exception boundary does not catch std::exception. Catch seek exceptions before dispatching either lazy or non-lazy reads and return an error with the file and cause. Keep the existing clean-EOF convention only for a stopped reader throwing "stop"; report other I/O failures even if cancellation races with them. Set the row count to zero when no requested RowIDs remain. ### Release note Fix BE termination when an ORC RowID seek fails during external TopN late materialization. ### Check List (For Author) - Test: Add real-ORC seek unit cases and an injected-error Hive TopN regression. Build hygiene, clang-format 16, diff checking and Groovy syntax compilation passed. BE build is blocked by the missing LLVM 20 third-party toolchain; UT download was stopped before execution; regression framework rejects existing duplicate Gson declarations. clang-tidy lacks a compilation database. Runtime verification remains for CI. - Behavior changed: Yes. Seek exceptions return errors; cancelled stop reads retain EOF behavior. - Does this need documentation: No
Gabriel39
pushed a commit
that referenced
this pull request
Oct 27, 2026
### What problem does this PR solve? Issue Number: N/A Related PR: #67141 The legacy ORC reader performs RowID seekToRow before the exception handlers around nextBatch. A seek can throw orc::ParseError while reading a missing or unreadable file, and that std::exception can escape the external row-fetch worker and terminate the BE. Catch seek exceptions before lazy or non-lazy reads and return an error with the file and cause. Preserve Doris exception propagation and error classification. Keep clean EOF only for a stopped reader throwing "stop"; report unrelated I/O errors even when cancellation races with them. Return zero rows when no requested RowIDs remain. The injected-error regression lives in its own `p0,external,hive,nonConcurrent` suite with a separate catalog and the preloaded Hive fixture. It pins the legacy-reader and TopN settings, asserts phase-two materialization, checks the expected seek failure, and verifies a successful five-row query after removing the debug point. Debug-point cleanup and session restoration run in finally blocks. The original TopN suite retains its concurrent correctness coverage. The branch has been rebased onto master `6e5a8929fba`, including the unrelated BE UT compile fix #69487. ### Release note Fix BE termination when an ORC RowID seek fails during external TopN late materialization. ### Check List (For Author) - Test: - Added real-ORC seek unit cases for lazy/non-lazy errors, cancellation, Doris exception propagation, empty RowIDs and normal seek progression. Runtime verification remains for CI. - Groovy 4.0.19 CONVERSION-phase compilation passed for both regression suites. - Local mock execution passed serial-group validation, independent catalog setup, recovery-query execution, and debug-point/session cleanup after success, unexpected query failure and partial enable failure. - git diff --check passed. - ./run-regression-test.sh --compile remains blocked by pre-existing duplicate Gson declarations rejected by the local Maven installation. Mocks do not replace live cluster regression. - BE UT and External Regression reruns requested for the updated head. The external runner enables -runNonConcurrent true. - Behavior changed: Yes. Seek exceptions return errors; cancelled stop reads retain EOF behavior. - Does this need documentation: No. ### Self-review - Goal and scope: repair the seek exception boundary and move only its global injection into an independently runnable serial suite. - Parallel paths: lazy and non-lazy seeks share the exception boundary; Doris errors retain their worker-side handling. - Concurrency and lifecycle: the all-BE injection is serial, scoped to one debug point, and removed before recovery. Session settings are restored on every exit. - Coverage and observability: the plan assertion ensures phase-two materialization; expected error matching and recovery remain enforced. The returned error includes the file and underlying cause. - Compatibility: no new configuration, FE/BE protocol, persistence, transaction or storage-format changes.
5 of 7 tasks
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)