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
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
run buildall |
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
FE UT Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: #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: #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
TPC-H: Total hot run time: 29551 ms |
TPC-DS: Total hot run time: 178285 ms |
ClickBench: Total hot run time: 25.73 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review: changes requested
I found one blocking issue: the newly added outer-join FE unit test expects a DATA access path that the changed collector cannot emit for the nullable struct field declared by that fixture. The inline comment gives the exact trace and correction.
Critical checkpoint summary:
- Goal and scope: the PR introduces versioned typed DATA/META access paths and recursively routes metadata-only reads through scalar, Struct, Array, and Map iterators. The 14-file scope is cohesive around that protocol and pruning change.
- Correctness and tests: descriptor forwarding, nested routing, read phases, map wildcard expansion, physical-versus-synthetic nullability, and the added FE/BE test matrices were reviewed. One new FE assertion is internally inconsistent and deterministically fails; no other substantiated correctness issue remains.
- Compatibility: reachable old-FE to new-BE traffic remains compatible through legacy DATA encoding. New-FE to old-BE typed META traffic is mechanically incompatible, but it is outside Doris's supported BE-first rolling-upgrade order, so it is not reported as a defect. Thrift/protobuf forwarding otherwise preserves version, type, and selected payload.
- Parallel and special paths: external FileScan/TVF paths normalize META suffixes back to DATA before AccessPathParser; variant, MV-fragment, short-circuit, row-id, and deep nested paths were checked without another finding.
- Concurrency, lifecycle, configuration, persistence, and write paths: this PR does not introduce material changes in these areas. Error validation and status propagation are explicit, and routing work occurs during iterator setup rather than row hot loops.
- Validation: review was static because the review contract prohibited builds and this checkout lacks thirdparty/installed and protoc. Live CI had passed compilation, Checkstyle, Clang Formatter, license, and dependency checks at submission time; FE/BE and regression suites were still running. The macOS BE-UT failure was an unrelated runner JDK-version setup failure.
- User focus: no additional focus points were supplied.
FE UT Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 29454 ms |
TPC-DS: Total hot run time: 177205 ms |
ClickBench: Total hot run time: 25.57 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
924060929
left a comment
924060929
left a comment
There was a problem hiding this comment.
Summary
Verified locally against head aa59c38ee5b — FE unit tests plus targeted probes, not review-by-reading. Net result: the core fix works and is well designed, but one change regresses external tables, and the PR is described as an optimization when it is actually a wrong-result fix.
What this PR actually fixes
CREATE TABLE t(id INT, s STRUCT<`null`:INT, x:INT>)
DUPLICATE KEY(id) DISTRIBUTED BY HASH(id) BUCKETS 1 PROPERTIES("replication_num"="1");
INSERT INTO t VALUES(1, named_struct('null',100,'x',200));
SELECT s.`null` FROM t; -- pruning ON -> NULL (wrong); OFF -> 100The root cause is BE-side, not FE-side. StructField's constructor lowercases every field name, and pre-PR BE matched markers case-insensitively:
// pre-PR column_reader.cpp:93-100
return StringCaseEqual()(component, ColumnIterator::ACCESS_OFFSET) ||
StringCaseEqual()(component, ColumnIterator::ACCESS_NULL);so ["s","null"] was consumed as a null-map-only sentinel. FE was correct throughout — it compares case-sensitively, so "null".equals("NULL") never matched.
I confirmed the new (path.type == META || uses_legacy_encoding) gate fixes it. FE emits DATA:s.null / DATA:s.offset for projection, predicate and multi-field forms; setVersion(TYPED) has no bypass (DescriptorToThriftConverter:90 is the only construction site in FE, and both BE sites forward it); marker detection converged from two sites pre-PR to one, all behind the same gate, so arbitrary nesting depth is covered.
This is a data-correctness fix on any struct field named null or offset, at any nesting depth, on plain projection, with enablePruneNestedColumns defaulting to true.
Blocking
Nested pruning regresses on external tables — details inline on AccessPathPlanCollector.java. Measured, with the pre-PR baseline for comparison.
Please update the PR metadata
Title says [opt], the description is empty, and the release note is None. Two things belong there:
- This fixes a wrong result, with the repro above.
- BE must be upgraded before FE. During the window where BE is new and FE is old,
uses_legacy_access_path_encodingshort-circuits the gate and the wrong result persists — deliberately, and pinned byLegacyStructMetaComponentsRemainSentinels. Upgrading BE alone does not fix a customer already hitting this.
Verified correct
- BE handles
DATA+METAcoexisting on one slot correctly._check_and_set_meta_read_mode(column_reader.cpp:2582) unconditionally falls back toDEFAULTwhenallcarries any data requirement, so sibling data children are never skipped. Three UTs cover it. - External tables never had the case-collision bug:
access_path_parser.cpp:256/307compares case-sensitively and never tests"NULL". - The variant path deliberately does not lowercase (
AccessPathExpressionCollector:356vs:397), and META stripping there is positional (subList) rather than string-based — correct. PruneNestedColumnTestpasses 58/58 locally on this head, CheckStyle green. The open bot P1 ontestNotNullStructOnOuterJoinNullableSideis resolved byaa59c38ee5band can be dismissed.
CI
The last run buildall ran on f01b15a0a7d, four commits behind head, including two behaviour-changing [P1] commits. Current head shows a BE UT (macOS) failure. Worth re-triggering before merge.
Remaining points are inline.
| sql "select element_at(struct_col, 'city') from ncp_tbl where struct_col is null" | ||
| contains "nested columns" | ||
| contains "all access paths: [struct_col]" | ||
| contains "all access paths: [struct_col.city, struct_col.NULL]" |
There was a problem hiding this comment.
The behaviour change here has no data-level coverage.
Only EXPLAIN assertions changed in this file, and no .out file changed anywhere in the PR. None of the three tables in this suite has a row where a STRUCT column is itself NULL — ncp_tbl holds one row, named_struct('city', null, 'zip', 10001), which is a non-null struct with a null field. ncp_nested_tbl and struct_map_pruning_lazy_read are the same.
So the new two-layer null rule (element_at(s,'f') IS NULL always emitting [s,NULL]) is asserted as a plan shape and never as a result. If the previous behaviour returned wrong rows for a NULL struct, no case here would have failed before, and none passes differently now.
More importantly, the wrong result this PR actually fixes has no case at all. Please add, with data and result comparison:
STRUCT<\null`:INT>` + plain projection (the repro in the summary).- An
offsetvariant —STRUCT<\offset`:INT>takes theOFFSET_ONLY` branch rather than the NULL branch, so the symptom may be worse than returning NULL. v['NULL']on a VARIANT column. The variant path never lowercases (AccessPathExpressionCollector:356vs:397), so it collides regardless of case — pre-PR it was misclassified as metadata by the last-component string rule. This is a real bug this PR fixes, and it is currently untested.- A row where the struct column itself is NULL, to give the two-layer rule actual coverage.
| 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]" |
There was a problem hiding this comment.
These assertions cannot pin the thing this PR introduces.
EXPLAIN drops the access-path type:
// PlanNode.java:957
.map(a -> StringUtils.join(a.getPath(), "."))DATA:["s","NULL"] and META:["s","NULL"] both render as s.NULL. Every assertion in this file that mentions .NULL or .OFFSET would still pass if FE emitted the wrong type — blind to exactly the DATA/META distinction the PR is built on.
Suggest rendering the type (META:struct_col.NULL) so these assertions actually constrain it. That also makes the plan output diagnosable for the struct<\null`:int>` case, where the same string means two different things.
| Expression fieldName = arguments.get(1); | ||
| DataType fieldType = fieldName.getDataType(); | ||
| if (fieldName.isLiteral() && (fieldType.isIntegerLikeType() || fieldType.isStringLikeType())) { | ||
| // element_at(s, 'field') IS NULL has two-layer null semantics: |
There was a problem hiding this comment.
The two-layer rule is applied only in the IS NULL branch, which makes it internally inconsistent.
If a struct's own null map is not implied by a child path, then these have identical exposure and get no [s,NULL]:
length(element_at(s,'f'))— META OFFSET context, falls through without emitting the parent path- plain
element_at(s,'f')— pruned tostruct<f>, no parent path either
If it is implied — which the long-standing correctness of plain projection suggests — then [s,NULL] is redundant, and it is what triggers the external-table collapse flagged on AccessPathPlanCollector.java.
Both cannot be true. Whichever holds, please state the BE invariant in a comment here: whether reading any descendant of a nullable struct always materialises the parent null map. That single sentence decides whether this block is required or removable.
| } | ||
|
|
||
| @Test | ||
| public void testDataPathNamedLikeMetadataComponent() { |
There was a problem hiding this comment.
This test does not exercise the defect, in two ways.
new StructField("NULL", ...) is lowercased to null by the constructor — the test's own assertEquals("STRUCT<null:TEXT>", ...) below confirms it. A struct field literally named NULL cannot be constructed.
More importantly, it drives setAccessByPath directly, which is the FE side — and FE was never wrong here. FE compares case-sensitively, so "null".equals("NULL") never matched. The defect was BE's StringCaseEqual consuming ["s","null"] as a sentinel. This test passes both before and after the fix.
What is missing is an end-to-end case: create the table, insert a row, select the field, compare results with pruning on and off. Suggested in more detail on null_column_pruning.groovy.
| String fieldName = path.get(index); | ||
| return children.get(AccessPathInfo.ACCESS_MAP_VALUES).replacePathByAnotherTree( | ||
| cast.children.get(fieldName), path, index + 1 | ||
| cast.children.get(fieldName), path, index + 1, pathType |
There was a problem hiding this comment.
Pre-existing, but this change makes it load-bearing.
The Map branch hardcodes children.get(ACCESS_MAP_VALUES) on the origin side while resolving the cast side with fieldName, which can be KEYS. For a path like [m, KEYS, OFFSET] the recursion therefore pairs the origin value subtree against the cast key subtree.
The new terminal check then returns type.equals(cast.type) on that mismatched pair. It fails safe — an unequal result just falls back to reading the original data — but the comparison it performs is not the one intended, and for map<string,string> it returns true by coincidence rather than by correctness. Worth fixing while this code is open.
englefly
changed the title
4 participants
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)