Repository navigation
fix: error instead of wrong result for LATERAL DISTINCT with a non-equality correlated filter - #27133
CuteChuanChuan wants to merge 3 commits into
Conversation
…uality correlated filter
github-actions
Bot
added
optimizer
jayzhan211
left a comment
jayzhan211
left a comment
There was a problem hiding this comment.
@CuteChuanChuan thanks for working on this! Erroring on the issue's query matches #25808. One change below keeps queries that are correct today from erroring too.
| // 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; | ||
| } |
There was a problem hiding this 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:
- // 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 bugPer-query results on 55.2.0, main, this PR, and with the change; test for lateral_join.slt
| 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 |
# 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 30With 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.
There was a problem hiding this comment.
Thanks for pointing this out. I only considered whether duplicates matter, not whether they're created at all. Addressed in 5eed032.
| # 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 |
There was a problem hiding this 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:
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;
|
@jayzhan211 |
jayzhan211
left a comment
jayzhan211
left a comment
There was a problem hiding this comment.
@CuteChuanChuan thanks for the quick turnaround! One more case from my earlier suggestion needs the change inline. Optional: the description still says the check is skipped "only for a DISTINCT under EXISTS/IN"; it now also covers a pull up that adds no key.
| // 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); |
There was a problem hiding this comment.
@CuteChuanChuan thanks for the update! My earlier suggestion still misses one case, sorry about that: SELECT DISTINCT t2.id FROM t2 WHERE t2.t1_id = t1.id AND t2.id > t1.id * 10 + 5 returns (1, 20), (2, 30) on 55.2.0 and main, but errors here. The pull up adds t2.t1_id as a key, but the equality pins it to one value per outer row, so it splits no group. is_partitionable_equality already decides this for the window pull up, so the gate can reuse it:
| // 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); | |
| // A DISTINCT may skip this check under EXISTS/IN, or when the pull | |
| // up adds no group key that splits a group. An added key only adds | |
| // duplicate rows, which EXISTS/IN ignore; elsewhere, e.g. LATERAL, | |
| // they reach the result (#25808). A key that a correlated equality | |
| // pins to one value per outer row splits no group. | |
| let is_distinct = aggregate.aggr_expr.is_empty(); | |
| let is_exists_or_in = | |
| self.exists_sub_query || self.in_predicate_opt.is_some(); | |
| let pinned_cols: Vec<&Expr> = self | |
| .join_filters | |
| .iter() | |
| .filter(|f| is_partitionable_equality(f, aggregate.input.schema())) | |
| .filter_map(|f| split_column_equality(f).map(|(col, _)| col)) | |
| .map(|col| match col { | |
| Expr::Cast(Cast { expr, .. }) => expr.as_ref(), | |
| _ => col, | |
| }) | |
| .collect(); | |
| let adds_group_keys = missing_exprs.iter().any(|expr| { | |
| !aggregate.group_expr.contains(expr) && !pinned_cols.contains(&expr) | |
| }); | |
| let can_skip_check = is_distinct && (is_exists_or_in || !adds_group_keys); |
Results on 55.2.0, main, this PR and with the change; tests for lateral_join.slt
| LATERAL DISTINCT subquery | 55.2.0 and main | this PR | with the change |
|---|---|---|---|
DISTINCT t2.id WHERE t2.t1_id = t1.id AND t2.id > k |
correct | error | correct |
same with CAST(t2.t1_id AS BIGINT) = t1.id |
correct | error | correct |
same as LEFT JOIN LATERAL |
correct | error | correct |
DISTINCT v WHERE k = t1.id AND g > t1.id - 3 (adds g) |
duplicates | error | error |
lossy cast CAST(d AS INT) = t1.id |
duplicates | error | error |
float key f = o.f AND v > k |
correct | error | error |
| issue query | duplicates | error | error |
Float keys still error because is_partitionable_equality rejects them (-0.0 = 0.0).
Each test below fails if its condition is removed. With the change, lateral_join.slt, subquery.slt, joins.slt, the optimizer unit tests and clippy pass.
@@ -898,6 +898,32 @@ ORDER BY t1.id, sub.id;
1 30
2 30
+# The pull up adds `t2.t1_id`, but the equality pins it to one value per t1
+# row, so it splits no group and the query stays decorrelated.
+query II
+SELECT t1.id, sub.id
+FROM t1, LATERAL (SELECT DISTINCT t2.id FROM t2 WHERE t2.t1_id = t1.id AND t2.id > t1.id * 10 + 5) AS sub
+ORDER BY t1.id, sub.id;
+----
+1 20
+2 30
+
+# The same through a cast that keeps distinct values distinct.
+query II
+SELECT t1.id, sub.id
+FROM t1, LATERAL (SELECT DISTINCT t2.id FROM t2 WHERE CAST(t2.t1_id AS BIGINT) = t1.id AND t2.id > t1.id * 10 + 5) AS sub
+ORDER BY t1.id, sub.id;
+----
+1 20
+2 30
+
+# A lossy cast does not pin `d`: 1.2 and 1.4 both match t1.id = 1, so the
+# added key would return v = 7 twice.
+# Expected result once supported: (1, 7) once.
+statement error OuterReferenceColumn
+SELECT t1.id, sub.v
+FROM t1, LATERAL (SELECT DISTINCT td.v FROM (VALUES (1.2, 7), (1.4, 7)) AS td(d, v) WHERE CAST(td.d AS INT) = t1.id AND td.v > t1.id) AS sub;
+
# --- Implicit LATERAL (comma without LATERAL keyword) ---| SELECT t1_id FROM t1 WHERE t1_int IN (SELECT DISTINCT t2_int FROM t2 WHERE t2_id > t1_id) | ||
| ---- | ||
| 11 | ||
| 33 |
There was a problem hiding this comment.
Optional: no test covers the is_distinct part of the gate. Without it, this query returns 11 instead of 11, 22. A statement error would pin it:
| 33 | |
| 33 | |
| #in_subquery_with_non_equality_filter_below_group_by | |
| #not de-correlated: the pulled up `t2_id` would become an extra group key and | |
| #change the counts. Expected result once supported: 11, 22. | |
| statement error InSubquery | |
| SELECT t1_id FROM t1 WHERE t1_int IN (SELECT count(*) FROM t2 WHERE t2_id > t1_id GROUP BY t2_int) |
|
@jayzhan211
|
Which issue does this PR close?
Rationale for this change
A
LATERALsubquery withSELECT DISTINCTand a non-equality correlated filter returns duplicate rows. As suggested in the issue discussion, this PR makes the query error instead of returning wrong results (see @alamb 's comments).What changes are included in this PR?
When the correlated filter is pulled above an
Aggregate, its column is added to the group keys. For aDISTINCT, this only adds duplicate rows.EXISTSandINignore duplicates, so currentlyPullUpCorrelatedExprskips thecan_pull_over_aggregationcheck for everyDISTINCT. However,LATERALreturns those duplicates.This PR skips the check for a
DISTINCTonly when the duplicates cannot reach the result:EXISTS/IN, which ignore duplicates, orDISTINCTkey, and an added key splits no group if a correlated equality pins it to one value per outer row (decided by the existingis_partitionable_equality, as for the window pull up).Otherwise, e.g. the issue's query, the
LATERALsubquery is no longer decorrelated and fails with the sameOuterReferenceColumnerror that theGROUP BYform already gives.What is the testing strategy for this PR?
lateral_join.slt: the issue's query now errors but the equality form still returns correct answer.subquery.slt:EXISTSandINwithDISTINCTand a non-equality filter are still decorrelated and return the right rows. No existing test covered this case.Are there any user-facing changes?
The
LATERALquery in the issue now returns aThis feature is not implemented errorinstead of wrong results.