[fix](paimon) Correct pruned struct field reads - #66223
Merged
Gabriel39 merged 1 commit intoJul 29, 2026
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Gabriel39
marked this pull request as ready for review
July 29, 2026 07:23
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Contributor
TPC-H: Total hot run time: 29591 ms |
Contributor
TPC-DS: Total hot run time: 177962 ms |
Contributor
ClickBench: Total hot run time: 25.13 s |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review completed for head f02cd306e8858d0cd655a4948b6b58d8cc4dc32d.
Requesting changes because the advertised pruned-struct failure is not reachable through the current Paimon scanner bridge, and the new tests manufacture the missing original-index list instead of exercising the production caller.
Critical checkpoint conclusions:
- Goal and proof: not achieved. The helper-level binary-row tests fail on the old code, but no production Paimon scan can currently supply their
[2]or[0,2]index lists. - Scope and clarity: the patch is small, but the new comment states an original-child-index invariant that the production bridge does not provide.
- Concurrency and lifecycle: not applicable; the change is synchronous row decoding and adds no managed lifetime.
- Configuration, persistence, transactions, data writes, and FE/BE variables: no changes.
- Compatibility and parallel paths: no serialized-format change. Legacy JNI, Format V2 JNI, and recursive struct/array/map paths all retain full struct shapes today, so the same reachability gap applies throughout.
- Error handling and observability: no new error boundary or operational signal is needed for this local helper change.
- Tests/results: no local build or test was run because the review runner explicitly forbids builds. Static review found that the added unit tests bypass
VectorColumn.appendValue, so passing results do not establish the claimed end-to-end fix. - Performance: no reachable hot-path behavior changes at this head.
There was no additional user-provided review focus; the complete two-file PR and its upstream/downstream paths were reviewed.
| InternalRow row = record.getRow(idx, structFieldIndex.size()); | ||
| RowType rowType = (RowType) dataType; | ||
| // Projection entries are original child indexes, so the binary row must keep the full RowType arity. | ||
| InternalRow row = record.getRow(idx, rowType.getFieldCount()); |
Contributor
There was a problem hiding this comment.
[P2] Reproduce this through the scanner's real index list
This line changes behavior only when structFieldIndex.size() differs from the full Paimon RowType arity, but that state is not reachable today. LogicalFileScan.supportPruneNestedColumn() excludes Paimon, both JNI paths therefore serialize the full Doris struct, and VectorColumn generates the full dense [0..N-1] list; PaimonJniScanner also applies only a top-level projection. The old size already equaled the full row arity. The new tests bypass this bridge by directly injecting [2] and [0, 2], so they fail before the patch without reproducing a scanner request. Please wire the actual nested-field mapping through the JNI path and test it through VectorColumn.appendValue/the scanner boundary; as written, the advertised production failure is not fixed.
Contributor
FE Regression Coverage ReportIncrement line coverage |
Mryange
approved these changes
Jul 29, 2026
12 tasks
github-actions Bot
pushed a commit
that referenced
this pull request
Jul 29, 2026
## Proposed changes - Preserve the full Paimon `RowType` arity when reading projected struct fields. - Add unit coverage for non-leading, non-contiguous, and complete struct projections backed by a binary row. ## Problem summary Struct projection entries use original child indexes, but the scanner initialized the nested binary row with the number of projected fields. A projection such as the third child therefore created a one-field row and then accessed index two, producing incorrect reads or an out-of-bounds failure. ## Regression coverage - `testUnpackNonLeadingStructField` constructs a three-field binary row and projects only original field index 2. The previous implementation fails with `index (2) should < 1`. - `testUnpackNonContiguousStructFields` projects original field indexes 0 and 2. The previous implementation fails with `index (2) should < 2`. - `testUnpackCompleteStruct` protects the existing full-struct behavior. - With this fix restored, all three tests pass with Maven build caching disabled. ## Validation - Reverted the row-arity fix locally and confirmed that the two pruned-field regression tests fail. - Restored the fix and reran the same test class: 3 tests, 0 failures, 0 errors. - Ran all Paimon scanner test classes with Maven build caching disabled: 15 tests, 0 failures, 0 errors. - Checkstyle passed for the affected reactor modules.
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
3 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.
Proposed changes
RowTypearity when reading projected struct fields.Problem summary
Struct projection entries use original child indexes, but the scanner initialized the nested binary row with the number of projected fields. A projection such as the third child therefore created a one-field row and then accessed index two, producing incorrect reads or an out-of-bounds failure.
Regression coverage
testUnpackNonLeadingStructFieldconstructs a three-field binary row and projects only original field index 2. The previous implementation fails withindex (2) should < 1.testUnpackNonContiguousStructFieldsprojects original field indexes 0 and 2. The previous implementation fails withindex (2) should < 2.testUnpackCompleteStructprotects the existing full-struct behavior.Validation