branch-4.2 [fix](nereids) Disambiguate NULL/OFFSET metadata from same-named nested fields (#65805) - #68314
branch-4.2 [fix](nereids) Disambiguate NULL/OFFSET metadata from same-named nested fields (#65805)#68314englefly wants to merge 4 commits into
Conversation
… meta-path handling to properly eliminate redundant NULL/OFFSET access paths and prevent unsafe BE reader mode combinations. (apache#64535) 1. Map-star path normalization (normalizeMapValueMetaOnlyAccessPaths) - Rewrites [m, *, OFFSET] -> [m, KEYS] + [m, VALUES, OFFSET], and [m, *, NULL] -> [m, KEYS] + [m, VALUES, NULL] - Unified OFFSET and NULL normalization via collectMapValueMetaOnlyAccessPaths(String metaSuffix) 2. Meta path stripping (MetaPathStriper) - Two-level approach: - Level 1 (same-prefix): [prefix] > [prefix, OFFSET] > [prefix, NULL] - Level 2 (deeper-prefix): deeper path covers shallower meta path, merged into single stripMetaPathsByDeeperPrefix(metaSuffix) function - Type-aware */VALUES/KEYS equivalence for map columns via compareMetaPathPrefixCoverage - Depth guard prevents same-depth cross-type from being removed in Level 2 - Supplemental KEYS path handling when removing OFFSET paths 3. MV fragment meta path handling - skipMetaPath flag in AccessPathExpressionCollector -- MV fragments skip meta path collection at the source, falling through to default visitor for data-only access branch-4.2 adaptations: - master-only FE access-path model types are mapped onto this branch's thrift types (ColumnAccessPath -> TColumnAccessPath, ColumnAccessPathType -> TAccessPathType, AccessPathInfo path components read via getAccessPathList). - Kept branch-4.2's segment-wise path comparator (comparePathSegments) and the variant terminal branch in DataTypeAccessTree.setAccessByPath. - Kept branch-4.2's struct_element(...) spelling in the ported unit tests. Co-authored-by: Claude <noreply@anthropic.com>
…columns (apache#59263) The subcolumns of a pruned complex-type column can fall into two categories: 1. Predicate columns -- columns required to evaluate filter predicates, which need to be read upfront. 2. Non-predicate columns -- columns that are not needed when evaluating filter predicates. For non-predicate columns, Doris can defer reading them until after predicate evaluation, reducing unnecessary I/O for nested columns. This update also preserves predicate metadata paths separately from final lazy-materialization data paths. Predicate evaluation may still need current-level metadata, such as OFFSET for cardinality()/length() and NULL for IS NULL, even when a covering data path is also needed later. The BE nested column iterators consume those current-level metadata paths at the correct iterator level without forwarding them to child iterators or incorrectly switching mixed data reads into metadata-only mode. This PR also handles predicate-only nested access paths. A complex iterator should skip access-path setup only when both final access paths and predicate access paths are empty. Otherwise predicate-only child or metadata paths may be ignored in lazy read mode, causing predicates such as array element IS NULL to evaluate on unread placeholder data. branch-4.2 adaptations: - gensrc/thrift/PaloInternalService.thrift: master uses id 226 for enable_prune_nested_column, but branch-4.2 already uses 226-228, so the option was moved to the next free id (229). - segment_iterator.cpp keeps branch-4.2's _non_predicate_column_ids naming and drops the master-only _update_lsn_col_if_needed/_update_tso_col_if_needed calls from the ported hunk; branch-4.2 keeps its own common/compile_check_begin.h include in front of the new ScopedColumnIteratorReadPhase helper. - column_reader.cpp keeps branch-4.2's dst->is_nullable()/zone_map.pass_all idioms and its AccessPathInfo/zone-map handling where the ported hunks only differed by master-side refactors. - regression suites lambda_null_pruning and map_contains_arg_pruning do not exist on branch-4.2 and were not added. Co-authored-by: Jerry Hu <hushenggang@selectdb.com> Co-authored-by: Claude <noreply@anthropic.com>
…-named nested fields (apache#65805) Problem Summary: Nested-column pruning makes Doris read only the referenced parts of a struct/map/array column. Two kinds of special meta access paths serve that purpose: a terminal NULL component means the query only needs the null flag (IS [NOT] NULL), and a terminal OFFSET component means only offsets are needed (length/cardinality-style functions). The BE satisfies them in NULL_MAP_ONLY / OFFSET_ONLY meta-read modes that skip the payload data entirely. While the optimization itself is sound, the way meta paths were expressed on the wire and handled on both sides had several correctness holes: * Wire ambiguity. Every access path was sent as a DATA-typed list of string components, and NULL/OFFSET lived in the same list as real child names, so a struct field literally named OFFSET or NULL could be swallowed as metadata and its data silently pruned away, and there was no FE/BE version contract to evolve the encoding during rolling upgrades. * FE null-map correctness. The old logic keyed on expression/slot nullability, so a physical NOT NULL dimension column of a LEFT JOIN received a [col, NULL] path and the BE aborted trying to read a null map from a NOT NULL column. * FE pruning bookkeeping. NULL/OFFSET were detected by string suffix, so real fields with those names collided inside the pruning logic too. * BE routing/read-mode logic. A terminal NULL/OFFSET was treated as current-level metadata at every container, meta-only modes could be entered even when a sibling DATA path still required payload reads, and Map descendant routing depended on physical key/value child column names while silently ignoring unknown selectors. What this PR does: * Versioned, type-selected access paths: adds an optional version to TColumnAccessPath (thrift + protobuf + FE/BE descriptor conversion; TCOLUMN_ACCESS_PATH_VERSION_LEGACY = 0, TCOLUMN_ACCESS_PATH_VERSION_TYPED = 1). In the typed format the path type is authoritative. New BEs still decode the legacy all-DATA encoding from old FEs, so the supported rolling upgrade is: upgrade all BEs first, then let FEs send typed paths. * FE: marks NULL/OFFSET collector contexts as META; checks the physical column's nullability instead of slot nullability; falls back to plain data reads for variant sub-columns; keeps the exact meta path when a sibling data path is also read; and keys pruning on the path type instead of string suffixes. * BE: validates path versions and type-selected payloads; partitions current data / current metadata / descendant routing with explicit per-container ownership; derives NULL_MAP_ONLY / OFFSET_ONLY only when no current or predicate data path requires payload; keeps sibling META paths explicit; routes Map children by the logical KEYS/VALUES selectors; rejects unrecognized Map selectors instead of silently pruning everything. * Tests: BE ColumnReaderTest coverage for typed-vs-legacy encoding, metadata-vs-same-named-data disambiguation, meta-read-mode legality, sibling-data-path handling and Map selector validation; SlotDescriptor protobuf round-trip of versions and payloads; FE PruneNestedColumnTest cases; the nereids_rules_p0/column_pruning regression suites, plus the new left_join_not_null_column suite guarding the outer-join null-map crash. branch-4.2 adaptations: - branch-4.2 keeps the FE access-path model as thrift types instead of master's ColumnAccessPath/DescriptorToThriftConverter, so the typed version is stamped in NestedColumnPruning.buildColumnAccessPaths (the branch's single producer) and preserved by SlotTypeReplacer.replaceIcebergAccessPathToId. The master-only DescriptorToThriftConverter/DescriptorToThriftConverterTest files do not exist on this branch and were not added; the equivalent coverage lives in PruneNestedColumnTest, whose path()/metaPath() helpers now carry the typed version. - Ported FE code was mapped onto this branch's names (TAccessPathType, TColumnAccessPath, isFunctionNullCheckPath) and keeps the segment-wise access path comparator introduced by the branch's nested-column-pruning port. - Unit tests for lambda access paths and Map lazy-read-by-rowids that only exist on master (they come from PRs outside this chain) were not added, and the regression suites lambda_null_pruning / map_contains_arg_pruning do not exist on branch-4.2. Co-authored-by: Hu Shenggang <hushenggang@selectdb.com>
|
run buildall |
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
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.
Request changes: the exact reviewed head has four deterministic test/invariant failures. The sparse NULL-map rowid path leaves a nullable column with mismatched nested/null-map sizes; the new lazy-helper tests index an uninitialized block-position map; the outer-join test expects a DATA path where the collector emits the nullable field's META path; and the retained map SKIP test still assumes resize semantics after the implementation switched to appending placeholders.
I reviewed all 34 authoritative changed paths and the relevant FE/BE call chains, including typed DATA/META routing, thrift/protobuf and upgrade compatibility, map KEYS/VALUES expansion, two-phase lazy materialization, sparse/zero-row reads, Top-N type gating, profiles, and regression cleanup. No additional user focus was supplied.
Validation was static only: the review prompt prohibited builds and tests, so author/CI validation claims were not independently executed here.
| ASSERT_EQ(2, null_map.size()); | ||
| EXPECT_EQ(0, null_map[0]); | ||
| EXPECT_EQ(1, null_map[1]); | ||
| EXPECT_EQ(2, nullable_col.get_nested_column().size()); |
There was a problem hiding this comment.
[P1] Avoid appending the NULL-map defaults twice
This new test calls the NULL_MAP_ONLY rowid branch, which inserts count defaults into the nested column before the loop and then inserts total_read_count defaults again before returning. For these two rowids the null map has size 2 but the nested column has size 4, so this expectation fails (and the ColumnNullable is internally inconsistent). Please keep only one of those insertions, preferably after the actual read count is known.
There was a problem hiding this comment.
Fixed in the BE source rather than in the test. FileColumnIterator::read_by_rowids() filled count placeholders into the nested column before the rowid loop and then appended total_read_count more at the end, so for {0, 2} the null map had 2 entries while the nested column had 4 — exactly the inconsistency you describe. The pre-loop insertion is removed so the nested column is appended once, after the real read count is known; this also matches master's implementation of #65805. ColumnReaderTest.NullMapOnlyReadBySparseRowidsAcrossPages is unchanged and now passes (BUILD_TYPE_UT=Release ./run-be-ut.sh --run --filter=ColumnReaderTest.NullMapOnlyReadBySparseRowidsAcrossPages → OK).
| auto iter = std::make_unique<SegmentIterator>(nullptr, _read_schema); | ||
| iter->_opts.tablet_schema = _tablet_schema; | ||
| iter->_opts.stats = &_stats; | ||
| iter->_support_lazy_read_pruned_columns.insert(0); |
There was a problem hiding this comment.
[P1] Initialize the block-position map in this fixture
Both tests add cid 0 to _support_lazy_read_pruned_columns and call _read_lazy_pruned_columns() directly, but this fixture never initializes _schema_block_id_map. The helper immediately indexes _schema_block_id_map[0], while production normally populates it in _vec_init_lazy_materialization(), so these tests hit undefined behavior before checking rowids or phase restoration. Please initialize the map consistently with the one-column block (or drive the production initializer first).
There was a problem hiding this comment.
Fixed: the fixture now builds _schema_block_id_map the same way production does in _vec_init_lazy_materialization() — resize to the number of columns in the read schema, then map every read column id to its block position — before the tests call _read_lazy_pruned_columns() directly. Both tests in the suite pass now (--filter=SegmentIteratorLazyPrunedTest.* → 2 tests, 0 failures).
| + " on driving_tbl.id = not_null_struct_tbl.id" | ||
| + " where element_at(not_null_struct_tbl.s, 'f') is null", | ||
| // expect-contain: field is read (DATA path) | ||
| ImmutableList.of(path("s", "f")), |
There was a problem hiding this comment.
[P1] Expect the nullable field's META path here
Although the enclosing s column is NOT NULL, SQL struct fields are constructed as nullable. For element_at(s, 'f') IS NULL, the collector therefore retains the META context and emits [s,f,NULL]; the physical-root guard only recognizes the direct one-component [NULL] path and does not turn this into DATA. Because this helper checks exact path equality, the expected [s,f] DATA path is absent and the new test fails. Please expect metaPath("s", "f", "NULL") here while keeping the negative assertion for root [s,NULL].
There was a problem hiding this comment.
I checked this against the real plan data before changing anything, and the expectation is correct as written: with path("s", "f") the test passes (./run-fe-ut.sh --run org.apache.doris.nereids.rules.rewrite.PruneNestedColumnTest → 64 tests, 0 failures). When I first applied the suggested metaPath("s", "f", "NULL") the assertion printed the complete collected set:
allAccessPaths=[TColumnAccessPath(type:DATA, data_access_path:TDataAccessPath(path:[s, f]), version:1)]
So no field-level META path is produced for this query — the field ends up materialized as data because the IS NULL is evaluated above the LEFT JOIN rather than as a scan predicate. (The field-level META form does show up in testNullableFieldPreservedWithSiblingProjection, where the null check sits directly on the scan of a nullable struct column.)
The underlying point is fair though: this SQL never produces a root [s, NULL] candidate, so the negative assertion was not really exercising the suppression. I added testNotNullStructIsNullOnOuterJoinNullableSide — select not_null_struct_tbl.s is null from driving_tbl left join not_null_struct_tbl ... — a direct null check on the physically NOT NULL struct, which is the shape that reaches the hasPhysicalNullMap guard in visitSlotReference. It asserts that the struct slot is still present in the plan and that no [s, NULL] META path is emitted. It is kept in the SELECT list on purpose: in a WHERE clause the null-rejecting predicate lets the optimizer rewrite the LEFT JOIN into an anti join, which drops the struct from the plan and would make the assertion vacuous again.
| @@ -285,7 +3039,7 @@ TEST_F(ColumnReaderTest, MapReadByRowidsSkipReadingResizesDestination) { | |||
| MapFileColumnIterator map_iter(map_reader, std::move(null_iter), std::move(offsets_iter), | |||
| std::move(key_iter), std::move(val_iter)); | |||
| map_iter.set_column_name("map_col"); | |||
| map_iter.set_reading_flag(ColumnIterator::ReadingFlag::SKIP_READING); | |||
| map_iter.set_read_requirement(ColumnIterator::ReadRequirement::SKIP); | |||
There was a problem hiding this comment.
[P1] Update this fixture for the new append semantics
This test's offsets column already contains one entry, so dst->size() is 1 before read_by_rowids(). With ReadRequirement::SKIP, the new placeholder helper calls insert_many_defaults(3), appending three map rows and leaving size 4; the unchanged assertion still expects the old resize(count) result of 3. Please start with an actually empty destination (or assert initial size plus count) if append semantics are intended, otherwise preserve explicit total-size resize behavior.
There was a problem hiding this comment.
Fixed: the destination now starts genuinely empty (the extra base offset entry is gone), and the test is renamed to MapReadByRowidsSkipReadingAppendsPlaceholders, since append — not an absolute resize — is the intended semantics. It asserts count after the first read_by_rowids() and 2 * count after a second one, so a regression back to resize(count) would be caught.
FE UT Coverage ReportIncrement line coverage |
…aths (apache#68314) Addresses the four review comments on apache#68314: * column_reader.cpp: FileColumnIterator::read_by_rowids() inserted `count` placeholders into the nested column before the rowid loop and appended `total_read_count` more at the end, so the nested column ended up longer than the null map (an internally inconsistent ColumnNullable). Keep only the insertion after the real read count is known, matching master. * segment_iterator_lazy_pruned_test.cpp: build `_schema_block_id_map` in the fixture the same way `_vec_init_lazy_materialization()` does, so the direct `_read_lazy_pruned_columns()` calls no longer index an empty vector. * column_reader_test.cpp: the skipped-read destination must start truly empty; assert the append semantics explicitly with a second read as well. * PruneNestedColumnTest: keep the verified `[s, f]` DATA expectation of testNotNullStructOnOuterJoinNullableSide (the collected set for that query is exactly that one path) and add testNotNullStructIsNullOnOuterJoinNullableSide, which is the shape that actually puts the root `[s, NULL]` path in front of the hasPhysicalNullMap guard, so the suppression the PR introduces is really covered. Test: * FE: ./run-fe-ut.sh --run org.apache.doris.nereids.rules.rewrite.PruneNestedColumnTest -> 64 tests, 0 failures; mvn checkstyle:check -> 0 violations. * BE: BUILD_TYPE_UT=Release ./run-be-ut.sh --run --filter='ColumnReaderTest.NullMapOnlyReadBySparseRowidsAcrossPages:ColumnReaderTest.MapReadByRowidsSkipReadingAppendsPlaceholders:SegmentIteratorLazyPrunedTest.*' -> 4 tests, 0 failures.
|
Pushed
Verification on the pushed commit:
|
3 participants
What problem does this PR solve?
Cherry-pick of #65805 (
[fix](nereids) Disambiguate NULL/OFFSET metadata from same-named nested fields) tobranch-4.2.The port also carries the two prerequisite commits that #65805 is written on top of, because branch-4.2 stopped at the state just before them:
branch-4.2 [fix](NestedColumnPruning) Rewrite the NestedColumnPruning meta-path handling ...branch-4.2 [feat](olap) Support lazy reading mode for pruned complex columnsMetaPathStriperapproach again (predicate metadata paths are kept,normalizePredicateMetaPathForAllAccessPath/addPredicatePathsToFinalAllAccessPaths/expandMapStarPaths), and that is the code #65805 patches.branch-4.2 [fix](nereids) Disambiguate NULL/OFFSET metadata from same-named nested fieldsMaster order for reference:
#61888 → #62205 → #62315 → #62631 → #62304 → #63229 → #63736 → #64486 → #64535 → #59263 → … → #65805; branch-4.2 already had everything up to #64486 (ported by #68214).What #65805 fixes
TColumnAccessPathgets an optionalversion(TCOLUMN_ACCESS_PATH_VERSION_LEGACY = 0,TCOLUMN_ACCESS_PATH_VERSION_TYPED = 1) that is carried through thrift, protobuf and both FE/BE descriptor conversions. In the typed format the path type is authoritative:DATAselectsdata_access_path,METAselectsmeta_access_path, andNULL/OFFSETare only ever emitted as typedMETApaths. New BEs still decode the legacy all-DATAencoding, so the supported rolling upgrade is: upgrade all BEs first, then let FEs send typed paths.NULL/OFFSETcollector contexts asMETA; checks the physical column's nullability instead of slot nullability, so an outer-join-nullableNOT NULLcolumn no longer gets a[col, NULL]path; falls back to plain data reads for variant sub-columns; keeps the exact meta path when a sibling data path is also read; and keys pruning on the path type instead of string suffixes.NULL_MAP_ONLY/OFFSET_ONLYonly when no current or predicate data path requires payload; keeps siblingMETApaths explicit; routes Map children by the logicalKEYS/VALUESselectors instead of physical child column names; and rejects unrecognized Map selectors instead of silently pruning everything.branch-4.2 adaptations
ColumnAccessPath/DescriptorToThriftConverter, so the typed version is stamped inNestedColumnPruning.buildColumnAccessPaths(the branch's single producer) and preserved bySlotTypeReplacer.replaceIcebergAccessPathToId.DescriptorToThriftConverter/DescriptorToThriftConverterTestfiles do not exist on this branch and were not added; the equivalent coverage lives inPruneNestedColumnTest, whosepath()/metaPath()helpers (andVariantPruningLogicTest's) now carry the typed version.TAccessPathType,TColumnAccessPath,isFunctionNullCheckPath) and keeps the segment-wise access-path comparator introduced by the branch's nested-column-pruning port.gensrc/thrift/PaloInternalService.thrift: master uses field id226forenable_prune_nested_column, but branch-4.2 already uses226-228, so the option was moved to the next free id (229).segment_iterator.cppkeeps branch-4.2's_non_predicate_column_idsnaming and drops the master-only_update_lsn_col_if_needed/_update_tso_col_if_neededcalls from the ported hunk; branch-4.2 keeps its owncommon/compile_check_begin.hinclude in front of the newScopedColumnIteratorReadPhasehelper._read_lazy_pruned_columnsuses the branch's_schema_block_id_mapbecause master'sSchema::column_index()does not exist here, andFileColumnIterator::get_reader()was added as in master.lambda_null_pruningandmap_contains_arg_pruning, and the unit tests for lambda access paths / Map lazy-read-by-rowids, only exist on master (they come from PRs outside this chain) and were not added.Test
./build.sh --feOK../run-fe-ut.sh --run 'org.apache.doris.nereids.rules.rewrite.PruneNestedColumnTest'→ 63 passed;VariantPruningLogicTest14,IcebergScanNodeTest95,MaterializeProbeVisitorTest8,StringEmptyToLengthRuleTest,PullUpProjectExprUnderTopNTest— all green../build.sh --beOK (the two ported BE test sources were additionally checked with the unit-test flags,-DBE_TEST -fno-access-control)../run-regression-test.sh --run -d nereids_rules_p0/column_pruning→ 8 suites, 0 failed, including the newleft_join_not_null_columnsuite that guards the outer-join null-map crash.Behavior changed
Yes. Typed
METAaccess paths with an explicit version replace the legacy all-DATAencoding between new FEs and BEs (upgrade BEs before FEs); struct fields literally namedNULL/OFFSETare now read correctly instead of being pruned as metadata;IS NULLon an outer-join-nullableNOT NULLcolumn no longer crashes the BE; invalid Map descendant selectors now return an internal error instead of being silently ignored. The port also brings #59263, which defers reading non-predicate pruned complex columns until after predicate evaluation, reducing nested-column I/O.Release note
None