fix(optimizer): EXISTS over a scalar aggregate is always true [CLAUDE] - #8304
fix(optimizer): EXISTS over a scalar aggregate is always true [CLAUDE]#8304jbylund wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
The change modifies core optimizer decorrelation behavior with subtle SQL scoping and aggregate/window semantics that warrants final human review despite the added tests.
Pull request overview
This PR adjusts unnest_subqueries.decorrelate() to correctly handle correlated EXISTS predicates when the subquery projects a scalar aggregate (no GROUP BY), where EXISTS is always true because the subquery returns exactly one row. It also adds guards to avoid unsound rewrites when the correlated predicate is not in the expected query scope or when the subquery is part of a set operation, and it introduces aggregate/window detection helpers to ensure only truly scalar aggregates are folded.
Changes:
- Fold
EXISTS(<scalar aggregate subquery>)toTRUE(and thereforeNOT EXISTS(...)toNOT TRUE) when safe, and skip folding whenHAVING/QUALIFYcan empty the scalar group. - Prevent decorrelation when
find_ancestor(exp.Predicate)resolves to a predicate in a different query scope, and skip decorrelation for correlated branches of set operations. - Add executor tests and optimizer fixture cases covering scalar aggregates, windowed aggregates, nested aggregates, and set-operation shapes.
File summaries
| File | Description |
|---|---|
sqlglot/optimizer/unnest_subqueries.py |
Adds safe folding of EXISTS over scalar aggregates and introduces scope/set-operation guards plus helper functions for windowed-aggregate detection. |
tests/test_executor.py |
Adds runtime executor coverage for the corrected EXISTS semantics across scalar aggregates, windows, nesting, and HAVING. |
tests/fixtures/optimizer/unnest_subqueries.sql |
Extends optimizer rewrite fixtures to assert correct folding/declines for scalar aggregates and problematic shapes (nested/set ops/qualify). |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
georgesittas
left a comment
georgesittas
left a comment
There was a problem hiding this comment.
@jbylund seems like handling the following input may have have regressed due to the PR's changes:
... EXISTS (SELECT COUNT(*) FROM y WHERE b = x.a FETCH FIRST 0 ROWS ONLY)The fix is likely easy. Let's get a test for this in while at it.
|
Just going to throw back into draft until I can address comments and re-self review. |
decorrelate() rewrites a correlated EXISTS into a test for at least one matching row. A subquery projecting an aggregate with no GROUP BY returns exactly one row, so EXISTS over it is always true. Fold those to TRUE, and decline the rewrite where the row count is conditional or the subquery cannot be hoisted: - a HAVING or QUALIFY can empty the single group - find_ancestor(exp.Predicate) crosses query boundaries, so the predicate can belong to another query - the join is added to the subquery's own parent_select, so the rewrite was never sound there - a branch of a set operation cannot be hoisted out of it Only the function a window is applied to is windowed: an aggregate in the window spec, or in that function's arguments, still groups. Verified against duckdb 1.5.5. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d [CLAUDE] exp.Fetch is a separate node from exp.Limit, so FETCH FIRST 0 ROWS ONLY slipped past the bail-out and the subquery was folded to TRUE despite returning no rows. isinstance(select.parent, exp.SetOperation) misses a branch wrapped in its own parens, since the parent is then a Subquery. Ask the scope instead, which classifies every set operation branch the same way. The executor can't run FETCH, so that case is a fixture test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6148246 to
b3880c2
Compare
georgesittas
left a comment
georgesittas
left a comment
There was a problem hiding this comment.
Just a couple of nits, should be good to go otherwise.
| node = agg | ||
|
|
||
| while ( | ||
| node.arg_key == "this" |
There was a problem hiding this comment.
Can we explain with a 1-line comment why we check this? Seems like a non-trivial contract. What is it that you're really trying to express?
| ): | ||
| node = node.parent | ||
|
|
||
| return isinstance(node.parent, exp.Window) and node.arg_key == "this" |
There was a problem hiding this comment.
Ditto.
Perhaps something like window.args["some_special_arg"] is node reads more clearly vs asserting the child's own arg key. It's a bit indirect now: "I'm this in my parent, and my parent is a Window, so ..." vs "the window parent's this is me, so I'm special".
There was a problem hiding this comment.
I restructured the whole walk up, I think it's more direct (less node.parent.x), and I like not having the dense conditional, but let me know what you think.
#60) `node.parent.this is node` states the relation from the window's side - "the window's function is me" - instead of asserting the child's own `arg_key` and leaving the reader to join that to the parent check. Same phrasing in the loop condition, so both places now read the same way. Two one-line comments: what the check expresses (a window applies to exactly one function, its `this`) and why the loop climbs (parens, `FILTER` and `IGNORE NULLS` wrap that function without changing which one it is). Behaviour is unchanged - the old and new forms agree on all 33 aggregates across the 23 window shapes I could construct, including `FILTER`, `IGNORE NULLS`, `WITHIN GROUP`, parenthesized specs, nested windows and `MAX(SUM(b)) OVER ()`. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
decorrelate()rewrites a correlatedEXISTSinto a test for at least one matching row. A subquery projecting an aggregate with noGROUP BYreturns exactly one row, soEXISTSover it is always true. Fold those toTRUE, and decline the rewrite where the row count is conditional or the subquery can't be hoisted.SELECT a FROM x WHERE ...EXISTS (SELECT COUNT(*) FROM y WHERE b = x.a)[2][1, 2, NULL]NOT EXISTS (SELECT COUNT(*) FROM y WHERE b = x.a)[1, NULL][]EXISTS (SELECT COUNT(*) FROM y WHERE b = x.a HAVING COUNT(*) = 0)[][1, NULL]EXISTS (SELECT LAG(SUM(b)) OVER (ORDER BY 1) FROM y WHERE b = x.a)[2][1, 2, NULL]EXISTS (SELECT * FROM (SELECT COUNT(*) AS c FROM y WHERE b = x.a) AS t WHERE t.c > 5)[]EXISTS (SELECT COUNT(*) FROM y WHERE b = x.a INTERSECT SELECT 0)[2][1, NULL]A
HAVINGorQUALIFYcan empty the single group, so it isn't folded.find_ancestor(exp.Predicate)crosses query boundaries, so the last two rows resolved the outerEXISTSand folded it, dropping the predicates around it - the join is added to the subquery's ownparent_select, so that rewrite was never sound in those shapes. Only the function a window is applied to is windowed: an aggregate in the spec, or in that function's arguments, still groups.GROUP BY,COUNT(*) OVER ()and non-aggregate projections are unchanged. Verified against duckdb 1.5.5.