Repository navigation
[fix](nereids) Fix deep copy of scans with virtual columns - #69399
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#69292 Problem Summary: A CTE fails to plan when the optimizer inlines it after one of its scans got a virtual column: WITH c AS ( SELECT id, score() AS s FROM t WHERE c1 MATCH 'apple' ORDER BY s DESC LIMIT 2 ) SELECT * FROM c; -- A expression contains slot not from children A score or ANN TopN pushed into a scan, and a MATCH in the select list, add a virtual column to the scan. After rewrite, the optimizer inlines a CTE that has one consumer left (Optimizer.normalizeCtePlan) by deep copying its rewritten body, so the copy includes these scans: - score() and l2_distance_approximate() are nondeterministic, so the rewrite does not inline a CTE that calls them, even with one consumer. Once the TopN pushdown has replaced them with a virtual column of the scan, the optimizer inlines the CTE. - A CTE with MATCH in its select list reaches the optimizer when the rewrite removes all but one of its consumers, e.g. a UNION ALL branch with WHERE 1 = 0. LogicalPlanDeepCopier copied a scan with virtual columns wrongly: - it gave each virtual column two new ids: the slots above the scan were mapped to the first one, and the copied scan output the second one; - the copied scan still output the original virtual column, with its original id, next to the new one; - the score and ANN order keys of the copied scan still used the original slots. So the plan above the copied scan read a slot that the scan does not output. The copier now gives each virtual column one new id, maps the original id to it and maps the score and ANN order keys to the new slots. Relations without virtual columns are copied as before. LogicalOlapScan.withVirtualColumns replaces the virtual columns of the scan, but kept their old slots in the output next to the new ones. It now replaces them there too and keeps the other output slots, which the copier has already mapped. Computing the output of the copy again would not work: it gives VARIANT subpath slots new ids. AdjustNullable, the other caller that replaces existing virtual columns, no longer keeps two slots for a virtual column whose nullability changes. ### Release note Fix "A expression contains slot not from children" for a CTE that orders by score() or by an ANN distance with a LIMIT, or that has MATCH in its select list and is inlined after rewrite. ### Check List (For Author) - Test: Unit Test and Regression test - Unit tests: LogicalPlanDeepCopierTest.testDeepCopyOlapScanWithVirtualColumn and CTEInlineTest.inlineCteWhoseScanHasVirtualColumns (CTEs with a score TopN, an ANN TopN, a MATCH projection, and a VARIANT subpath with a MATCH projection, with one consumer and with a second consumer removed by WHERE 1 = 0). Both fail on master (4 output slots instead of 3; "A expression contains slot not from children") and pass with this change. OrExpansionTest, SplitMultiDistinctTest, DecomposeRepeatWithPreAggregationTest, LogicalOlapScanTest, IvmDeltaRewriteHelperTest and PushDownVirtualColumnsIntoOlapScanTest pass. Checkstyle passes. - Regression test: nereids_rules_p0/cte/test_cte_inline_virtual_column, with its .out generated by the framework, on an FE built from this branch and a BE built from 41a58af plus apache#69292's SegmentIterator change (this change is FE only). The VARIANT case is only in the unit test, because that BE reads v['x'] as NULL even without a CTE. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
…ANT slots in scan copies ### What problem does this PR solve? Issue Number: None Related PR: apache#69292, apache#69399 Problem Summary: Review of 8fc7d7c found two problems: - A minimum score under GROUPING SETS, ROLLUP or CUBE, e.g. SELECT c2, count(*) FROM t WHERE c1 MATCH 'apple' AND score() > 0.00001 GROUP BY GROUPING SETS ((c2), ()), was rejected as an unsupported SELECT: the binder did not walk through the LogicalRepeat that analysis puts below the aggregate. It now does, so the minimum score works as with any other GROUP BY, and score() in the select list of such a SELECT gets the aggregate error. - The plan copier computed the output of a copied scan again after replacing its virtual columns. That gives VARIANT subpath slots new ids, which the copier had already mapped, so a CTE that reads v['x'] and has a MATCH in its select list failed with "A expression contains slot not from children" when the optimizer inlined it. The copier fix is now the one of apache#69399: LogicalOlapScan.withVirtualColumns replaces the slots of the old virtual columns in the output and keeps the other slots, and the copier no longer computes the output again. This PR carries the same change until apache#69399 is merged. ### Release note None ### Check List (For Author) - Test: Unit Test and Regression test - BindScoreToOlapScanTest: GROUPING SETS, ROLLUP and CUBE with a minimum score, and max(score()) with ROLLUP; all 73 tests pass. The copier tests are the ones of apache#69399, whose CTEInlineTest covers the VARIANT case. LogicalPlanDeepCopierTest, SearchCteRewriteTest, PushDownVirtualColumnsIntoOlapScanTest, PhysicalPlanTranslatorTest, PushDownMatchProjectionAsVirtualColumnTest, CTEInlineTest, LogicalOlapScanTest and SearchExpressionTest pass (157 tests with BindScoreToOlapScanTest). Checkstyle passes. - Regression tests: test_score_filter_usage gets a GROUPING SETS case, with its .out regenerated by the framework. The 19 suites that use score() pass on a private cluster. - Behavior changed: Yes. A minimum score is supported with GROUPING SETS, ROLLUP and CUBE. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed all seven changed files at be40091 and traced virtual-column production, early/late/CBO CTE inlining, other deep-copy callers, scan replacement, and physical translation. No newly introduced issue was verified. There are no existing inline findings to carry forward and no additional user focus points.
Critical checkpoint conclusions:
- Goal and evidence: the new mapping/output/order-key handling repairs the stated score TopN, ANN TopN and MATCH projection cases. Added unit and planner tests exercise these cases, late inlining, and VARIANT subpaths; execution was not performed in this review.
- Scope and reuse: changes are focused; both scan classes share output replacement and TopN copying reuses the order-key helper.
- Concurrency and locking: no new threads or shared mutable execution state; copy context remains local. No new lock order or heavy work under locks.
- Lifecycle: existing planner object ownership is preserved; no new resources, cleanup obligations, or static initialization dependency.
- Configuration: no configuration is added or changed.
- Compatibility: no persisted format, public wire contract, or rolling-upgrade behavior changes.
- Thrift/protobuf: no definitions changed.
- Parallel paths: regular/stream scans, no-virtual scans, score/ANN/MATCH/CSE producers, nullability adjustment, operative slots and other copier callers were checked. Stream subtype and read mode are preserved.
- Conditions: the nonempty-virtual guard skips unnecessary reconstruction; replacement removes the current virtual ids while retaining ordinary/VARIANT slot identities.
- Test coverage: direct id/order-key/output invariants and eight planner query shapes are covered; regression cases use ordered output and preserve tables for debugging. Stream/nullability edge cases and dependent CSE aliases lack new dedicated tests.
- Expected results: the three new result sets agree with BM25 arithmetic, distinct ANN distances and MATCH truth values. The author reports framework generation using this FE and a separate BE revision plus a related patch; that patch does not affect these three query shapes. Exact-head BE execution and output generation were not independently reproduced.
- Observability: existing planning diagnostics suffice for this local identity repair; no new distributed operation needs metrics or logging.
- Transactions and persistence: no transaction, EditLog, replay, or failover path changes.
- Data writes and crashes: no production data-write or atomicity behavior changes.
- FE-BE variables: existing virtual-column and TopN metadata is forwarded; no new transmitted variable or send path.
- Performance: output replacement is linear in output plus virtual-column count; no material regression was substantiated.
- Other issues and limits: dependent CSE virtual aliases can still retain a forward dependency on an old id with experimental_enable_virtual_slot_for_cse enabled. The baseline already fails the same case through its intermediate/final alias-id mismatch, so this is recorded as a pre-existing limitation, with no new inline finding attributed to this PR. No unresolved introduced-behavior candidate remains.
Validation was static only, as required: no builds, tests, or source modifications.
Review converged after 2 round(s).
924060929
left a comment
There was a problem hiding this comment.
Reviewed be40091. The independent score/ANN/MATCH remapping and output replacement look correct, but the inline comment describes a remaining dependent-virtual-column case in the copier. Source review only; I did not run builds or tests.
…them ### What problem does this PR solve? Issue Number: None Related PR: apache#69399 Problem Summary: With experimental_enable_virtual_slot_for_cse=true, a virtual column can refer to another one, and PushDownVirtualColumnsIntoOlapScan does not list them in dependency order, e.g. [v2 := v1 * v1, v1 := x + y]. The copier copied the virtual columns in list order. When it copied v2, the id of v1 still mapped to itself, so the copy of v2 referred to the old v1, which the copied scan does not output. The copier now maps every virtual column to its new id first and then copies their expressions, which reuse those ids. ### Release note None ### Check List (For Author) - Test: Unit Test - LogicalPlanDeepCopierTest.testDeepCopyOlapScanWithDependentVirtualColumns copies a scan whose virtual columns refer to each other, in both orders. It fails with the previous commit, where "(v1#10002 * v1#10002) AS v2" refers to the old v1, and passes now. LogicalPlanDeepCopierTest, CTEInlineTest, OrExpansionTest, SplitMultiDistinctTest, DecomposeRepeatWithPreAggregationTest, LogicalOlapScanTest, IvmDeltaRewriteHelperTest and PushDownVirtualColumnsIntoOlapScanTest pass. Checkstyle passes. The regression test was not run again: it creates no virtual columns that refer to each other. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <[email protected]>
…pache#69399 ### What problem does this PR solve? Issue Number: None Related PR: apache#69292, apache#69399 Problem Summary: Review of apache#69399 found that the plan copier still mishandled virtual columns that refer to each other, which CSE creates with experimental_enable_virtual_slot_for_cse=true: copying them in list order kept the old id of a virtual column that comes later in the list. apache#69399 now maps every virtual column to its new id before copying their expressions. This PR carries the identical change until apache#69399 is merged. ### Release note None ### Check List (For Author) - Test: Unit Test - LogicalPlanDeepCopierTest, with the new test for virtual columns that refer to each other, BindScoreToOlapScanTest, SearchCteRewriteTest, PushDownVirtualColumnsIntoOlapScanTest, PhysicalPlanTranslatorTest, PushDownMatchProjectionAsVirtualColumnTest, CTEInlineTest, LogicalOlapScanTest and SearchExpressionTest pass. Checkstyle passes. The regression suites were not run again: they create no virtual columns that refer to each other. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed all seven changed files at 5be9ff5372e41aeb75170c85454cd59ba657ced0. No new actionable findings from this static review.
The previously reported dependent-virtual-column issue in thread 4338098220 is fixed: every virtual ExprId is allocated before any dependent expression is copied, and both alias orders are covered by the new unit test. No existing P0/P1 finding remains to carry forward. The user-focus file supplies no additional concerns.
Traced virtual-column creation through CTE normalization/inlining, relation and expression copying, output replacement, physical implementation, and descriptor/order-key translation. Both complete-review agents and the additional mechanism reviewer returned NO_NEW_VALUABLE_FINDINGS on the unchanged empty finding set in round 1. All initial risks and subsidiary candidates were dismissed with source evidence; no changed path or concrete code question remains uncovered.
Critical-checkpoint conclusions:
- Goal and evidence: two-pass ExprId allocation fixes dependent aliases; replacement removes old virtual outputs while preserving mapped ordinary/VARIANT slots; score/ANN keys use the final mapping. Added copier, full-planner, and regression cases target the reported failures.
- Scope and reuse: changes are focused; ordinary/stream scans share the output helper and TopN copying reuses the expression copier.
- Concurrency and locks: no new thread, shared concurrent state, or locking; replacement maps remain per-copy contexts.
- Lifecycle: no new resource or static-initialization dependency; relation-cache reuse preserves the completed copy within its context.
- Configuration: none added or changed.
- Compatibility: no wire/storage-format change or external function-symbol change.
- Thrift/protobuf: no gensrc definitions changed.
- Parallel paths: ordinary and stream builders preserve their state; schema frontend-conjunct copying remains outside the virtual-column guard. Score, ANN, MATCH, CSE, and nullable replacement callers were checked.
- Conditional checks: nonempty-virtual guarding is supported by score/ANN producers that install aliases with their keys; CSE replacement requires an empty existing virtual list. Dependency preallocation is documented.
- Test coverage: JUnit 5 tests check fresh IDs, output cardinality, removed old slots, score-key alignment, dependent aliases in both orders, and score/ANN/MATCH/VARIANT CTE planning with single and eliminated consumers. Regression cases cover score, ANN, and MATCH with deterministic output ordering.
- Expected results: every added output row was inspected; MATCH booleans, ANN nearest IDs, and BM25 score ordering/values are consistent with the fixtures and scoring formula. Tables are dropped before use, names are hardcoded, and the output is marked as generated.
- Observability and errors: existing planner exceptions/plan diagnostics remain adequate for this local fix; no swallowed error or new distributed boundary was introduced.
- Transactions and persistence: no EditLog, replay, visible-version, committed-data visibility, or delete-bitmap behavior changes.
- Data writes and atomicity: no write path, transaction boundary, or crash-cleanup change.
- FE/BE propagation: no new transmitted fields; existing physical implementation and translation receive the copied virtual expressions and remapped keys.
- Performance and memory: the added ID/output/key passes are linear in their respective lists; replacement prevents virtual-output accumulation. No material performance or native-memory ownership issue found.
- Additional recheck: operative-slot positions, repeated relation reuse, cached-output recomputation, stream read modes, mixed VARIANT output, limits, range payloads, and downstream descriptor availability yielded no distinct new defect.
Validation limits: static inspection only, as required; no builds, tests, or runtime queries were run. Runtime correctness and .out generation provenance were not independently reproduced. Stream modes, combined metadata/range/zero-limit cases, and repeated-context identity were traced statically rather than covered by new direct assertions. These execution limits do not leave an unresolved code candidate in the reviewed scope.
Review converged after 1 round(s).
TPC-H: Total hot run time: 28274 ms |
TPC-DS: Total hot run time: 150056 ms |
ClickBench: Total hot run time: 24.33 s |
FE Regression Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 28345 ms |
TPC-DS: Total hot run time: 151279 ms |
ClickBench: Total hot run time: 24.36 s |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
3 similar comments
FE Regression Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
924060929
left a comment
There was a problem hiding this comment.
Re-reviewed 5be9ff5, including the full PR diff and the changes since be40091. The previous dependent-virtual-column finding is fixed: all virtual IDs are allocated before copying expressions, and ExpressionDeepCopier reuses those IDs. The added test covers both dependency orders and checks copied input slots against scan output. The output replacement, VARIANT subpath preservation, stream-scan overrides, and score/ANN order-key remapping look consistent. No further actionable issue found. Source review only; I did not independently run builds or tests.
### What problem does this PR solve? Issue Number: None Related PR: apache#69292, apache#69399 Problem Summary: apache#69399, the plan copier fix that this PR carried until it was merged, is now on master. Merge master so that the copier change leaves the diff of this PR. ### Release note None ### Check List (For Author) - Test: Unit Test (the FE tests around the copier and the binder are run after the merge) - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
PR approved by anyone and no changes requested. |
|
PR approved by at least one committer and no changes requested. |
… in scan expressions ### What problem does this PR solve? Issue Number: None Related PR: apache#69399, apache#69419 Problem Summary: The score() support of this PR special-cased generic code: TopN filter pushdown skipped scans with a score, the MV rewrite check skipped them, and the rewrite-time binder recognized the delete sign filter of UNIQUE tables and rejected table streams by class. All of it came from one BE limitation and from binding score() after analysis. BE: the olap scan prepares the score of every row of its row bitmap before the first batch, but SegmentIterator marked only the stored columns of a pushed-down expression as common-expression columns. An expression on the score alone, e.g. score() > 0.5, set no _is_need_expr_eval, so it was never evaluated, while the scanner had already removed it from its own conjuncts. An expression that read the score next to other columns, e.g. a TopN filter on CAST(score() AS INT), evaluated Score() itself, which returns 0. SegmentIterator now marks the score column like a stored column: it is read with the other common-expression columns, before the expressions run. FE, analysis: BindScore replaces BindScoreToOlapScan. It runs first on each plan the analyzer binds (a statement, and on their own its views, CTEs and subqueries), while the plan nodes of a SELECT are still its clauses, so it needs no knowledge of what analysis adds later. For each SELECT that calls score(), it checks the usage rules, asks the table in its FROM clause for a score column and replaces the score() calls with that column. BindRelation adds the column to the olap scan and rejects a relation that cannot provide it, e.g. a view, a CTE or a table stream. The column is left out of *. CheckScoreInCorrelatedSubquery rejects a scored scan under a correlated subquery after SubqueryToApply. Since score() no longer reaches type coercion as a function, the check that it is compared with numbers moves from ComparisonPredicate to BindScore, which sees the type of a literal before binding. FE, rewrite: the minimum score is an ordinary predicate of the scan. PushDownScoreRangeIntoOlapScan is removed; PushDownScoreTopNIntoOlapScan moves the minimum into the scan together with a TopN on the score, as on master, because the scan keeps the best rows of a segment only when no predicate follows. TopnFilterPushDownVisitor, the MV check and the translator lose their score() cases: a TopN filter reads the score like any column, and a materialized view without the score cannot answer a predicate on it. ### Release note None ### Check List (For Author) - Test: Unit Test and Regression test - BE: SegmentIteratorScoreCommonExprTest is new. With SegmentIteratorApplyIndexExprTest and SegmentIteratorCandidatePushdownTest, 35 tests pass. - FE: BindScoreTest (79 tests), the tests of nereids/rules/analysis, CTEInlineTest, LogicalPlanDeepCopierTest, TopNRuntimeFilterTest, PullUpProjectExprUnderTopNTest and MaterializedViewUtilsTest pass. Checkstyle passes. - Regression: the 20 suites that use score() (inverted_index_p0, search, ann_index_p0, nereids_rules_p0/cte) pass on an ASAN build of this commit. Their .out files are unchanged; two suites check the new EXPLAIN output. - Behavior changed: Yes. TopN filters on keys computed from score() reach the scan. Without a TopN on the score, EXPLAIN shows the minimum score among the scan predicates instead of SCORE RANGE. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <[email protected]>
### What problem does this PR solve? Issue Number: N/A Related PR: #69399 (merged; the plan copier fix this PR needs), #69419 (merged; fixes the pre-existing MATCH predicate inference that review of this PR found) Problem Summary: `score()` only worked as `ORDER BY score() LIMIT n` directly above the scan whose MATCH defines it. A minimum score filter without that TopN evaluated the unbound `score()` placeholder as zero and silently dropped every row: ```sql SELECT id FROM documents WHERE content MATCH 'apple' AND score() > 0.00001; -- returned nothing ``` `score()` is computed by the olap scan that evaluates the MATCH and SEARCH predicates of its SELECT. This PR binds `score()` to that scan at the start of rewrite, makes the scan's expressions read the score, and rejects every other use with an error that names the rule. **Supported.** A SELECT that reads one table and has a MATCH or SEARCH predicate in its WHERE clause can use `score()`: - in the WHERE clause, as one minimum score: `score() > x` or `score() >= x` (also `x < score()`, `x <= score()`, and with a FLOAT or DOUBLE cast); - in the select list, window functions and ORDER BY, if the SELECT does not aggregate (with GROUP BY, including GROUPING SETS, ROLLUP and CUBE, aggregate functions or DISTINCT, `score()` can only be the minimum score). A scored SELECT in a subquery, view, CTE or set operation still needs `ORDER BY score() LIMIT n`, as before. The TopN keeps the predicates of the outer query out of its scan, e.g. an outer MATCH cannot add to its score. A correlated subquery cannot call `score()`, because decorrelation removes that TopN. **Rejected.** A join, a subquery or a lateral view in the scored SELECT; a view, CTE or other relation that is not an olap table (a table stream fails during planning, because its scan is replaced by scans of the table and of its changes); any other filter on the score (`score() < x`, `score() = x`, lossy casts, ORs, a second bound); `score()` in aggregates, GROUP BY, HAVING or QUALIFY; MATCH or SEARCH in HAVING or QUALIFY of a scored SELECT, also through an alias; `score()` in a correlated subquery; a SELECT without its own MATCH or SEARCH; a nested scored SELECT without the TopN. **How.** - BE: the scan reads the score in its own expressions. `SegmentIterator` prepares the score of every row of its row bitmap before the first batch. It marked only stored columns of a pushed-down expression as common-expression columns, so an expression on the score alone, e.g. `score() > 0.5`, was never evaluated (and the scanner had already removed it from its conjuncts), and an expression that read the score next to other columns, e.g. a TopN filter on `CAST(score() AS INT)`, evaluated `Score()` itself, which returns 0. Now the score column is read with the other common-expression columns, before the expressions run. - FE, binding: `BindScoreToOlapScan` replaces `CheckScoreUsage`. It runs first in rewrite, while subqueries, views and CTEs still have their own plan nodes, so the analysis classes are as on master. For each SELECT that calls `score()`, it checks the rules above, adds a score column to the scan (score limit 0: the scan scores every row it returns) and replaces the `score()` calls of the SELECT with that column. A table is a relation, also under the filters analysis puts on it (the delete sign filter of a UNIQUE table, row policies). Binding after analysis also covers `* REPLACE (score() AS c)`, alias functions over `score()` and `ORDER BY` a position, and a UDF named `score` is never mistaken for it. - FE, rewrite: the minimum score is an ordinary predicate of the scan. `PushDownScoreTopNIntoOlapScan` pushes a TopN on the score into the scan, and with it the minimum score, exactly as before this PR (with a zero score limit when SEARCH has another predicate), because the scan keeps the best rows of a segment only when no predicate follows. TopN filter pushdown and MV rewrite need no `score()` cases: a TopN filter reads the score like any other column, and a materialized view without the score cannot answer a predicate on it. - This PR needs the plan copier fix of #69399, which is merged: copying a scan with virtual columns gave each one two ids and kept the old slot in the output. - The BE keeps the score unchanged by MATCH projections: `SegmentIterator` evaluates them without the score collector. - `SearchExpression` can be rendered as SQL, which the shape plan logged at debug level and `EXPLAIN SHAPE PLAN` need. ### Release note `score()` works in the select list, window functions and ORDER BY of a SELECT on one table whose WHERE clause has MATCH or SEARCH, with an optional minimum score `score() > x` or `score() >= x`, without `ORDER BY score() LIMIT`. A scored SELECT in a subquery, view, CTE or set operation still needs `ORDER BY score() LIMIT`. Other uses of `score()` are rejected instead of evaluating it as 0.
…#69399 (#69560) ### What problem does this PR solve? Related PR: #69399 Problem Summary: Backport #69399 to `branch-4.2`. A CTE fails to plan with "A expression contains slot not from children" when the optimizer inlines it after one of its scans got a virtual column: a CTE that orders by `score()` or by an ANN distance with a LIMIT, or that has MATCH in its select list and is inlined after rewrite. `LogicalPlanDeepCopier` gave each virtual column two ids, copied them in list order, kept the old slot in the output of the copied scan and left the score and ANN order keys on the old slots. The copier now maps every virtual column to its new id before copying their expressions and remaps the order keys, and `LogicalOlapScan.withVirtualColumns` replaces the old virtual slots in the output. #### Branch adaptations The branch has no table streams, so the change to `LogicalOlapTableStreamScan` is dropped. `withVirtualColumns` keeps the constructor call of the branch, `LogicalPlanDeepCopier` imports `LogicalOlapScan`, which master already imported, and `LogicalPlanDeepCopierTest` keeps the tests of the branch and adds the two tests of #69399. ### Release note Fix "A expression contains slot not from children" for a CTE that orders by `score()` or by an ANN distance with a LIMIT, or that has MATCH in its select list and is inlined after rewrite. ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - [ ] Manual test - Behavior changed: - [x] No. - Does this need documentation? - [x] No. ### Validation Validated together with the other two backports (#69399, #69419 and #69292 on `branch-4.2` at ecbc5fe), in a local ASAN build: - PASS: FE unit tests `LogicalPlanDeepCopierTest`, `CTEInlineTest`, `InferPredicateByReplaceTest`, `InferPredicatesTest`, `UnequalPredicateInferTest`, `BindScoreToOlapScanTest`, `SearchExpressionTest`, `TopNRuntimeFilterTest`, `PullUpProjectExprUnderTopNTest` and `MaterializedViewUtilsTest` (280 tests), and FE Checkstyle. - PASS: `./build.sh --fe --be`. - PASS: BE unit tests `SegmentIteratorScoreCommonExprTest`, `SegmentIteratorApplyIndexExprTest` and `VirtualSlotRefTest` (31 tests); clang-format 16 on the changed BE files. - PASS: the 23 regression suites of the branch that use `score()`, including `test_cte_inline_virtual_column`, `infer_match_predicate`, `test_score_filter_usage` and `test_score_filter_matrix`, with unchanged `.out` files. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <[email protected]>
…69292 (#69562) ### What problem does this PR solve? Related PR: #69292 Problem Summary: Backport #69292 to `branch-4.2`. It contains the commit of #69560, the backport of #69399, which it needs: binding runs before CTE inlining, which copies scans with virtual columns. Merge #69560 first. `score()` only worked as `ORDER BY score() LIMIT n` directly above the scan whose MATCH defines it. A minimum score filter without that TopN evaluated the unbound `score()` placeholder as zero and silently dropped every row: ```sql SELECT id FROM documents WHERE content MATCH 'apple' AND score() > 0.00001; -- returned nothing ``` `BindScoreToOlapScan`, which replaces `CheckScoreUsage`, binds `score()` to the scan of its SELECT at the start of rewrite and rejects the uses it cannot bind. The minimum score stays a predicate of the scan, and `PushDownScoreTopNIntoOlapScan` still pushes a TopN on the score together with the minimum. In the BE, an expression on the score reads the score column instead of evaluating `Score()`, and MATCH projections do not add to the score. See #69292 for the supported and rejected uses. #### Branch adaptations - The `SegmentIterator` of the branch collects the columns of pushed-down expressions in `_extract_common_expr_columns` instead of per-column states, so the score column is marked there, and the new BE test checks `_common_expr_columns` and `_is_common_expr_column`. The branch reads the columns collected there before it evaluates the expressions, also virtual ones, and a virtual slot uses a column that is already read. - When `limit + offset` overflows, `PushDownScoreTopNIntoOlapScan` keeps the behavior of the branch and skips the push-down instead of failing. - `SearchDslParser` is imported from its package on the branch. - The branch does not have #68672, which keeps predicates on virtual columns out of the condition cache. The minimum score is now a predicate of the scan, and a score depends on every rowset, so a cached result goes stale when a rowset is added: without the change, `test_score_filter_matrix` lost the rows of the first rowset. `VirtualSlotRef::get_digest` returns 0 as on master, the part of #68672 that this needs, with the `VirtualSlotRef` tests of #68672 that the branch can run. ### Release note `score()` works in the select list, window functions and ORDER BY of a SELECT on one table whose WHERE clause has MATCH or SEARCH, with an optional minimum score `score() > x` or `score() >= x`, without `ORDER BY score() LIMIT`. A scored SELECT in a subquery, view, CTE or set operation still needs `ORDER BY score() LIMIT`. Other uses of `score()` are rejected instead of evaluating it as 0. ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - [ ] Manual test - Behavior changed: - [x] Yes. A minimum score works without `ORDER BY score() LIMIT`, and unsupported uses of `score()` fail planning instead of evaluating it as 0. A minimum score without a TopN needs the BE change, so BEs must be upgraded before FEs. - Does this need documentation? - [x] Yes. The `score()` documentation should describe where `score()` can be used. ### Validation Validated together with the other two backports (#69399, #69419 and #69292 on `branch-4.2` at ecbc5fe), in a local ASAN build: - PASS: FE unit tests `LogicalPlanDeepCopierTest`, `CTEInlineTest`, `InferPredicateByReplaceTest`, `InferPredicatesTest`, `UnequalPredicateInferTest`, `BindScoreToOlapScanTest`, `SearchExpressionTest`, `TopNRuntimeFilterTest`, `PullUpProjectExprUnderTopNTest` and `MaterializedViewUtilsTest` (280 tests), and FE Checkstyle. - PASS: `./build.sh --fe --be`. - PASS: BE unit tests `SegmentIteratorScoreCommonExprTest`, `SegmentIteratorApplyIndexExprTest` and `VirtualSlotRefTest` (31 tests); clang-format 16 on the changed BE files. - PASS: the 23 regression suites of the branch that use `score()`, including `test_cte_inline_virtual_column`, `infer_match_predicate`, `test_score_filter_usage` and `test_score_filter_matrix`, with unchanged `.out` files. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <[email protected]>
…cross column equalities #69419 (#69561) ### What problem does this PR solve? Related PR: #69419 Problem Summary: Backport #69419 to `branch-4.2`. Predicate inference copied a predicate on one column to every column that is equal to it, also when the predicate contained MATCH, multi_match or SEARCH. Their result depends on the inverted index of the column, not only on its value: with `c1 = c2` and different parsers on the two columns, the copy removed matching rows, and with the same index it doubled `score()`, so a minimum score let low-scored rows through. `InferPredicateByReplace.infer` now skips every input that contains `Match`, `MultiMatch`, `Search` or `SearchExpression`. #### Branch adaptations The branch does not have the `NoneMovableFunction` check that master has next to the new check, so only the new check is added. The test imports `SearchDslParser` from its package on the branch. ### Release note Fix wrong results and scores when predicate inference copied a MATCH, multi_match or SEARCH predicate to an equal column. ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - [ ] Manual test - Behavior changed: - [x] Yes. Predicate inference no longer copies a predicate that contains MATCH, multi_match or SEARCH to an equal column. - Does this need documentation? - [x] No. ### Validation Validated together with the other two backports (#69399, #69419 and #69292 on `branch-4.2` at ecbc5fe), in a local ASAN build: - PASS: FE unit tests `LogicalPlanDeepCopierTest`, `CTEInlineTest`, `InferPredicateByReplaceTest`, `InferPredicatesTest`, `UnequalPredicateInferTest`, `BindScoreToOlapScanTest`, `SearchExpressionTest`, `TopNRuntimeFilterTest`, `PullUpProjectExprUnderTopNTest` and `MaterializedViewUtilsTest` (280 tests), and FE Checkstyle. - PASS: `./build.sh --fe --be`. - PASS: BE unit tests `SegmentIteratorScoreCommonExprTest`, `SegmentIteratorApplyIndexExprTest` and `VirtualSlotRefTest` (31 tests); clang-format 16 on the changed BE files. - PASS: the 23 regression suites of the branch that use `score()`, including `test_cte_inline_virtual_column`, `infer_match_predicate`, `test_score_filter_usage` and `test_score_filter_matrix`, with unchanged `.out` files. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <[email protected]>
### What problem does this PR solve? Related PR: apache#69399 Problem Summary: CI on branch-4.1 draws enable_common_expr_pushdown at random for each session. With it off, MATCH is not evaluated with the inverted index in the scan, so no BM25 score is collected and score() returns 0. test_cte_inline_virtual_column got score 0.0 for its TopN. Like the score suites of the branch, the suite sets enable_common_expr_pushdown = true. ### Release note None ### Check List (For Author) - Test: Regression test. With use_fuzzy_session_variable = true and the pull request number of CI, the suite failed in sessions that drew enable_common_expr_pushdown = false; with the setting pinned, four runs passed. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <[email protected]>
…#69399 (#69563) ### What problem does this PR solve? Related PR: #69399 Problem Summary: Backport #69399 to `branch-4.1`. A CTE fails to plan with "A expression contains slot not from children" when the optimizer inlines it after one of its scans got a virtual column: a CTE that orders by `score()` or by an ANN distance with a LIMIT, or that has MATCH in its select list and is inlined after rewrite. `LogicalPlanDeepCopier` gave each virtual column two ids, copied them in list order, kept the old slot in the output of the copied scan and left the score and ANN order keys on the old slots. The copier now maps every virtual column to its new id before copying their expressions and remaps the order keys, and `LogicalOlapScan.withVirtualColumns` replaces the old virtual slots in the output. #### Branch adaptations The branch has no table streams, so the change to `LogicalOlapTableStreamScan` is dropped. `withVirtualColumns` keeps the constructor call of the branch, and `LogicalPlanDeepCopierTest` and `CTEInlineTest` keep the tests of the branch and add the tests of #69399. `test_cte_inline_virtual_column` sets `enable_common_expr_pushdown = true`, as the score suites of the branch do: CI on branch-4.1 draws it at random for each session, and without it MATCH is not evaluated with the index in the scan and `score()` is 0. ### Release note Fix "A expression contains slot not from children" for a CTE that orders by `score()` or by an ANN distance with a LIMIT, or that has MATCH in its select list and is inlined after rewrite. ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - [ ] Manual test - Behavior changed: - [x] No. - Does this need documentation? - [x] No. ### Validation Validated together with the other two backports (#69399, #69419 and #69292 on `branch-4.1` at 3f911d0), in a local ASAN build: - PASS: FE unit tests `LogicalPlanDeepCopierTest`, `CTEInlineTest`, `InferPredicateByReplaceTest`, `InferPredicatesTest`, `UnequalPredicateInferTest`, `BindScoreToOlapScanTest`, `SearchExpressionTest`, `TopNRuntimeFilterTest` and `MaterializedViewUtilsTest` (228 tests), and FE Checkstyle. - PASS: `./build.sh --be` and `./build.sh --fe`. - PASS: BE unit tests `SegmentIteratorScoreCommonExprTest`, `SegmentIteratorApplyIndexExprTest` and `VirtualSlotRefTest` (31 tests); clang-format 16 on the changed BE files. - PASS: the 16 regression suites of the branch that use `score()`, including `test_cte_inline_virtual_column`, `infer_match_predicate`, `test_score_filter_usage` and `test_score_filter_matrix`. The framework regenerated the `.out` of `test_score_filter_matrix` on this branch: it drops the 46 SNII cases and keeps the 94 V2 and V3 cases of master unchanged. The other `.out` files are unchanged. - PASS: with the fuzzy session variables of CI (`use_fuzzy_session_variable = true` and its pull request number), the score suites failed in sessions that drew `enable_common_expr_pushdown = false`; with the setting pinned, four runs of `test_score_filter_usage`, `test_score_filter_matrix` and `test_cte_inline_virtual_column`, and four of `infer_match_predicate`, passed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <[email protected]>
…cross column equalities #69419 (#69564) ### What problem does this PR solve? Related PR: #69419 Problem Summary: Backport #69419 to `branch-4.1`. Predicate inference copied a predicate on one column to every column that is equal to it, also when the predicate contained MATCH, multi_match or SEARCH. Their result depends on the inverted index of the column, not only on its value: with `c1 = c2` and different parsers on the two columns, the copy removed matching rows, and with the same index it doubled `score()`, so a minimum score let low-scored rows through. `InferPredicateByReplace.infer` now skips every input that contains `Match`, `MultiMatch`, `Search` or `SearchExpression`. #### Branch adaptations The branch does not have the `NoneMovableFunction` check that master has next to the new check, so only the new check is added. The test imports `SearchDslParser` from its package on the branch. `infer_match_predicate` sets `enable_common_expr_pushdown = true`, as the score suites of the branch do: CI on branch-4.1 draws it at random for each session, and without it MATCH is not evaluated with the index in the scan and `score()` is 0. ### Release note Fix wrong results and scores when predicate inference copied a MATCH, multi_match or SEARCH predicate to an equal column. ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - [ ] Manual test - Behavior changed: - [x] Yes. Predicate inference no longer copies a predicate that contains MATCH, multi_match or SEARCH to an equal column. - Does this need documentation? - [x] No. ### Validation Validated together with the other two backports (#69399, #69419 and #69292 on `branch-4.1` at 3f911d0), in a local ASAN build: - PASS: FE unit tests `LogicalPlanDeepCopierTest`, `CTEInlineTest`, `InferPredicateByReplaceTest`, `InferPredicatesTest`, `UnequalPredicateInferTest`, `BindScoreToOlapScanTest`, `SearchExpressionTest`, `TopNRuntimeFilterTest` and `MaterializedViewUtilsTest` (228 tests), and FE Checkstyle. - PASS: `./build.sh --be` and `./build.sh --fe`. - PASS: BE unit tests `SegmentIteratorScoreCommonExprTest`, `SegmentIteratorApplyIndexExprTest` and `VirtualSlotRefTest` (31 tests); clang-format 16 on the changed BE files. - PASS: the 16 regression suites of the branch that use `score()`, including `test_cte_inline_virtual_column`, `infer_match_predicate`, `test_score_filter_usage` and `test_score_filter_matrix`. The framework regenerated the `.out` of `test_score_filter_matrix` on this branch: it drops the 46 SNII cases and keeps the 94 V2 and V3 cases of master unchanged. The other `.out` files are unchanged. - PASS: with the fuzzy session variables of CI (`use_fuzzy_session_variable = true` and its pull request number), the score suites failed in sessions that drew `enable_common_expr_pushdown = false`; with the setting pinned, four runs of `test_score_filter_usage`, `test_score_filter_matrix` and `test_cte_inline_virtual_column`, and four of `infer_match_predicate`, passed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <[email protected]>
…69292 (#69565) ### What problem does this PR solve? Related PR: #69292 Problem Summary: Backport #69292 to `branch-4.1`. It contains the commit of #69563, the backport of #69399, which it needs: binding runs before CTE inlining, which copies scans with virtual columns. Merge #69563 first. `score()` only worked as `ORDER BY score() LIMIT n` directly above the scan whose MATCH defines it. A minimum score filter without that TopN evaluated the unbound `score()` placeholder as zero and silently dropped every row: ```sql SELECT id FROM documents WHERE content MATCH 'apple' AND score() > 0.00001; -- returned nothing ``` `BindScoreToOlapScan`, which replaces `CheckScoreUsage`, binds `score()` to the scan of its SELECT at the start of rewrite and rejects the uses it cannot bind. The minimum score stays a predicate of the scan, and `PushDownScoreTopNIntoOlapScan` still pushes a TopN on the score together with the minimum. In the BE, an expression on the score reads the score column instead of evaluating `Score()`, and MATCH projections do not add to the score. See #69292 for the supported and rejected uses. #### Branch adaptations - The `SegmentIterator` of the branch collects the columns of pushed-down expressions in `_extract_common_expr_columns` instead of per-column states, so the score column is marked there, and the new BE test checks `_common_expr_columns` and `_is_common_expr_column`. The branch reads the columns collected there before it evaluates the expressions, also virtual ones, and a virtual slot uses a column that is already read. - The branch evaluates MATCH projections with `evaluate_inverted_index` directly, and that loop runs without the score collector. - `PushDownScoreTopNIntoOlapScan` keeps the overflow check of the branch for `limit + offset`, which fails like master does. - `SearchDslParser` and `CollectionSimilarity` are included from their packages on the branch. - The branch has no SNII storage format, so `BindScoreToOlapScanTest` and `test_score_filter_matrix` cover V2 and V3, and the `.out` of `test_score_filter_matrix` is regenerated without the SNII cases. - The branch does not have #68672, which keeps predicates on virtual columns out of the condition cache. The minimum score is now a predicate of the scan, and a score depends on every rowset, so a cached result goes stale when a rowset is added: without the change, `test_score_filter_matrix` lost the rows of the first rowset. `VirtualSlotRef::get_digest` returns 0 as on master, the part of #68672 that this needs, with the `VirtualSlotRef` tests of #68672 that the branch can run. - `test_score_filter_usage`, `test_score_filter_matrix` and `test_cte_inline_virtual_column` set `enable_common_expr_pushdown = true`, as the score suites of the branch do: CI on branch-4.1 draws it at random for each session, and without it MATCH is not evaluated with the index in the scan and `score()` is 0. ### Release note `score()` works in the select list, window functions and ORDER BY of a SELECT on one table whose WHERE clause has MATCH or SEARCH, with an optional minimum score `score() > x` or `score() >= x`, without `ORDER BY score() LIMIT`. A scored SELECT in a subquery, view, CTE or set operation still needs `ORDER BY score() LIMIT`. Other uses of `score()` are rejected instead of evaluating it as 0. ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - [ ] Manual test - Behavior changed: - [x] Yes. A minimum score works without `ORDER BY score() LIMIT`, and unsupported uses of `score()` fail planning instead of evaluating it as 0. A minimum score without a TopN needs the BE change, so BEs must be upgraded before FEs. - Does this need documentation? - [x] Yes. The `score()` documentation should describe where `score()` can be used. ### Validation Validated together with the other two backports (#69399, #69419 and #69292 on `branch-4.1` at 3f911d0), in a local ASAN build: - PASS: FE unit tests `LogicalPlanDeepCopierTest`, `CTEInlineTest`, `InferPredicateByReplaceTest`, `InferPredicatesTest`, `UnequalPredicateInferTest`, `BindScoreToOlapScanTest`, `SearchExpressionTest`, `TopNRuntimeFilterTest` and `MaterializedViewUtilsTest` (228 tests), and FE Checkstyle. - PASS: `./build.sh --be` and `./build.sh --fe`. - PASS: BE unit tests `SegmentIteratorScoreCommonExprTest`, `SegmentIteratorApplyIndexExprTest` and `VirtualSlotRefTest` (31 tests); clang-format 16 on the changed BE files. - PASS: the 16 regression suites of the branch that use `score()`, including `test_cte_inline_virtual_column`, `infer_match_predicate`, `test_score_filter_usage` and `test_score_filter_matrix`. The framework regenerated the `.out` of `test_score_filter_matrix` on this branch: it drops the 46 SNII cases and keeps the 94 V2 and V3 cases of master unchanged. The other `.out` files are unchanged. - PASS: with the fuzzy session variables of CI (`use_fuzzy_session_variable = true` and its pull request number), the score suites failed in sessions that drew `enable_common_expr_pushdown = false`; with the setting pinned, four runs of `test_score_filter_usage`, `test_score_filter_matrix` and `test_cte_inline_virtual_column`, and four of `infer_match_predicate`, passed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <[email protected]>
What problem does this PR solve?
Issue Number: N/A
Related PR: #69292 (depends on this fix)
Problem Summary:
A CTE fails to plan when the optimizer inlines it after one of its scans got a virtual column:
A score or ANN TopN pushed into a scan, and a MATCH in the select list, add a virtual column to the scan. After rewrite, the optimizer inlines a CTE that has one consumer left (
Optimizer.normalizeCtePlan→CTEInliner) by deep copying its rewritten body, so the copy includes these scans:score()andl2_distance_approximate()are nondeterministic, so the rewrite does not inline a CTE that calls them, even with one consumer. Once the TopN pushdown has replaced them with a virtual column of the scan, the optimizer inlines the CTE. So every CTE withORDER BY score() LIMIT nor an ANN TopN fails, with default settings.... UNION ALL SELECT * FROM c WHERE 1 = 0.LogicalPlanDeepCopiercopied a scan with virtual columns wrongly:experimental_enable_virtual_slot_for_cse=true, e.g.[v2 := v1 * v1, v1 := x + y]) kept the old id of the later one;So the plan above the copied scan read a slot that the scan does not output.
The fix:
LogicalOlapScan.withVirtualColumnsreplaces the virtual columns of the scan, but kept their old slots in the output next to the new ones. It now replaces them there too and keeps the other output slots, which the copier has already mapped. Computing the output of the copy again would not work: it gives VARIANT subpath slots new ids.AdjustNullable, the other caller that replaces existing virtual columns, no longer keeps two slots for a virtual column whose nullability changes.#69292 binds
score()to its scan before CTEs are inlined, so it needs this fix; it carries the same change until this PR is merged.Release note
Fix "A expression contains slot not from children" for a CTE that orders by
score()or by an ANN distance with a LIMIT, or that has MATCH in its select list and is inlined after rewrite.Check List (For Author)
Test
Regression test
Unit Test
Manual test (add detailed scripts or steps below)
No need to test or manual test. Explain why:
Unit tests:
LogicalPlanDeepCopierTest.testDeepCopyOlapScanWithVirtualColumn,LogicalPlanDeepCopierTest.testDeepCopyOlapScanWithDependentVirtualColumns(virtual columns that refer to each other, in both orders) andCTEInlineTest.inlineCteWhoseScanHasVirtualColumns(CTEs with a score TopN, an ANN TopN, a MATCH projection, and a VARIANT subpath with a MATCH projection, with one consumer and with a second consumer removed byWHERE 1 = 0). They fail on master (4 output slots instead of 3; a copied virtual column that refers to the old one;A expression contains slot not from children) and pass with this change.OrExpansionTest,SplitMultiDistinctTest,DecomposeRepeatWithPreAggregationTest,LogicalOlapScanTest,IvmDeltaRewriteHelperTestandPushDownVirtualColumnsIntoOlapScanTestpass. Checkstyle passes.Regression test:
nereids_rules_p0/cte/test_cte_inline_virtual_column, with its.outgenerated by the framework, on an FE built from this branch and a BE built from 41a58af plus [fix](nereids) Support minimum score filters without TopN #69292'sSegmentIteratorchange (this change is FE only). The VARIANT case is only in the unit test, because that BE readsv['x']as NULL even without a CTE.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)
🤖 Generated with Claude Code