[fix](Meta path) nested column prunning meta path--test private branch - #66215
Draft
englefly wants to merge 8 commits into
Draft
[fix](Meta path) nested column prunning meta path--test private branch#66215englefly wants to merge 8 commits into
englefly wants to merge 8 commits into
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Nested-column pruning uses terminal NULL and OFFSET components to read only null maps or offsets. The legacy wire format encodes every access path as DATA, which makes those metadata selectors ambiguous with real nested fields that have the same names. Introduce an optional access-path version, preserve it through Thrift and protobuf descriptors, and use the path type to select the authoritative DATA or META payload in the typed version. New BEs continue to decode the legacy all-DATA format from old FEs, while the supported rolling-upgrade order upgrades all BEs before the FE emits typed paths. Refactor nested iterator planning to separate current data, current metadata, and descendant routing, and keep sibling META paths explicit so unrelated DATA projections do not disable metadata-only reads.
### Release note
Fix metadata-only nested-column pruning and distinguish metadata selectors from same-named data fields.
### Check List (For Author)
- Test: Unit Test and Regression Test
- `./run-fe-ut.sh --run 'org.apache.doris.nereids.rules.rewrite.PruneNestedColumnTest,org.apache.doris.analysis.DescriptorToThriftConverterTest'` (74 passed on the final rebased FE tree)
- `./run-be-ut.sh --run --filter='ColumnReaderTest.*:SlotDescriptorTest.AccessPathsPreservedThroughProtobuf'` (50 passed before the final rebase; the final rebuild was blocked before GTest by the missing master-added `liblance_c.a`)
- `nereids_rules_p0/column_pruning/string_length_column_pruning`, `nereids_rules_p0/column_pruning/null_column_pruning`, and `nereids_rules_p0/column_pruning/nested_container_offset_pruning` (passed before the final rebase; the final rerun was blocked by the same missing dependency)
- Behavior changed: Yes. Version-1 paths use explicit DATA/META payload semantics, while absent/version-0 paths retain the legacy encoding.
- Does this need documentation: No
Co-authored-by: minghong <zhouminghong@selectdb.com>
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: The descendant access-path router used generic route terminology for container-specific selection, rewriting, and distribution, and referred to the payload selected by the path type as normalized or active. Rename the internal helpers and result fields to describe their actual behavior, document selected-payload semantics, and keep routing behavior unchanged.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- `./run-be-ut.sh --run --filter='ColumnReaderTest.*'` was attempted, but no test binary ran because `thirdparty/installed/lib64/liblance_c.a` is missing.
- `build-support/check-format.sh`
- Behavior changed: No. This only renames internal helpers, result fields, local variables, and tests, and clarifies comments.
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Map descendant routing rewrote logical KEYS and VALUES selectors to physical key/value child column names before forwarding paths. Those physical names are storage/schema details and should not participate in the access-path contract. Make Map child iterators consume KEYS/VALUES as their stable logical names, keep explicit selectors unchanged, and expand wildcard paths directly to KEYS/VALUES. Add focused coverage for physical-name independence and nested Map wildcard routing.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- `build-support/check-format.sh`
- `./run-be-ut.sh --run --filter='ColumnReaderTest.*'` (blocked before compilation because `thirdparty/installed/lib64/liblance_c.a` is missing)
- Behavior changed: No. Map routing uses the same key/value children but no longer depends on their physical names.
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: DescendantAccessPathRouter documented its input assumptions but not the complete per-level processing flow. Add a class-level comment describing common path splitting, current DATA and metadata handling, container-specific Struct/Array/Map routing, child read requirements, and recursive nested processing.
### Release note
None
### Check List (For Author)
- Test: No need to test. Comment-only change.
- `build-support/check-format.sh`
- `git diff --check`
- Behavior changed: No.
- Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#65591 Problem Summary: Map descendant access-path routing silently ignored selectors other than `*`, `KEYS`, and `VALUES`. This could drop the only descendant path and prune both key and value iterators instead of reporting an invalid FE-BE access-path contract. Reject invalid selectors during classification while continuing to inspect only the payload selected by the path type. ### Release note None ### Check List (For Author) - Test: Unit Test - Added `ColumnReaderTest.MapRejectsUnrecognizedDescendantSelectors` - Focused UT execution was blocked before linking because `thirdparty/installed/lib64/liblance_c.a` is missing - `build-support/check-format.sh` - `git diff --check` - Behavior changed: Yes. Invalid Map descendant access-path selectors now return an internal error instead of being silently ignored. - Does this need documentation: No
Issue Number: None Related PR: apache#65591 Problem Summary: Emitting the parent Struct null path for `element_at` null predicates changes the nested-column plan from a child-only predicate path to parent and child null paths. Update the regression assertions and comments to validate both paths. Also replace a misplaced Javadoc marker that caused FE Checkstyle to fail. None - Test: Regression test / Manual test - `./build.sh --fe` - `./run-regression-test.sh --conf output/local-regression/regression-conf-20000.groovy --run -d nereids_rules_p0/column_pruning -s null_column_pruning` - `git diff --check` - Behavior changed: No. This updates validation for the existing Struct null-path behavior. - Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
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.
Request changes. The typed DATA/META contract is coherent for current FE/current BE peers, but two blocking issues remain: version 1 is emitted without rolling-upgrade negotiation, and a changed regression assertion contradicts the planner contract established by this PR's FE unit tests.
Critical checkpoint conclusions:
- Goal and proof: the change disambiguates real nested fields from NULL/OFFSET metadata on current peers, but the goal is not safely met during mixed-version execution, and one end-to-end EXPLAIN expectation cannot match the collector output.
- Scope: the implementation is focused on access-path collection, wire propagation, BE routing, and tests; no unrelated behavior was found.
- Concurrency and lifecycle: planning, descriptor conversion, and iterator setup are query-local. No shared mutable state, locking, background lifecycle, or static-initialization issue applies.
- Configuration: no configuration item is added.
- Compatibility: blocking. Old BEs ignore the new version/type semantics, causing both a hard META-path failure and a silent reserved-name DATA misread.
- Parallel paths and conditions: direct Thrift, point query, row-id/materialization protobuf forwarding, OLAP reads, Struct/Array/Map/Variant routing, external/TVF normalization, casts, and wildcard paths were traced. Current-peer routing is consistent, while forwarding preserves the mixed-version incompatibility rather than negotiating it.
- Tests and results: current-peer FE/BE unit coverage is broad, but no mixed-version test covers either compatibility failure. The changed regression expectation is inconsistent with the changed FE unit test and actual collector/printing path. Tests were not run because this is a review-only environment.
- Observability: no additional log or metric is needed for this query-local representation change.
- Persistence, transactions, and data writes: not involved.
- FE-BE variable propagation: version, type, and selected payloads are wired through current-peer Thrift/protobuf paths; mixed-version semantics are not safe.
- Performance: no material hot-path regression or missed obvious optimization was substantiated.
- Other issues: none after three full review/convergence rounds; every final reviewer returned NO_NEW_VALUABLE_FINDINGS for this exact two-comment set.
User focus: no additional review focus was supplied, so the complete PR was reviewed.
| public static TColumnAccessPath toThrift(ColumnAccessPath accessPath) { | ||
| TColumnAccessPath result = new TColumnAccessPath( | ||
| accessPath.getType() == ColumnAccessPathType.DATA ? TAccessPathType.DATA : TAccessPathType.META); | ||
| result.setVersion(DescriptorsConstants.TCOLUMN_ACCESS_PATH_VERSION_TYPED); |
Contributor
There was a problem hiding this comment.
[P1] Negotiate typed access paths before enabling version 1
This enables version 1 unconditionally, but an old BE ignores the unknown version field. For META paths, the converter populates only meta_access_path while the old reader still consumes data_access_path.path, so ordinary IS NULL/length queries fail with an empty-path error, including after BE-to-BE descriptor forwarding. Adding that compatibility payload alone is not enough: for DATA ["s","NULL"], the old Struct reader treats NULL as the legacy parent-null sentinel and skips the real same-named child. Please capability-gate typed encoding until every target BE supports it, or emit complete legacy-safe fallbacks (a duplicate data payload for META and a whole-parent DATA path for reserved-name fields), and cover both direct and forwarded mixed-version decoding.
| contains "nested columns" | ||
| contains "struct_col.city" | ||
| contains "predicate access paths: [struct_col.city.NULL]" | ||
| contains "predicate access paths: [struct_col.NULL, struct_col.city.NULL]" |
Contributor
There was a problem hiding this comment.
[P1] Match the predicate path this planner actually emits
The changed FE unit test for this same query shape expects only META s.city.NULL, and the collector creates exactly that one suffix: visitIsNull() adds NULL, visitElementAt() prepends city, and descriptor translation copies the list unchanged. printNestedColumns() also prints the actual predicate list, so it cannot contain the exact string [struct_col.NULL, struct_col.city.NULL] and this regression assertion will fail. Please remove the parent entry from this expectation (and the duplicate expectation below), or implement explicit ancestor META paths and update the FE unit contract consistently.
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.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)