Repository navigation
fix: error instead of wrong rows for a LATERAL with a HAVING over an ungrouped aggregate - #26874
Conversation
…ungrouped aggregate PullUpCorrelatedExpr only pulls up a HAVING that is true on an empty group, and the lateral rewrite already refuses that one. A HAVING that is false on an empty group stayed above the aggregate, so the inner join turned outer kept the outer rows it removes, with NULLs. A HAVING that reads an outer column moved into the join condition, where the count bug handling made its unmatched rows a count of 0, for a left join too. Leave both correlated so the query errors. Refs apache#21198
github-actions
Bot
added
optimizer
|
@neilconway @alamb when you have a moment, this one is ready for a look. It makes the LATERAL + HAVING shape from #21198 error instead of returning wrong rows, as discussed there. |
jayzhan211
left a comment
jayzhan211
left a comment
There was a problem hiding this comment.
@raghavyadav01 thanks for taking this on! One change is needed before merge: the new gate also turns four queries that return correct rows in the 55.2.0 release into errors. The inline comment has a narrower condition that keeps all of your error cases, plus tests for it.
| // - A HAVING that reads an outer column is moved into the join condition, | ||
| // where the count bug handling turns its unmatched rows into a count of 0, | ||
| // for a left lateral join too. | ||
| if let Some(having) = having_over_ungrouped_aggregate(subquery_plan) |
There was a problem hiding this comment.
The gate also turns queries that work in the 55.2.0 release into errors. Without a correlated WHERE, the aggregate isn't grouped by the correlation columns and keeps one row per outer row, so a HAVING that reads an outer column is just a join filter. Four such queries match DuckDB on 55.2.0 and main, and error on this branch, for example:
SELECT t1.id, sub.cnt FROM t1, LATERAL (SELECT count(*) AS cnt FROM t2 HAVING count(*) > t1.id) AS sub;
-- 55.2.0, main and DuckDB: (1, 3), (2, 3); this PR: error
SELECT t1.id, sub.cnt FROM t1 LEFT JOIN LATERAL (SELECT count(*) AS cnt FROM t2 HAVING count(*) > t1.id) AS sub ON true;
-- 55.2.0, main and DuckDB: (1, 3), (2, 3), (3, NULL); this PR: errorGating on pull_up.pulled_up_scalar_agg (so after the pull-up) keeps all your error cases and brings these back. lateral_join.slt passes with it and with the tests below, and both tests fail on the current head. Could you apply it and add the tests?
let subquery_plan = subquery.subquery.as_ref();
let original_join_filter = join.filter.clone();
+ // Walk the subquery plan bottom-up, extracting correlated filter
+ // predicates into join conditions and converting ungrouped aggregates
+ // into group-by aggregates keyed on the correlation columns.
+ let mut pull_up = PullUpCorrelatedExpr::new().with_need_handle_count_bug(true);
+ let rewritten_subquery = subquery_plan.clone().rewrite(&mut pull_up).data()?;
+ if !pull_up.can_pull_up {
+ return Ok(Transformed::no(LogicalPlan::Join(join)));
+ }
+
// A HAVING over an ungrouped aggregate can remove the aggregate's only
// row. The rewrite below handles neither case correctly, so leave the join
// correlated and let the query error instead of returning wrong rows.
@@
- if let Some(having) = having_over_ungrouped_aggregate(subquery_plan)
+ if pull_up.pulled_up_scalar_agg
+ && let Some(having) = having_over_ungrouped_aggregate(subquery_plan)
&& (original_join_type == JoinType::Inner || having.predicate.contains_outer())
{
return Ok(Transformed::no(LogicalPlan::Join(join)));
}
- // Walk the subquery plan bottom-up, extracting correlated filter
- // predicates into join conditions and converting ungrouped aggregates
- // into group-by aggregates keyed on the correlation columns.
- let mut pull_up = PullUpCorrelatedExpr::new().with_need_handle_count_bug(true);
- let rewritten_subquery = subquery_plan.clone().rewrite(&mut pull_up).data()?;
- if !pull_up.can_pull_up {
- return Ok(Transformed::no(LogicalPlan::Join(join)));
- }
-Tests for lateral_join.slt (both error on the current head, pass with the change)
# Without a correlated WHERE the aggregate keeps its single row for every
# outer row, so a HAVING that reads an outer column is a plain join filter.
query II
SELECT t1.id, sub.cnt
FROM t1, LATERAL (
SELECT count(*) AS cnt FROM t2 HAVING count(*) > t1.id
) AS sub
ORDER BY t1.id;
----
1 3
2 3
query II
SELECT t1.id, sub.cnt
FROM t1 LEFT JOIN LATERAL (
SELECT count(*) AS cnt FROM t2 HAVING count(*) > t1.id
) AS sub ON true
ORDER BY t1.id;
----
1 3
2 3
3 NULL
The other two queries that changed: max(t2.id) ... WHERE t2.id > 10 HAVING max(t2.id) > t1.id * 10 (55.2.0 and main: (1, 30), (2, 30)) and count(*) ... WHERE t2.id > 100 HAVING count(*) >= t1.id - 1 (55.2.0 and main: (1, 0)). Both match DuckDB on 55.2.0 and main, and error on this branch.
There was a problem hiding this comment.
Thanks @jayzhan211, good catch. Applied the pulled_up_scalar_agg gate, added both tests (they fail on the old head and pass now), and merged the two Filter arms.
| LogicalPlan::Filter(filter) if feeds_ungrouped_aggregate(&filter.input) => { | ||
| Some(filter) | ||
| } | ||
| LogicalPlan::Filter(filter) => having_over_ungrouped_aggregate(&filter.input), |
There was a problem hiding this comment.
Optional cleanup: the second Filter arm always returns None. If a filter further down fed the aggregate, feeds_ungrouped_aggregate(&filter.input) would already be true for this one, so the two arms can be one:
| LogicalPlan::Filter(filter) if feeds_ungrouped_aggregate(&filter.input) => { | |
| Some(filter) | |
| } | |
| LogicalPlan::Filter(filter) => having_over_ungrouped_aggregate(&filter.input), | |
| LogicalPlan::Filter(filter) => { | |
| feeds_ungrouped_aggregate(&filter.input).then_some(filter) | |
| } |
Without a correlated WHERE the ungrouped aggregate keeps one row per outer row, so a HAVING that reads an outer column is a plain join filter and those queries worked before. Suggested by jayzhan211.
|
Thanks @raghavyadav01 ! |
Which issue does this PR close?
HAVINGsupport. Following [dbolympics] General framework to decorrelate the subqueries #5492 (comment), it makes the shapes that return wrong rows error instead.Rationale for this change
LATERALsubquery over an aggregate withoutGROUP BY, with aHAVING, returns wrong rows today instead of an error:HAVINGthatPullUpCorrelatedExprpulls up, which happens when it is true on an empty group. Two cases get past it:HAVINGthat is false on an empty group stays above the aggregate. The rewrite turns the inner join into an outer join, so an outer row whose row theHAVINGremoves comes back with NULLsHAVINGthat reads an outer column is moved into the join condition, and the count bug handling turns its unmatched rows into a count of 0. This is wrong for a left lateral join too ((2, 0)where the answer is(2, NULL))What changes are included in this PR?
DecorrelateLateralJoinlooks for aHAVINGover an ungrouped aggregate in the original subquery, and leaves the join correlated (so it errors) when the join is inner, or when theHAVINGreads an outer column.HAVINGand is not affected, and neither is a left lateral join with aHAVINGthat reads only the subquery.What is the testing strategy for this PR?
lateral_join.sltcases:HAVING count(*) > 1and withHAVING max(t2.id) > 15: now an errorHAVING count(*) > t1.id: now an errorHAVING count(*) > 1, and an inner join withGROUP BY ... HAVING: still decorrelated, with the same rows as beforeAre there any user-facing changes?
OuterReferenceColumn. Every query that returned correct rows still does.🤖 Generated with Claude Code