github-actions[bot] commented on code in PR #63763:
URL: https://github.com/apache/doris/pull/63763#discussion_r3600057032
##########
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) {
+ return true;
+ }
+
+ CatalogRelation outerOnlyTable = outerTables.stream()
+ .filter(node -> outerIds.contains(node.getTable().getId()))
+ .findFirst().orElse(null);
+ if (outerOnlyTable == null) {
+ return true;
+ }
+
+ // 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] Make uniqueness validation honor duplicate-producing scan modes
This accepts the scan's `DataTrait`, but `LogicalOlapScan` still advertises
a unique key under `skip_storage_engine_merge`, even though BE direct mode
exposes unmerged versions. It also calls `super.computeUnique()` before the
`skip_delete_bitmap` / `read_mor_as_dup_tables` early returns, so declared
UNIQUE/PK traits survive those duplicate-producing modes. With two visible
`d(k=1)` versions and one matching fact row `v=10`, the original scalar sum is
10 per outer row and `20 > 10` keeps both; this rewrite groups both versions in
`PARTITION BY d.k`, produces 20, and `20 > 20` drops both. Please make the
trait scan-mode-aware or reject the rewrite for these modes, and add negative
coverage for each path.
--
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]
