morrySnow commented on code in PR #63763:
URL: https://github.com/apache/doris/pull/63763#discussion_r3613116721
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AggScalarSubQueryToWindowFunction.java:
##########
@@ -130,6 +140,14 @@ private Optional<LogicalApply<Plan, Plan>>
findApply(LogicalFilter<? extends Pla
}
private boolean check(LogicalFilter<? extends Plan> outerFilter,
LogicalApply<Plan, Plan> apply) {
+ // Clear per-candidate state: the same rule instance visits every
+ // LogicalFilter in one rewriteRoot() call, and earlier rejected
+ // candidates must not leak into later ones.
+ outerPlans.clear();
+ innerPlans.clear();
+ functions.clear();
+ innerOuterSlotMap.clear();
Review Comment:
**Minor / Defensive coding:** `matchedInnerFilterConjuncts` is not cleared
here alongside `outerPlans`, `innerPlans`, `functions`, and
`innerOuterSlotMap`. It is cleared inside `checkFilter()` (line 220), so there
is no bug today — but only because `checkFilter()` is always the subsequent
check in the `&&` chain, and `matchedInnerFilterConjuncts` is only consumed by
`rewrite()` which won't be called on a failed `check()`. To prevent a future
reordering from introducing a stale-state bug, consider adding
`matchedInnerFilterConjuncts.clear();` here for consistency.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AggScalarSubQueryToWindowFunction.java:
##########
@@ -281,6 +343,55 @@ private boolean checkRelation(List<Slot> correlatedSlots) {
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) {
Review Comment:
**Refactoring opportunity:** The logic to compute `outerTables`,
`innerTables`, `outerIds`, `innerIds`, and identify the outer-only table is
duplicated across three methods: `checkRelation()` (lines 316–332),
`checkUniqueCorrelatedTable()` (this method), and `rewrite()` (lines 539–554).
Since `check()` guarantees `checkRelation()` passes before
`checkUniqueCorrelatedTable()` runs, this method could instead accept the
already-identified outer-only `CatalogRelation` as a parameter. If the author
prefers to keep it self-contained for readability, consider extracting a shared
helper: `private CatalogRelation findOuterOnlyTable()` that returns the sole
outer-only relation or null.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalOlapScan.java:
##########
@@ -1100,6 +1099,50 @@ public void computeFd(DataTrait.Builder builder) {
}
}
+ /**
+ * Whether the scan is configured with a session variable or scan
+ * mode that makes the BE return potentially duplicate rows even for
+ * declared unique keys. In these modes any uniqueness guarantee —
+ * whether from OLAP key metadata or from user-declared PRIMARY KEY /
+ * UNIQUE constraints — is unreliable.
+ */
+ private boolean isDuplicateProducingScanMode() {
+ SessionVariable sv = ConnectContext.get().getSessionVariable();
+ // skipStorageEngineMerge: BE returns unmerged versions — all
+ // table types may have duplicate key rows.
+ if (sv.skipStorageEngineMerge) {
+ return true;
+ }
+ // skipDeleteBitmap: on UNIQUE_KEYS tables, rows that were replaced
+ // due to the same key are also read, so the key duplicates.
+ if (sv.skipDeleteBitmap && getTable().getKeysType() ==
KeysType.UNIQUE_KEYS) {
+ return true;
+ }
+ // readMorAsDup: MOW tables are read as DUPLICATE — uniqueness
+ // of the declared key is not guaranteed.
+ if (getTable().getKeysType() == KeysType.UNIQUE_KEYS
+ && getTable().isMorTable()
+ && sv.isReadMorAsDupEnabled(
+ getTable().getQualifiedDbName(), getTable().getName())) {
Review Comment:
**Design suggestion:** `LogicalOlapScan` using `instanceof
LogicalOlapTableStreamScan` is a parent-class-querying-about-subclass pattern.
This works correctly but creates a subtle coupling. An alternative:
`LogicalOlapTableStreamScan` could override a protected `boolean
producesDuplicateRows()` method (default `false` in `LogicalOlapScan`). Then
`isDuplicateProducingScanMode()` calls `producesDuplicateRows()` instead of
doing instanceof. This follows the Open/Closed principle: adding future scan
subclasses with special uniqueness semantics would not require modifying
`LogicalOlapScan`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AggScalarSubQueryToWindowFunction.java:
##########
@@ -281,6 +343,55 @@ private boolean checkRelation(List<Slot> correlatedSlots) {
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) {
+ List<CatalogRelation> outerTables = outerPlans.stream()
+ .filter(CatalogRelation.class::isInstance)
+ .map(CatalogRelation.class::cast)
+ .collect(Collectors.toList());
+ List<CatalogRelation> innerTables = innerPlans.stream()
+ .filter(CatalogRelation.class::isInstance)
+ .map(CatalogRelation.class::cast)
+ .collect(Collectors.toList());
+
+ List<Long> outerIds = outerTables.stream().map(node ->
node.getTable().getId()).collect(Collectors.toList());
+ List<Long> innerIds = innerTables.stream().map(node ->
node.getTable().getId()).collect(Collectors.toList());
+
+ innerIds.forEach(outerIds::remove);
+ if (outerIds.size() != 1) {
Review Comment:
**Clarification:** Returning `true` (pass) when there isn't exactly one
outer-only table is correct (no uniqueness guarantee needed, so the check is
vacuously true). However, since `checkRelation()` is always called before this
method in the `check()` chain and already validates the same condition, these
early returns are dead code in the normal flow. Consider making this a
`Preconditions.checkState(outerIds.size() == 1, "expected exactly one
outer-only table after checkRelation")` to make the invariants explicit. The
same applies to the `outerOnlyTable == null` guard on line 384.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AggScalarSubQueryToWindowFunction.java:
##########
@@ -362,53 +523,294 @@ 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).
+ List<CatalogRelation> outerRels = outerPlans.stream()
+ .filter(CatalogRelation.class::isInstance)
+ .map(CatalogRelation.class::cast)
+ .collect(Collectors.toList());
+ List<CatalogRelation> innerRels = innerPlans.stream()
+ .filter(CatalogRelation.class::isInstance)
+ .map(CatalogRelation.class::cast)
+ .collect(Collectors.toList());
+ Set<Long> innerTableIds = innerRels.stream()
+ .map(r -> 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)) {
+ aboveWindowConjuncts.add(conj);
+ continue;
+ }
+ // Predicates referencing shared-table columns must stay
+ // ABOVE the window; otherwise the window sees fewer rows
+ // than the original scalar subquery.
+ if (referencesSharedTable(conj, sharedOuterExprIds)) {
+ aboveWindowConjuncts.add(conj);
+ } else {
+ belowWindowConjuncts.add(conj);
+ }
+ }
+ }
+
+ // Extract and classify conjuncts from any LogicalFilter nodes nested
+ // inside the outer child (apply.left()). Nested shared-table filters
+ // (e.g. f.v > 6 under a CrossJoin) must be hoisted ABOVE the window;
+ // otherwise the window would aggregate over a filtered subset of rows
+ // while the original scalar subquery sees all rows for the key.
+ //
+ // Nested outer-only filters (e.g. d.tag > 0) are safe below and can
+ // stay in place after the filters are stripped.
+ //
+ // Use a barrier-aware collector: filters below a retained unsafe
+ // filter are NOT collected, matching stripOuterFilters() semantics.
+ // Otherwise descendant predicates would be reinserted above the join
+ // while the originals remain below the unsafe filter — evaluated
+ // twice per joined row.
+ List<LogicalFilter<Plan>> nestedOuterFilters =
collectStrippableFilters(apply.left());
+ Set<ExprId> extractedConjunctExprIds = Sets.newHashSet();
+ for (LogicalFilter<Plan> nf : nestedOuterFilters) {
Review Comment:
**Clarification about barrier semantics:** For an unsafe
(volatile/NoneMovable) filter, `collectStrippableFilters()` collects it (line
770) but stops recursion below it (line 773). The loop here classifies its safe
conjuncts for extraction. However, `stripOuterFilters()` (line 632) then
reconstructs that filter with only the volatile conjuncts and does NOT recurse
into its children. This means safe filters underneath the unsafe barrier are
neither collected nor stripped — they stay in place, which is correct (the
unsafe filter is a subtree-movement barrier). Consider adding a brief comment
here noting that the barrier applies symmetrically to both collection and
stripping, to avoid confusion during future modifications.
--
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]
