jayzhan211 commented on code in PR #27133:
URL: https://github.com/apache/datafusion/pull/27133#discussion_r4504914462
##########
datafusion/optimizer/src/decorrelate.rs:
##########
@@ -573,10 +573,15 @@ impl TreeNodeRewriter for PullUpCorrelatedExpr {
self.can_pull_up = false;
return Ok(Transformed::new(plan, false,
TreeNodeRecursion::Stop));
}
- // If the aggregation is from a distinct it will not change
the result for
- // exists/in subqueries so we can still pull up all predicates.
+ // Only a DISTINCT under EXISTS/IN may skip this check. The
pulled
+ // up column becomes an extra group key, which for a DISTINCT
only
+ // adds duplicate rows, and EXISTS/IN ignore duplicates.
Elsewhere,
+ // e.g. LATERAL, the duplicates reach the result. See #25808.
let is_distinct = aggregate.aggr_expr.is_empty();
- if !is_distinct {
+ let is_exists_or_in =
+ self.exists_sub_query || self.in_predicate_opt.is_some();
+ let can_skip_check = is_distinct && is_exists_or_in;
+ if !can_skip_check {
self.can_pull_up = self.can_pull_up &&
self.can_pull_over_aggregation;
}
Review Comment:
This also turns LATERAL queries that are correct today into errors. When the
correlated column is already a DISTINCT key, the pulled up filter reads only
group keys, so it works like a HAVING and adds no duplicates. For example,
`SELECT DISTINCT t2.id FROM t2 WHERE t2.id > t1.id * 10` returns `(1,20) (1,30)
(2,30)` on main and on 55.2.0, and an `OuterReferenceColumn` error here. The
change below doesn't decorrelate anything main doesn't already handle
correctly. It only skips the check when the pull up adds no group key:
```diff
- // Only a DISTINCT under EXISTS/IN may skip this check. The
pulled
- // up column becomes an extra group key, which for a
DISTINCT only
- // adds duplicate rows, and EXISTS/IN ignore duplicates.
Elsewhere,
- // e.g. LATERAL, the duplicates reach the result. See
#25808.
- let is_distinct = aggregate.aggr_expr.is_empty();
- let is_exists_or_in =
- self.exists_sub_query ||
self.in_predicate_opt.is_some();
- let can_skip_check = is_distinct && is_exists_or_in;
- if !can_skip_check {
- self.can_pull_up = self.can_pull_up &&
self.can_pull_over_aggregation;
- }
let mut local_correlated_cols = BTreeSet::new();
@@
)?
};
+ // A DISTINCT may skip this check when the pull up adds no
group
+ // key, or under EXISTS/IN. An added key only adds
duplicate rows,
+ // which EXISTS/IN ignore. Elsewhere, e.g. LATERAL, the
duplicates
+ // reach the result. See #25808.
+ let is_distinct = aggregate.aggr_expr.is_empty();
+ let is_exists_or_in =
+ self.exists_sub_query ||
self.in_predicate_opt.is_some();
+ let adds_group_keys = missing_exprs
+ .iter()
+ .any(|expr| !aggregate.group_expr.contains(expr));
+ let can_skip_check = is_distinct && (is_exists_or_in ||
!adds_group_keys);
+ if !can_skip_check {
+ self.can_pull_up = self.can_pull_up &&
self.can_pull_over_aggregation;
+ }
+
// if the original group expressions are empty, need to
handle the Count bug
```
<details>
<summary>Per-query results on 55.2.0, main, this PR, and with the change;
test for lateral_join.slt</summary>
| LATERAL subquery (non-equality filter) | 55.2.0 and main | this PR | with
the change |
|---|---|---|---|
| issue: `DISTINCT x WHERE y > k` | duplicates | error | error |
| `DISTINCT y WHERE y > k` | correct | error | correct |
| `DISTINCT x, y WHERE y > k` | correct | error | correct |
| `LEFT JOIN LATERAL (DISTINCT y WHERE y > k)` | correct | error | correct |
| `DISTINCT y + 1 WHERE y > k` | correct | error | error |
```sql
# A non-equality filter on a column that is already a DISTINCT key adds no
# key, so the pull up acts as a HAVING and stays decorrelated.
query II
SELECT t1.id, sub.id
FROM t1, LATERAL (SELECT DISTINCT t2.id FROM t2 WHERE t2.id > t1.id * 10) AS
sub
ORDER BY t1.id, sub.id;
----
1 20
1 30
2 30
```
With the change, `lateral_join.slt`, `subquery.slt`, `joins.slt`, the
optimizer unit tests, and clippy pass, and your new tests still hold. The test
fails on this PR as is.
</details>
##########
datafusion/sqllogictest/test_files/lateral_join.slt:
##########
@@ -855,6 +855,27 @@ ORDER BY t1.id, sub.id;
1 20 2 1
2 30 1 1
+# --- Non-equality correlated filter below a DISTINCT ---
+# https://github.com/apache/datafusion/issues/25808
+# The pull up adds `t2.id` to the DISTINCT keys, so the t2 rows 10 and 20,
+# which both have t1_id = 1, are no longer merged.
+# Expected result once supported: sub.t1_id = 1 and 2 once each, for every
+# t1 row (6 rows).
+statement error OuterReferenceColumn
Review Comment:
Optional: two more shapes returned duplicates on main and now error, `GROUP
BY` without aggregates and `LEFT JOIN LATERAL`. A `statement error` for each
would pin them:
```sql
statement error OuterReferenceColumn
SELECT t1.id, sub.t1_id
FROM t1, LATERAL (SELECT t2.t1_id FROM t2 WHERE t2.id > t1.id GROUP BY
t2.t1_id) AS sub;
statement error OuterReferenceColumn
SELECT t1.id, sub.t1_id
FROM t1 LEFT JOIN LATERAL (SELECT DISTINCT t2.t1_id FROM t2 WHERE t2.id >
t1.id) AS sub ON true;
```
--
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]
