Repository navigation
[fix](be) Catch ORC exceptions during RowID seeks - #69465
Merged
Gabriel39 merged 4 commits intoOct 27, 2026
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28339 ms |
Contributor
TPC-DS: Total hot run time: 150717 ms |
Contributor
ClickBench: Total hot run time: 24.24 s |
### 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
### What problem does this PR solve? Issue Number: N/A Related PR: N/A Problem Summary: Doris exceptions also derive from std::exception. Preserve their existing worker-side error-code handling, including memory-limit classification, instead of converting them into generic ORC I/O errors. Add lazy/non-lazy real-seek unit cases for Doris exception propagation and keep the test stream fault behavior reusable. ### Release note None ### Check List (For Author) - Test: Build-hygiene gates and clang-format 16 passed. Added two parameterized Doris exception cases; runtime BE/UT and sanitizer validation remain blocked by the local third-party toolchain/download environment. The SQL regression is syntax-compiled but its runner is blocked by existing duplicate Gson dependencies. - Behavior changed: No additional user-visible behavior beyond the ORC seek fix; existing Doris error classification is preserved. - Does this need documentation: No
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69465 Problem Summary: The TopN Hive suite runs concurrently, so the framework rejects its new all-BE debug point before the ORC seek is exercised. Move the injection into a separate nonConcurrent suite with its own catalog and explicit legacy-reader/cache/TopN settings. Keep the plan assertion and expected seek error, verify a successful five-row query after cleanup, and restore session settings on every exit. The original TopN suite retains its existing parallel correctness coverage. ### Release note None ### Check List (For Author) - Test: Manual test - Groovy 4.0.19 CONVERSION-phase compilation passed for both suites. - Local mock execution passed serial-group validation, independent catalog setup, recovery query, and debug-point/session cleanup on success, unexpected query error and partial debug-point 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. Full external execution and BE UT are delegated to the requested CI reruns. - Behavior changed: No production behavior changes. - Does this need documentation: No
suxiaogang223
force-pushed
the
codex/fix-orc-rowid-seek-exception
branch
from
October 23, 2026 07:02
f29710b to
5b85b28
Compare
Member
Author
|
run beut |
Member
Author
|
run external |
Member
Author
|
run buildall |
Member
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.
Reviewed exact head 5b85b28. No new actionable defects found, and there are no existing inline findings to carry forward.
Critical checkpoint conclusions:
- Goal: the shared seek boundary converts standard ORC seek exceptions into query errors before either legacy read branch, while rethrowing Doris exceptions to preserve worker error classification.
- Scope: the four-file change is focused; moving the private helper into the source file keeps debug-point dependencies out of the header.
- Concurrency: per-task reader ownership and existing status-before-completion publication are preserved; no new locks or thread interactions. The global fault regression uses the serial suite group.
- Lifecycle: scanner/profile/stream/IOContext lifetimes and unit-fixture destruction order are consistent; no new ownership cycles or static initialization dependency.
- Configuration: no new settings; the regression restores the three session values it changes.
- Compatibility: no public interface, class layout, storage format, or new libc API change.
- Thrift/protobuf: unchanged; field-ID compatibility checks are not applicable.
- Parallel paths: both legacy lazy modes share this boundary. Condition-cache seeks and V2 RowID seeks already have exception boundaries.
- Conditions: clean EOF requires cancellation plus the exact stop token; unrelated I/O failures remain errors, failed seeks retain their RowID, and exhausted IDs return zero rows.
- Test coverage: eight unit cases parameterized across both lazy flags cover errors, cancellation classification, Doris codes, empty IDs, and seek progression. The independent Hive regression checks materialization, injected worker failure, fault cleanup, and a five-row recovery query.
- Test results: no output baselines changed. Fixture provisioning, expected error text, row-count assertions, and test DSL were inspected.
- Observability: the new error carries the file path and original cause through existing Status propagation.
- Transactions/persistence: unchanged; visibility, delete-bitmap, EditLog, and failover protocols are outside this read-side fix.
- Data writes/atomicity: no production write path changes.
- FE/BE variables: none added or changed.
- Memory/performance: no new significant retained allocation, successful-read copying, or asymptotic cost; existing task memory context remains attached.
- Other concerns: no additional user focus was supplied, and no unresolved code candidate remains after the complete runtime, test, and focused-risk reviews.
Limits: static review only; no builds, unit/regression tests, or completed ELF compatibility audits were run. The new unit cases stop before successful nextBatch decoding, cancellation uses an initially empty block, and recovery is checked with a fresh query rather than reuse of a failed reader. The shared boundary placement, clearing branch, existing initialized reader tests, and regression provide complementary static coverage; runtime validation remains for CI.
Review converged after 2 round(s).
Contributor
TPC-H: Total hot run time: 28468 ms |
Contributor
TPC-DS: Total hot run time: 150858 ms |
Contributor
ClickBench: Total hot run time: 24.53 s |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69465 Problem Summary: The ORC RowID seek unit tests expected a wrapped input stream to fail during seekToRow. For the fixture file, seekToRow did not read the stream, so the test entered nextBatch with an uninitialized batch and aborted. Inject the requested exception directly at the existing seek debug point and initialize the test batch as a real reader does. Keep the SQL regression default debug-point failure unchanged. ### Release note None ### Check List (For Author) - Test: Formatting and build hygiene checks passed; BE UT is pending CI. No local compilation or unit tests were run. - Behavior changed: No; this changes test injection and the existing debug point callback path only. - Does this need documentation: No
Member
Author
|
run buildall |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 28829 ms |
Contributor
TPC-DS: Total hot run time: 151453 ms |
Contributor
ClickBench: Total hot run time: 24.43 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
No actionable defects found in the four-file diff at 4acaeb7. Two complete-scope reviewers and a separate exception/cancellation reviewer returned NO_NEW_VALUABLE_FINDINGS. The main review independently verified their evidence. Existing inline threads are empty; no existing P0/P1 finding is carried forward. No additional user focus was provided.
Critical checkpoint conclusions:
- Goal and evidence: the common seek boundary converts ORC/std exceptions to a checked error with file/cause, preserves Doris exception propagation, and returns zero rows on RowID exhaustion. Added unit and SQL cases target these behaviors; they were inspected, not executed.
- Scope and clarity: the helper extraction, exception boundary, injection point, unit cases, and independent regression are focused on the seek failure.
- Concurrency and locks: the existing external fetch worker catches Doris exceptions, publishes status before completion notification, releases its semaphore, and waits for accepted tasks before releasing captured state. This change introduces no new locks or lock order. Global SQL injection runs in the nonConcurrent single-suite executor.
- Lifecycle and memory: scanner/profile/IOContext ownership remains valid through worker completion. Unit callbacks are removed before fixture destruction; nested finally blocks remove the SQL point and restore settings. No new persistent allocation or cross-TU initializer is introduced.
- Configuration: no configuration item is added. The regression scopes and restores the three existing session settings needed to select legacy materialization and avoid cached results.
- Compatibility: no persisted format or public protocol changes. The internal helper extraction introduces no new libc API, linker, or toolchain dependency; final ELF/glibc compatibility was not audited without built artifacts.
- Thrift/protobuf: no gensrc definitions or field IDs change.
- Parallel paths: both legacy lazy/non-lazy modes share the boundary. Existing condition-cache seeks are already guarded; V2 ORC already guards RowID seeks in its own read boundary. Parquet uses a separate row-range mechanism.
- Conditional behavior: only the established stop flag plus exact "stop" sentinel becomes clean EOF. Unrelated I/O failures remain errors during cancellation; seek failure leaves the requested RowID unconsumed. Comments explain the exception boundary and injection use.
- Test coverage: eight parameterized unit cases cover seek errors, Doris errors, cancellation before/during seek, stop without cancellation, unrelated errors during cancellation, empty IDs, and seek progression. The Hive2 fixture is independently preloaded; the regression requires VMaterializeNode, the unique injected error, and a successful five-row query afterward.
- Test results: no expected-output files change. SQL uses ORDER BY and test/sql/exception; recovery checks row count. Unit lazy flags cover the common pre-read boundary, rather than independently proving successful decoding in both modes.
- Observability: seek errors include the file and underlying cause; existing scanner/RPC layers preserve the error through query failure. Additional logs or metrics are unnecessary for this localized boundary.
- Transactions and persistence: no commit, replay, version visibility, or delete-bitmap behavior changes.
- Data writes and atomicity: production changes only reads; the regression creates its private catalog and reads existing fixtures. No new data-write/crash-atomicity path is introduced.
- FE/BE variables: none added; the existing query option already governs both scanner phases.
- Performance: no extra row scans, data copies, or unbounded buffers. The success path adds a disabled debug-point check and an out-of-line helper call; performance was not benchmarked.
- BE conventions and other issues: includes stay in implementation/test files; no new PCH, header-budget, explicit-instantiation, source-including test, or unity-skip obligation is introduced. The final sweep found no additional actionable issue.
Coverage limits: static inspection only. No builds, tests, benchmarks, formatting/hygiene execution, or live Hive/BE verification was performed. Third-party ORC internals were not rebuilt or executed, and external CI invocation flags outside this checkout were not verified. No unresolved source-level candidate or uncovered changed-file scope remains. Review converged in round 1.
Review converged after 1 round(s).
Gabriel39
approved these changes
Oct 27, 2026
Contributor
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
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: 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,nonConcurrentsuite 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)
Self-review