github-actions[bot] commented on code in PR #63763:
URL: https://github.com/apache/doris/pull/63763#discussion_r3620727629
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AggScalarSubQueryToWindowFunction.java:
##########
@@ -184,17 +211,33 @@ private boolean checkAggregate() {
}
/**
- * check inner scope only have one filter. and inner filter is a sub
collection of outer filter
+ * check inner filter conjuncts are a sub collection of outer filter.
+ * Also records which outer conjuncts were matched against inner conjuncts
so that rewrite()
+ * can force them below the window — they are part of the inner
aggregate's filter, not extra
+ * outer-only predicates.
+ * <p>
+ * PushDownFilterThroughProject can split a single LogicalFilter into
multiple filters
+ * (correlated predicates that reference slots not in a project's output
stay above,
+ * while non-correlated predicates can be pushed below). We collect
conjuncts from ALL
+ * inner filters rather than requiring exactly one.
*/
private boolean checkFilter(LogicalFilter<? extends Plan> outerFilter) {
List<LogicalFilter<Plan>> innerFilters = innerPlans.stream()
.filter(LogicalFilter.class::isInstance)
.map(p -> (LogicalFilter<Plan>)
p).collect(Collectors.toList());
- if (innerFilters.size() != 1) {
+ matchedInnerFilterConjuncts.clear();
+ // An inner plan with zero filters cannot prove its shape
+ // through checkFilter — reject early to avoid bypassing the
+ // relation/identity/uniqueness proofs that depend on filter
+ // conjunct matching.
+ if (innerFilters.isEmpty()) {
Review Comment:
[P1] Reject unaccounted non-catalog relations
This nonempty-filter guard still leaves the relation proof bypassable.
`LogicalTVFRelation` is accepted through `LogicalRelation`, but
`checkRelation()`, slot mapping, shared-slot classification, and
`isSameScanDomain()` only inspect `CatalogRelation`. For example, with `d.id`
unique/non-null, `SELECT n.number FROM numbers("number"="2") n, d WHERE d.id >
0 AND 3 = (SELECT COUNT(*) FROM numbers("number"="3") n2 WHERE d.id > 0)`
passes this guard because the correlated inner filter is nonempty and matches
the outer predicate. The scalar subquery counts 3 rows, while the rewrite
counts the two outer TVF rows in `COUNT(*) OVER (PARTITION BY d.id)`, changing
the result. The prior zero-filter CTE thread called out this generic
relation-accounting invariant; rejecting only empty filters does not close it.
Please reject non-`CatalogRelation` leaves here, or prove
identity/mapping/row-domain equivalence for each accepted relation kind, and
add this nonempty-filter negative case.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AggScalarSubQueryToWindowFunction.java:
##########
@@ -274,13 +344,64 @@ private boolean checkRelation(List<Slot> correlatedSlots)
{
createSlotMapping(outerTables, innerTables);
+ // Shared-table scans must have equivalent row-domain semantics.
+ // Table.getId() equality alone is insufficient: wrappers
+ // (RowBinlogTableWrapper, OlapTableStreamWrapper) share the base
+ // table ID but read different rows. Likewise, two scans of the
+ // same base table can differ by @incr params, partition/tablet
+ // selection, index, snapshot, or sample.
+ if (!isSameScanDomain(outerTables, innerTables)) {
+ return false;
+ }
+
+ // Identify and stash the sole outer-only table for downstream checks
+ // and for rewrite(). checkRelation() already validated that exactly
+ // one outer-only table exists.
+ outerOnlyTable = outerTables.stream()
+ .filter(node -> outerIds.contains(node.getTable().getId()))
+ .findFirst().get();
+ Preconditions.checkState(outerOnlyTable != null,
+ "outerOnlyTable must be non-null after checkRelation
validation");
+
Set<ExprId> correlatedRelationOutput = outerTables.stream()
.filter(node -> outerIds.contains(node.getTable().getId()))
.map(LogicalRelation.class::cast)
.map(LogicalRelation::getOutputExprIdSet).flatMap(Collection::stream).collect(Collectors.toSet());
return correlatedSlots.stream().allMatch(e ->
correlatedRelationOutput.contains(e.getExprId()));
}
+ /**
+ * The correlated columns of the outer-only table must form a unique,
non-null
+ * key for the WinMagic window-function rewrite to be correct. Without
unique
+ * and non-null keys, the window function may aggregate over duplicated or
+ * null-grouped outer rows, producing wrong results for aggregates like SUM
+ * and COUNT.
+ * <p>
+ * In particular, nullable correlated keys are unsafe even with a
uniqueness
+ * guarantee: PARTITION BY groups all null-key outer rows into a single
+ * partition. Under a null-safe equality ({@code <=>}) correlation, those
+ * rows can join the same null-key inner rows and multiply the window
+ * aggregate, whereas the original scalar subquery is evaluated per outer
row.
+ * <p>
+ * Uses {@link DataTrait#isUniqueAndNotNull(Set)} which covers OLAP key
+ * metadata (PRIMARY_KEYS / UNIQUE_KEYS), declared constraints
+ * (PRIMARY KEY / UNIQUE), and rejects nullable slots.
+ */
+ private boolean checkUniqueCorrelatedTable(List<Slot> correlatedSlots) {
+ // outerOnlyTable was identified and stashed by checkRelation() which
+ // runs before this method in the check() && chain, so it is guaranteed
+ // non-null here.
+ Preconditions.checkState(outerOnlyTable != null,
+ "checkRelation() must run and set outerOnlyTable before "
+ + "checkUniqueCorrelatedTable()");
+
+ // Check uniqueness and non-nullability via DataTrait on the
correlated (outer-only) table.
+ // Must use isUniqueAndNotNull: nullable unique keys are unsafe for
window-rewrite
+ // because PARTITION BY groups all null-key rows together.
+ DataTrait dataTrait = outerOnlyTable.getLogicalProperties().getTrait();
+ return dataTrait.isUniqueAndNotNull(Sets.newHashSet(correlatedSlots));
Review Comment:
[P1] Reject partial uniqueness from direct rollups
This gate can trust a false trait for an explicitly selected non-base index.
A direct rollup scan outputs the rollup schema, but
`LogicalOlapScan.computeUnique()` first propagates declared constraints and
only then returns for `selectedIndexId != baseIndexId`; `findSlotsByColumn()`
silently keeps the constraint columns that remain. Consequently, a rollup
`(a,c)` on a table declared `UNIQUE(a,b)` records `{a}` as unique, although
valid rows `(1,1,c1)` and `(1,2,c2)` both have `a=1`. Using that rollup as the
outer-only table lets this rewrite put both rows in one `PARTITION BY a` and
multiply the aggregate versus the per-row scalar subquery. Please reject
non-base outer-only scans unless uniqueness is proved from that index, and
require all columns of a declared composite constraint before propagating it.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AggScalarSubQueryToWindowFunction.java:
##########
@@ -362,53 +602,297 @@ private Plan rewrite(LogicalFilter<? extends Plan>
filter, LogicalApply<Plan, Pl
windowFilterConjunct = ExpressionUtils.replace(windowFilterConjunct,
ImmutableMap.of(aggOut.toSlot(), aggOutExpr));
- LogicalFilter<Plan> newFilter =
filter.withConjunctsAndChild(conjuncts.get(true), apply.left());
+ // Split uncorrelated conjuncts: predicates that reference ONLY shared
+ // relation slots (tables appearing in both outer and inner plans) must
+ // stay ABOVE the window. Otherwise the window function would see a
+ // different set of rows than the original scalar subquery.
+ //
+ // For example, with fact(f) as shared table and dim(d) as outer-only:
+ // f.v > 6 → shared-only → must stay above the window
+ // d.tag > 0 → outer-only → safe below the window
+ // f.k = d.k → join cond → needed below the window
+ //
+ // We find shared tables by comparing table IDs that appear in both
+ // outer and inner plans, then collect ALL output slots of those
+ // tables (not just columns referenced in the inner query).
+ // outerOnlyTable is already identified by checkRelation().
+ List<CatalogRelation> outerRels = outerPlans.stream()
+ .filter(CatalogRelation.class::isInstance)
+ .map(CatalogRelation.class::cast)
+ .collect(Collectors.toList());
+ Set<Long> innerTableIds = innerPlans.stream()
+ .filter(CatalogRelation.class::isInstance)
+ .map(r -> ((CatalogRelation) r).getTable().getId())
+ .collect(Collectors.toSet());
+ Set<ExprId> sharedOuterExprIds = outerRels.stream()
+ .filter(r -> innerTableIds.contains(r.getTable().getId()))
+ .flatMap(r -> r.getOutput().stream())
+ .map(Slot::getExprId)
+ .collect(Collectors.toSet());
+ Set<Expression> uncorrelatedConjuncts = conjuncts.get(true);
+ Set<Expression> belowWindowConjuncts = Sets.newHashSet();
+ Set<Expression> aboveWindowConjuncts = Sets.newHashSet();
+ if (uncorrelatedConjuncts != null) {
+ for (Expression conj : uncorrelatedConjuncts) {
+ // Conjuncts that were matched against inner subquery filter
+ // conjuncts (tracked by checkFilter) must stay BELOW the
+ // window. They are semantically part of the inner aggregate's
+ // filter, not extra outer-only predicates. Placing them above
+ // the window would let the window see more rows than the
+ // original scalar subquery, producing wrong aggregate results.
+ if (matchedInnerFilterConjuncts.contains(conj)) {
+ belowWindowConjuncts.add(conj);
+ continue;
+ }
+ // Volatile and NoneMovableFunction predicates must stay
+ // ABOVE the window — pushing them below would change
+ // evaluation frequency or move a side-effecting call.
+ if (isVolatileOrNoneMovable(conj)) {
Review Comment:
[P1] Keep error-preserving filter siblings together
This per-conjunct split can change which rows reach a `NoneMovableFunction`.
Consider a nested candidate whose top filter contains both `x.keep > g.keep`
and `assert_true(x.tag > g.tag, 'bad')`, where `g` is a grandparent correlation
and `x.k` is the candidate's unique/non-null key. The `g` slots keep both
predicates in the same filter above the inner Apply. In that original operator,
BE evaluates `assert_true` against the full input block even if the cheap
sibling rejects a row. Here the comparison is put below the window while the
assertion is put above it, so that row is physically removed before
`assert_true` runs and an error can disappear. Please treat a filter containing
any volatile/`NoneMovableFunction` conjunct as an indivisible barrier (or
reject the rewrite), and add a same-node mixed-conjunct regression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
