Repository navigation
feat: Improve SQL coverage - #29006
Merged
Merged
Conversation
SQL evaluates window functions on the result of `GROUP BY`, but `expr_reduces_group` classified `Expr::Over` as an aggregation, so a window expression was pushed into `.agg(...)` alongside the aggregates it wrapped. `avg(sum(v)) OVER (PARTITION BY c)` therefore returned a per-group list rather than the broadcast window value -- a silently wrong answer for queries that only projected the column, and a type error for those that compared it. `rank() OVER (PARTITION BY c ORDER BY sum(v) DESC)` was likewise ranking within single-row groups. Window expressions now run in a `with_columns` stage after `.agg(...)`, with the aggregates inside them hoisted into the aggregation and replaced by references to their output columns. Note that `map_expr` does not rewrite an `Expr::Over`'s `order_by` (unlike `push_expr!`), so `hoist_group_aggregates` recurses into it explicitly. Takes TPC-DS coverage from 67/99 to 72/99 (queries 47, 53, 57, 63, 89). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uVEEL5SiJifQEYPiUxszh
Two problems in `get_table_from_current_scope`. It consulted the registered `table_map` first, so a registered table shadowed a CTE of the same name and a `FROM` alias naming that CTE. TPC-DS q51 aliases a CTE as `store`, which TPC-DS also has as a base table, and q75 defines a CTE `all_sales` whose name an earlier query had already left in `table_map` as a derived-table alias. Resolution now runs innermost-scope-first: a `FROM` alias, then a CTE, then a registered table. It also matched names exactly, while unquoted SQL identifiers are case-insensitive; q49 declares an alias as `CATALOG` and refers to it as `catalog`. Relation lookup, `relation_in_scope` and the join-alias lookup in `resolve_name` now fall back to a case-insensitive match. Column names are untouched, since a frame may hold names differing only in case. Takes TPC-DS coverage from 72/99 to 75/99 (queries 49, 51, 75). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uVEEL5SiJifQEYPiUxszh
…keys Two problems behind TPC-DS q04/q11/q31/q74, which self-join a CTE under several aliases and compare the copies with `CASE ... END > CASE ... END`. `is_join_comparison` classified a condition as belonging to a join as soon as one operand named the relation being joined, without checking that the condition named nothing joined later. The `CASE` predicates span four aliases, so they were applied at the join of the third, where the fourth alias's columns did not exist yet and resolved to the third's instead -- `d.total / c.total` became `c.total / c.total`, silently dropping rows. Such conditions now stay in the residual WHERE until every relation they name is present. Repeated relations also suffix their columns, so `resolve_column` wraps references in an alias, and an alias anywhere in a join key or `join_where` predicate is rejected -- including nested inside a `CASE`. Only a single top-level alias was stripped, and only for equi keys; both paths now strip recursively. Takes TPC-DS coverage from 75/99 to 79/99 (queries 04, 11, 31, 74). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uVEEL5SiJifQEYPiUxszh
…names Three problems behind TPC-DS q10/q32/q35/q41/q69/q92. `eligible_subquery_select` required the subquery's FROM to hold exactly one entry, so a comma-joined inner relation was rejected outright and fell back to isolated execution, where the correlated column does not exist. This is what the "EXISTS subquery is not currently supported in this position" error was really reporting. Comma-separated inner relations now cross join, which the optimizer folds back into a join using the equalities that link them. `is_join_comparison` saw the outer relation named by a correlated subquery and took the comparison holding that subquery for a join predicate between two outer relations, shipping it into the join instead of leaving it for decorrelation. A condition holding a subquery is never a join key. `classify_correlation_column` treated an unqualified name present in both the inner and outer relation as unclassifiable, so a subquery over a relation the outer query also selects from was never decorrelated. Such a name binds to the innermost scope that holds it. Also hoist a predicate shared by every branch of an `OR` in a subquery's WHERE up to the top level, since only a top-level conjunct is visible to the rewrites. q41 repeats its correlation predicate in both branches of an OR. Takes TPC-DS coverage from 79/99 to 85/99. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uVEEL5SiJifQEYPiUxszh
`Expr::map_children` never rewrote `Over.order_by`, while `apply_children` (via `push_expr!`) has always traversed it, so read and write traversal disagreed on what a window's children are. `map_expr` callers therefore silently skipped it -- including `Expr::meta().undo_aliases()`, which could not strip an alias there. Rewriting it in the shared traversal removes the hand-rolled recursion that `hoist_group_aggregates` needed to work around the gap. Also fold three near-duplicates back into what already existed: `from_entry_table_names` into `declared_relations`, which additionally reaches relations nested in a parenthesized join; `combine_sql_terms` into `combine_and_conditions`, generalised to take the operator; and the OR-factoring of a subquery's WHERE, which the `EXISTS` path ran twice over the same input. `factored_selection` cloned every conjunct before deciding whether anything had been factored, so its borrowed fast path never avoided the clone, and the per-round deferred-relation sets in `process_implicit_joins` were re-derived once per candidate rather than once per round. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uVEEL5SiJifQEYPiUxszh
…ilter `is_join_comparison` accepted an operand that names the relation being joined even when it also names an already-joined one. `process_join_on` then suffixes a non-equi operand wholesale according to whether it references the right table, so the left relation's columns were renamed as the right one's. In TPC-DS q31, `(ws2.web_sales*1.0000)/ws1.web_sales` therefore became `ws2.web_sales/ws2.web_sales`, collapsing to 1.0 and inverting the comparison it feeds: counties that fail the test were returned and counties that pass were dropped (67 rows against DuckDB's 44). An operand must now name only one side. The three sibling queries were unaffected because their operands land on one side of whichever join is being built; q31 draws its six relations from two CTEs, so one operand straddles the boundary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uVEEL5SiJifQEYPiUxszh
Drop the rationale, the SQL-standard justifications and the descriptions of prior behaviour that had accumulated in the new comments, keeping only what a reader needs and cannot see from the code itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uVEEL5SiJifQEYPiUxszh
ritchie46
requested review from
MarcoGorelli,
alexander-beedie,
c-peters and
orlp
as code owners
August 27, 2026 09:16
github-actions
Bot
added
A-sql
`clippy::manual_contains` fails the lint build. The slices hold `&SQLExpr`, so `contains` compares the same way the closures did. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uVEEL5SiJifQEYPiUxszh
Contributor
|
The uncompressed lib size after this PR is 60.4710 MB. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #29006 +/- ##
==========================================
- Coverage 81.58% 81.36% -0.23%
==========================================
Files 1890 1886 -4
Lines 268762 267286 -1476
Branches 3264 3061 -203
==========================================
- Hits 219280 217481 -1799
- Misses 48637 49027 +390
+ Partials 845 778 -67 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Made with opus 5.