Repository navigation
[fix](lazy topn) Fix slot-not-found after PullUpProjectExprUnderTopN with chained expressions - #64486
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29320 ms |
Contributor
TPC-DS: Total hot run time: 169457 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
englefly
marked this pull request as ready for review
June 14, 2026 12:30
englefly
requested review from
924060929,
morrySnow and
starocean999
as code owners
June 14, 2026 12:30
Contributor
TPC-H: Total hot run time: 29047 ms |
Contributor
TPC-DS: Total hot run time: 169006 ms |
Contributor
Author
|
run cloud_p0 |
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.
I found one blocking correctness issue in the new bottom-up rewrite. The rule can now move aliases that canPullUp explicitly rejects when those aliases depend on a lower alias that was pulled up.
Critical checkpoint conclusions:
- Goal/test: The PR aims to fix slot-not-found/chained-expression behavior for
PullUpProjectExprUnderTopN; the added unit tests cover several nested TopN and rename cases, but miss the chained non-movable/volatile case below. - Scope: The change is focused to the Nereids rewrite and its unit test, but the replacement-map broadening changes semantic eligibility beyond the stated fix.
- Concurrency/lifecycle/config/compatibility/persistence: Not applicable; this is a logical optimizer rewrite with no new config, storage, protocol, or transaction persistence changes.
- Parallel paths: The affected path is the custom TopN expression pull-up rule used before lazy materialization; the lower lazy-materialization post processor still relies on this rule preserving expression movability constraints.
- Tests: I attempted
./run-fe-ut.sh --run org.apache.doris.nereids.rules.rewrite.PullUpProjectExprUnderTopNTest, but it failed before test execution becausethirdparty/installed/bin/protocis missing in this runner. - Observability/performance: No additional observability needed; performance intent is reasonable once the semantic guard is restored.
User focus: No additional user-provided review focus was present.
Contributor
FE Regression Coverage ReportIncrement line coverage |
1 similar comment
Contributor
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary: PullUpProjectExprUnderTopN records alias replacement chains so upper TopN nodes can continue pulling expressions restored by lower TopN nodes. Non-movable aliases such as assert_true(x > 0), volatile expressions, Score, or L2DistanceApproximate could also be recorded in that replacement map. If a lower pull-up removed their input slot, the unavailable alias could be synthesized above TopN through the replacement chain, bypassing canPullUp and changing error or evaluation semantics. This change makes non-forwarding aliases that cannot be pulled up block their input slots, while keeping simple forwarding aliases available for chain resolution. It also adds a unit test for assert_true(x > 0) depending on x = a + 1 under TopN.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- ./run-fe-ut.sh --run org.apache.doris.nereids.rules.rewrite.PullUpProjectExprUnderTopNTest
- Behavior changed: No
- Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28839 ms |
Contributor
TPC-DS: Total hot run time: 168431 ms |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review summary
No additional inline findings from this pass. I reviewed the two actual PR files reported by GitHub: PullUpProjectExprUnderTopN.java and PullUpProjectExprUnderTopNTest.java.
Critical checkpoint conclusions:
- Goal and proof: The change aims to keep dependencies of non-movable aliases below TopN.
shouldBlockProjectInputsnow propagates blocked input ExprIds for rejected non-forwarding aliases, and the new unit test covers theassert_true(x > 0)overx = a + 1case. - Scope: The implementation is small and localized to the TopN pull-up rule plus one focused regression test.
- Concurrency, lifecycle, config, compatibility: Not applicable; this is a logical optimizer rewrite/test change with no shared runtime state, new config, protocol, or storage format changes.
- Parallel paths: The new predicate uses
canPullUp, so the same blocking behavior applies to the existing rejection categories such asNoneMovableFunction, volatile expressions,Score, andL2DistanceApproximate, while forwarding aliases and literals remain available for chain resolution. - Tests: The added test covers the reported dependency case. I also ran
git diff --checksuccessfully. I attempted./run-fe-ut.sh --run org.apache.doris.nereids.rules.rewrite.PullUpProjectExprUnderTopNTest, but it failed before Java test execution becausethirdparty/installed/bin/protocis missing in this runner. - Observability, transactions, data writes, memory tracking: Not applicable to this optimizer-only change.
- User focus: No additional user-provided review focus.
I read the existing inline thread about replacement-map handling for non-movable aliases and treated it as already-known context, so I did not duplicate that thread. I did not find a distinct additional issue.
Contributor
FE Regression Coverage ReportIncrement line coverage |
englefly
changed the title
starocean999
approved these changes
Jun 15, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
englefly
added a commit
to englefly/incubator-doris
that referenced
this pull request
Sep 20, 2026
…rUnderTopN with chained expressions (apache#64486) ### What problem does this PR solve? this pr refactor PullUpProjectExprUnderTopN to avoid slot-not-found error. in this version, pullup is done in bottom up algorithm. it makes pullup simpler than previous version Issue Number: close #xxx Related PR: apache#63736 Problem Summary: ### Release note None ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into -->
yiguolei
pushed a commit
that referenced
this pull request
Sep 21, 2026
… path series (#68214) Cherry-pick of #62205, #62315, #62631, #62304, #63229, #63736, #64486 to branch-4.2 (the meta path series, applied in merge order). Adaptations for branch-4.2: * `AccessPathInfo` already lives in `org.apache.doris.analysis` on this branch; the new `ACCESS_STRING_OFFSET` constant was ported there (aliasing the existing `ACCESS_OFFSET`). * master-only types renamed to this branch's thrift types (`ColumnAccessPath* -> TColumnAccessPath`, `ColumnAccessPathType -> TAccessPathType`); `NestedColumnPruning` keeps branch-4.2's `comparePathSegments`/variant branch alongside upstream's new logic. * `LazyMaterializeTopN` merges upstream's restructure while preserving branch-4.2's Lance lazy-materialization gate; `LogicalJoin.getRightConditionSlot()` (a master-only helper) had to be added for the ported `PushDownTopNThroughJoin`. * `StringEmptyToLengthRuleTest` uses `ElementAt` (branch-4.2 folded `StructElement` into `ElementAt`); the regression suite expectation for the same reason. REQUIRED EXTRA COMMIT: the FE now emits offset-only / null-only nested access paths, which branch-4.2's BE could not read — `null_column_pruning.groovy` aborted the BE with `std::vector<...ColumnIterator>::operator[]: Assertion '__n < this->size()' failed`. This PR therefore also cherry-picks the BE-side prerequisite #61888 "[Exec](be) Support offset prue column and null column in BE" (adapted to branch-4.2's `IColumn::mutate`/Defer COW shape and `ColumnNullable::get_null_map_column_ptr()` signature). Please review that extra commit carefully. Testing on branch-4.2 (FE + BE rebuilt from this branch): * FE unit tests: PruneNestedColumnTest (57), PullUpProjectExprUnderTopNTest (28), OperativeColumnDeriveTest (4), StringEmptyToLengthRuleTest (11), MaterializeProbeVisitorTest (8) — 108 tests, 0 failures. * Regression: nereids_rules_p0/column_pruning (7 suites incl. the new string_length/null/nested_container_offset/topn_expr_pullup/topn_lazy_nested suites) 0 failures; shape_check clickbench/tpcds_sf100/tpcds_sf1000/tpcds_sf10t_orc/tpcds_sf1000_constraints suites touched by the series regenerated with -forceGenOut and green; nereids_rules_p0 push_filter_through / null_un_safe_equals / lazy_materialize_topn green. * Known deviations: #62446 (struct field slot type for OFFSET-only access) is not part of the requested series and was not included; the BE-side `MapAccessAllWithOffsetDoesNotPropagateOffsetToKey` unit test from #61888 was kept byte-identical to upstream although it may not hold (test binary not built with MAKE_TEST=ON). --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: HappenLee <happenlee@selectdb.com>
yiguolei
pushed a commit
that referenced
this pull request
Oct 8, 2026
…-named nested fields (#65805) (#68314) ### What problem does this PR solve? Cherry-pick of #65805 (`[fix](nereids) Disambiguate NULL/OFFSET metadata from same-named nested fields`) to `branch-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: | commit | PR | why it is needed | | --- | --- | --- | | `branch-4.2 [fix](NestedColumnPruning) Rewrite the NestedColumnPruning meta-path handling ...` | #64535 | branch-4.2's FE pruning code was exactly master-before-#64535; #65805's FE hunks are written against the rewritten meta-path handling. | | `branch-4.2 [feat](olap) Support lazy reading mode for pruned complex columns` | #59263 | on master #59263 replaced #64535's `MetaPathStriper` approach 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 fields` | #65805 | the requested pick. | Master 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 * **Versioned, type-selected access paths.** `TColumnAccessPath` gets an optional `version` (`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: `DATA` selects `data_access_path`, `META` selects `meta_access_path`, and `NULL`/`OFFSET` are only ever emitted as typed `META` paths. New BEs still decode the legacy all-`DATA` encoding, 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, so an outer-join-nullable `NOT NULL` column 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. * **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 instead of physical child column names; and rejects unrecognized Map selectors instead of silently pruning everything. ### 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 (and `VariantPruningLogicTest`'s) 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. * `gensrc/thrift/PaloInternalService.thrift`: master uses field 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. `_read_lazy_pruned_columns` uses the branch's `_schema_block_id_map` because master's `Schema::column_index()` does not exist here, and `FileColumnIterator::get_reader()` was added as in master. * Regression suites `lambda_null_pruning` and `map_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 * FE: `./build.sh --fe` OK. `./run-fe-ut.sh --run 'org.apache.doris.nereids.rules.rewrite.PruneNestedColumnTest'` → 63 passed; `VariantPruningLogicTest` 14, `IcebergScanNodeTest` 95, `MaterializeProbeVisitorTest` 8, `StringEmptyToLengthRuleTest`, `PullUpProjectExprUnderTopNTest` — all green. * BE: `./build.sh --be` OK (the two ported BE test sources were additionally checked with the unit-test flags, `-DBE_TEST -fno-access-control`). * Regression: `./run-regression-test.sh --run -d nereids_rules_p0/column_pruning` → 8 suites, 0 failed, including the new `left_join_not_null_column` suite that guards the outer-join null-map crash. ### Behavior changed Yes. Typed `META` access paths with an explicit version replace the legacy all-`DATA` encoding between new FEs and BEs (upgrade BEs before FEs); struct fields literally named `NULL`/`OFFSET` are now read correctly instead of being pruned as metadata; `IS NULL` on an outer-join-nullable `NOT NULL` column 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 ### Rebased onto branch-4.2 (2026-09-23) The branch was rebased onto the current `branch-4.2` (`c0cbcc3e49c`) and is now a single commit `e044385682f`. The two prerequisite ports listed above are folded into it: branch-4.2 has since gained its own nested-column-pruning series (`#68214`) and the `#68331` predicate-path fix, and `#59263` had already replaced the `#64535` `MetaPathStriper` design inside this pick, so replaying the intermediate states is no longer meaningful. See the comment below for the conflict-resolution details and the verification results.
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
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: #63736
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)