[fix](parquet) Restore column ownership after interrupted reads - #66780
Open
hubgeter wants to merge 1 commit into
Open
[fix](parquet) Restore column ownership after interrupted reads#66780hubgeter wants to merge 1 commit into
hubgeter wants to merge 1 commit into
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: The direct Parquet conversion path temporarily moves the caller-owned column into the physical read column. If an interrupted read returns before conversion, the caller is left with a null column pointer. Complex-column cleanup can then dereference that null child and crash the BE. Restore the transferred column on every early return, and disarm the restoration only after conversion moves ownership back successfully. Add a focused test that triggers the IO stop path and verifies the caller retains a valid column.
### Release note
Fix an occasional BE crash when a Parquet scan is interrupted.
### Check List (For Author)
- Test: Unit Test (added but not run; compilation was stopped at the user request)
- Added `ParquetColumnChunkReaderTest.ScalarNestedReadRestoresColumnWhenStopped`
- `build-support/check-format.sh`
- `git diff --check`
- Behavior changed: Yes. Interrupted direct Parquet reads now preserve caller column ownership.
- Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 17612 ms |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review conclusion: approve (comment-only; no inline findings).
Completion status: converged after Round 1. The main-agent review, two complementary full-scope subagent reviews, and the separate ownership-risk review found no new valuable issues. All three initial risk items were independently resolved safe, and the final changed-file/unresolved-candidate sweep found zero outstanding points.
Critical checkpoint conclusions:
- Goal and proof: The change prevents direct Parquet reads from leaving the caller's
ColumnPtrnull when a nested/page read exits before conversion. The scope guard restores the sole live owner on every armed early return, and disarms only after the consistent converter moves ownership back. The new test selects BYTE_ARRAY-to-String direct conversion, forces the nested page-header stop after the move, and would fail atASSERT_NE(column, nullptr)on the base implementation. - Scope and focus: The patch is small and focused: one ownership guard plus one targeted BE unit test. There was no additional user focus; the entire two-file PR was reviewed.
- Error handling and lifecycle: Array, Map, and Struct pass embedded child slots by reference and propagate child errors immediately, so the leaf guard restores the child before any parent unwinds or block cleanup calls recursive
is_exclusive(). Partial decode state is discarded on stop/error and is neither retried nor published. The guard closure performs no fallible cleanup. - Concurrency and static lifecycle: The flag, guard, and pointers are invocation-local. No locks, shared mutable state, thread-entry behavior, atomics, cross-TU statics, or deadlock surface are introduced.
- Conditions, parallel paths, and compatibility: The guarded condition exactly matches
convert()'s consistent physical/logical move-only success branch. Nested and non-nested reads, offset-index specializations, dictionary-filter direct reads, page transitions, and complex-parent shapes share the same template body. No configuration, protocol, storage-format, symbol, rolling-upgrade, FE/BE variable, persistence, transaction, or data-write behavior changes. - Tests and results: The deterministic unit test covers the reported cancellation regression and is included by the existing BE-UT glob. No local build or test was run because the authoritative review instructions prohibit builds in this runner. GitHub's Clang Formatter check passed; BE-UT and compile checks were still pending at review time. No regression
.outresult is involved. - Performance, memory, and observability: The local scope guard adds no heap allocation or column clone and preserves the exclusive-owner COW fast path. No new material memory-accounting, logging, metric, or documentation need was identified.
- Other correctness risks: No transaction visibility, delete-bitmap, nullable-shape, schema/persistence, crash-recovery, or parallel-path issue is implicated by this change, and no additional substantiated bug was found.
User focus response: no additional focus was provided; the full PR and its upstream/downstream ownership and cleanup paths were reviewed.
Contributor
TPC-DS: Total hot run time: 85377 ms |
Contributor
ClickBench: Total hot run time: 14.68 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
2 participants
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: None
Related PR: None
Problem Summary: The direct Parquet conversion path temporarily moves the caller-owned column into the physical read column. If an interrupted read returns before conversion, the caller is left with a null column pointer. Complex-column cleanup can then dereference that null child and crash the BE. Restore the transferred column on every early return, and disarm the restoration only after conversion moves ownership back successfully. Add a focused test that triggers the IO stop path and verifies the caller retains a valid column.
coredump:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)