feat: SQL conformance - #28494
Open
ritchie46 wants to merge 15 commits into
Open
Conversation
ritchie46
requested review from
MarcoGorelli,
alexander-beedie,
c-peters and
orlp
as code owners
July 23, 2026 09:51
ritchie46
marked this pull request as draft
July 23, 2026 09:51
github-actions
Bot
added
A-sql
Contributor
|
The uncompressed lib size after this PR is 60.4410 MB. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #28494 +/- ##
==========================================
- Coverage 81.61% 81.12% -0.49%
==========================================
Files 1870 1870
Lines 263028 263836 +808
Branches 3217 3217
==========================================
- Hits 214660 214036 -624
- Misses 47537 48969 +1432
Partials 831 831 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3 tasks
ritchie46
force-pushed
the
sql-conformance-fixes
branch
from
July 30, 2026 14:05
0f27912 to
e801970
Compare
Contributor
|
The uncompressed lib size after this PR is 60.4342 MB. |
- Uncorrelated and correlated scalar subquery support, including decorrelation via join_where - EXISTS / NOT EXISTS support in WHERE clauses and in general expression position - IN / NOT IN subqueries with full three-valued (SQL NULL) logic - Non-literal elements in IN-list expressions - DISTINCT support in MIN/MAX/SUM/AVG aggregates - EXCEPT / INTERSECT support, including ALL and chained set operations - SELECT-list name-collision handling - TOTAL() SQLite semantics (always returns REAL, respects DISTINCT) - group_concat now errors when combining DISTINCT with a separator - Constant-predicate JOIN ON constraints - Aggregate NULL-semantics fixes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Gate unused imports in tests/set_ops.rs behind the semi_anti_join feature (all tests in the file are already gated per-item). - Drop an unneeded `mut` in tests/correlated_subqueries.rs (SQLContext::register takes &self). - Remove a stale #[expect(clippy::too_many_arguments)] on the non-semi_anti_join stub of try_rewrite_in_subquery_as_join in subquery.rs; the stub never exceeds the argument-count threshold. - rustfmt fix in tests/in_subquery.rs. CI (lint-rust.yml) fails the build on any compiler warning via tools/cargo-fail-warning.py, so these needed fixing before merge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Four real bugs found while running the new/changed python tests under py-polars/tests/unit/sql for the first time: - `SUM(x) OVER (...)` (no ORDER BY) returned 0 instead of NULL for an empty/all-null partition. Polars' native `Expr::sum` treats an empty/all-null input as 0 (by design), and the non-windowed `SUM` path already null-guards for this, but the windowed path bypassed the guard and used the raw `.sum()`. `TOTAL(...) OVER (...)` was unaffected, since 0 is TOTAL's correct empty-input value. - `<aggregate> IN (<literals>)` produced a spurious `list[bool]` instead of `bool` when evaluated in a GROUP BY/HAVING context (e.g. `... GROUP BY g HAVING COUNT(*) IN (1, 3)` raised "cannot cast List type (inner: 'Boolean', to: 'Boolean')"). Root cause: Polars' `is_in` does not reliably reduce to scalar-per-group when its LHS is itself an aggregate, even though the equivalent `eq`/`or` chain does. Fixed by routing IN-lists whose LHS contains an aggregation through the existing OR-chain fallback (previously only used for non-literal list elements) instead of the `is_in` fast path. - `STRING_AGG(DISTINCT x, ',' ORDER BY x)` incorrectly raised "DISTINCT is only supported with a single argument", contradicting its own doc example. The "DISTINCT + separator errors" rule is a `GROUP_CONCAT` (SQLite) idiosyncrasy and was mistakenly applied to all three `STRING_AGG`/`LISTAGG`/`GROUP_CONCAT` aliases; it now only applies when the function was actually invoked as `GROUP_CONCAT`. - The "EXISTS subquery in this position" error message read "is not supported" instead of the codebase's established "is not currently supported" wording, breaking a pre-existing test that matched on the latter phrase. All four fixes are confined to crates/polars-sql/src and should be ported back to the sql2 branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…comparisons Move the SQL-conformance coverage that lived in crates/polars-sql/tests/ (correlated_subqueries, exists_subqueries, in_list, in_subquery, joins, select_naming, set_ops, subqueries, plus the additions to functions_aggregate) into py-polars/tests/unit/sql, so the cases can be checked against sqlite/duckdb oracles via `assert_sql_matches` rather than only against our own pinned expectations. Most cases were already mirrored in python at landing time; this adds the handful that weren't: - test_correlated_subqueries.py: self-correlated equality-with-exclusion case. - test_exists_subqueries.py: SEMI JOIN/ANTI JOIN plan-shape assertions for the equality-correlated EXISTS/NOT EXISTS fast path. - test_agg_semantics.py: COUNT(*) dtype pin; GROUP_CONCAT DISTINCT+ separator error (and the two cases that must still work). - test_operators.py: two additional non-literal IN-list cases needing a 4-row fixture; raw (SELECT-list) three-valued-logic value assertions for literal and column-element IN-lists; strengthened the all-literal fast-path regression to also assert the OR-chain is NOT used. - test_set_ops.py: NULL-equality for plain (non-ALL) EXCEPT; a 4-term mixed UNION ALL/EXCEPT/INTERSECT chain; positional column matching with differing operand names; ORDER BY+LIMIT over a 3-way plain UNION chain. - test_subqueries.py: unaliased scalar subquery in the SELECT list. `functions_aggregate.rs` contained pre-existing, unrelated tests (test_median, test_quantile_*, test_corr*) alongside the ones added for this feature; only the added tests were removed, restoring the file to its origin/main content. `duckdb` is already installed in CI via py-polars/requirements-ci.txt, so no dependency changes were needed; it was installed locally in this worktree's venv to actually exercise those comparisons rather than skip them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Flip `compare_with="sqlite"` to `compare_with="duckdb"` in every test this branch added or modified (test_agg_semantics.py, test_correlated_subqueries.py, test_exists_subqueries.py, test_select_naming.py, test_subqueries.py) -- DuckDB's dialect tracks our PostgreSQL alignment target much more closely than SQLite's. Pre-existing tests already on main keep whatever backend they had. Verified every flipped construct executes against DuckDB (scalar/ correlated subqueries, EXISTS, MIN/MAX/SUM/AVG DISTINCT, GROUP_CONCAT with and without DISTINCT/separator, naming collisions). No semantic mismatches were found: the full py-polars/tests/unit/sql suite passes with all comparisons actually executing (duckdb is installed in this worktree's venv), so no `expected=` values or query shapes needed to change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GROUP_CONCAT(DISTINCT a) with no ORDER BY has no guaranteed
concatenation order; comparing it against DuckDB (previous commit)
surfaced this as a flaky-looking mismatch ("1,2" vs "2,1") rather than
a real semantic divergence. Add an explicit ORDER BY inside the
aggregate, which both engines respect, to make the comparison
deterministic instead of weakening the assertion.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Correlated-subquery decorrelation has to run on the LazyFrame before
expressions are parsed, because it adds columns and so changes the
schema. That gap was bridged with two `PlHashMap<String, PlSmallStr>`
fields on SQLContext keyed by `subquery.to_string()`, which the
expression visitor consulted to resolve a subquery node to its
materialised column.
Substitute the column into an owned copy of the AST instead:
`visit_identifier` is exactly `col(ident.value)`, so an injected
`SQLExpr::Identifier` resolves to precisely what the lookup returned.
The substitution *is* the binding, so there is no key to canonicalise
and no context-resident state to clear; the two non-local invariants
that held the side table together (execute_select clearing both maps,
isolated() building from Default) stop being load-bearing.
The traversal now uses sqlparser's derived VisitorMut with a query-depth
counter that keeps each subquery a leaf, replacing collect_subqueries
and collect_exists_predicates (~120 near-identical lines that enumerated
expression-nesting positions by hand) and covering every position
sqlparser knows about rather than a listed subset. A LowerScope
separates the WHERE pass (scalar only, so a top-level EXISTS is still
claimed by the semi/anti-join fast path, which is a far better lowering)
from the SELECT list, HAVING, and the residual WHERE conjuncts.
Expressions are cloned only when a lowering actually fires, and a
read-only pre-check keeps the common no-subquery case allocation-free.
The previous scheme rendered every candidate subquery to text once per
registration and once per lookup.
Also fixes a correlated subquery in HAVING silently degrading to its
uncorrelated form. HAVING had no lowering pass, so the outer reference
resolved against the inner schema:
SELECT k, SUM(v) AS s FROM t1 GROUP BY k
HAVING SUM(v) > (SELECT SUM(w) FROM t2 WHERE t2.k = t1.k)
returned k=3 -- byte-identical to the same query with the correlation
removed -- where SQL says k=2. Lowering alone is not enough: the
materialised column is per-input-row and evaluates to a list per group,
so it reduces with `.first()`, the same shape process_subqueries already
produces for uncorrelated scalar subqueries in HAVING.
Restores the `projections.clone()` dropped in edc6646. It is
load-bearing: projection_aliases/group_key_aliases hold &str borrowed
from `projections` and outlive the move, so that commit does not build
(E0505).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ritchie46
force-pushed
the
sql-conformance-fixes
branch
from
July 31, 2026 12:50
e801970 to
af9640b
Compare
…eview
Two queries returned the wrong thing rather than failing.
QUALIFY calls process_subqueries, which hconcats a placeholder column onto
the frame, and never dropped it. Unlike the WHERE/HAVING uses of the same
helper, QUALIFY runs after the final projection, so nothing downstream
removed it and the internal `_POLARS_TMP_n` column appeared in the user's
result. The helper already returns the placeholder names; drop them after
the filter.
A projection list in which every item derived its height from a scalar
subquery collapsed to a single row:
SELECT (SELECT MAX(v) FROM t) + 1 FROM t -- 1 row
SELECT CASE WHEN (SELECT MAX(v) FROM t) > 5 THEN 'y' ELSE 'n' END FROM t -- 1 row
is_resolved_scalar_subquery matched only the bare `col(x).first()` shape, so
as soon as the subquery was an operand the projection was classified
Independent and the frame was selected down to height 1. The existing test
missed it by also selecting a real column, which supplied the height. A
resolved scalar subquery is a broadcast scalar, so neutralise it to a literal
before height classification and let the surrounding expression be judged on
its own terms; is_resolved_scalar_subquery is subsumed and removed.
Removing the visit_binary_op early bail in #28494 let Expr::SubPlan be
produced in every expression position, but only WHERE, HAVING and the SELECT
list resolved it, so ORDER BY, GROUP BY and JOIN ... ON surfaced the engine's
internal "'Expr: .subplan(no_eq)' not allowed in this context/location" where
they previously raised a clean SQLSyntaxError.
ORDER BY is now supported outright. The correlated case matters most: left
unlowered it does not error, it resolves the outer reference against the
inner schema and silently answers the uncorrelated question -- the same
failure mode as the HAVING bug fixed in af9640b. process_subqueries
reduces the broadcast placeholder with `.first()`, which is right for a
filter but yields a length-1 series where sorting needs full height, so the
sort keys reference the placeholder column directly. Placeholders are dropped
after sorting, since the set-op paths return the sorted frame with no
projection to strip them.
GROUP BY and JOIN ... ON keep their restriction but now report it in SQL
terms via reject_unresolved_subquery. A column-free ON containing a subquery
(`ON (SELECT COUNT(*) FROM t2) > 0`) was additionally mistaken for a constant
predicate and evaluated against an empty frame, since a subquery references
no column of its own; expr_contains_subquery excludes it from that path.
A correlated subquery in a GROUP BY select list leaked an internal column
name into its error ("'__POLARS_CORR_..._res' should participate in the GROUP
BY clause"). It gets the same `.first()` reduction as HAVING: the correlation
is on a grouping column, so the value is constant within the group.
Also: EXCEPT ALL numbered duplicate rows in a hardcoded
`__POLARS_SQL_SETOP_OCCURRENCE` column, which a user column of that name
collided with; use unique_column_name() as the rest of the file does. And
drop the sql_sum doc comment that outlived its function and had come to sit
on sql_corr.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nstant joins
An unaliased scalar subquery took its output column name from the internal
placeholder that `process_subqueries` broadcasts onto the frame. That name
comes from a process-wide counter, so the same query produced a different
column name on every execution:
>>> q = "SELECT (SELECT MAX(v) FROM t) FROM t"
>>> [ctx.execute(q).columns for _ in range(3)]
[['_POLARS_TMP_0'], ['_POLARS_TMP_1'], ['_POLARS_TMP_2']]
Such a projection now gets `literal`, the name Polars already uses for any
other unnamed scalar expression (`SELECT 1+1` and `SELECT CASE ... END` both
produce it), and collisions disambiguate through the existing path exactly as
`SELECT 1+1, 2+2` does. Explicit aliases are untouched.
The IN-list fast path parsed its element list twice: once through
`array_expr_to_series` purely as an is-it-all-literals probe, then again via
`visit_array_expr`, whose result was destructured behind an `unreachable!`
tied to an unenforced invariant. Failing to build the element Series *is* the
"not all literals" signal, so the parse now doubles as the test. The implicit
temporal-string conversion is split into `cast_array_elements_for` so both
callers share it and error propagation is unchanged.
A constant `INNER JOIN ... ON <true>` built a hash join over `lit(1) = lit(1)`
where a cross join is the same thing; use `JoinType::Cross`. The outer join
types keep the constant-key form, since `LEFT JOIN ... ON TRUE` must still
emit null-extended left rows when the right side is empty, which a cross join
would not.
Also rebuild the ORDER BY sort keys by value instead of moving through a
`std::mem::replace(e, Expr::Len)` sentinel, and fix typos in the
ProjectionItem doc comment.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Cleanup pass over the three preceding commits; no behaviour change. `process_subqueries` now takes a `SubqueryShape` saying how the placeholder column should be referenced. It already knew it was constructing `col(ph).first()`, and ORDER BY immediately pattern-matched that back out to strip the `.first()` again, because sorting needs a full-height expression rather than a length-1 one. Passing the shape in deletes both `broadcast_resolved_subqueries` and `resolved_subquery_column`, and one whole expression walk per ORDER BY. `expr_has_lowerable_subquery` in subquery.rs was a 28-line nested visitor re-implementing query-depth tracking to answer a question that `expr_contains_subquery` -- added to sql_visitors.rs in the same series -- already answers. It only gated a clone, so the broader predicate costs at worst one wasted clone and a no-op walk. Deleted, along with the `Visitor` import and the UFCS comment it forced. `expr_contains_subquery` itself hand-rolled a `Visitor` impl for a single predicate; sqlparser ships `visit_expressions` as exactly that closure-to-visitor adapter. `CorrelatedLowering` held `lf: Option<LazyFrame>` with a take/put-back dance, but never moved the frame -- it clones it into the callee so it survives a declined lowering -- so the `Option`, two `expect`s and the restore arm were working around a move that wasn't happening. Its `changed` flag was likewise `!bindings.is_empty()` by construction. The `is_correlated_result_col -> .first()` group-context reduction was written out twice, verbatim and with near-identical comments, 110 lines apart in `process_group_by`; extracted as `reduce_correlated_cols_in_group_context`. Also: `drop_subquery_placeholders` now uses the `cols()` idiom already used elsewhere in the file rather than a raw `Selector::ByName` literal, and the scalar-subquery output-name loop is gated on there being any placeholder at all, so the common case skips a `to_field` schema resolution per projection. Fixes an unused-import warning this series introduced: `unique_column_name` is only reachable under the `semi_anti_join` feature, so a default-feature build warned, and CI fails on warnings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
The uncompressed lib size after this PR is 60.9551 MB. |
The SQL-text side table this series replaced was cleared once per SELECT, so
a correlated subquery appearing in several places was decorrelated once and
every occurrence resolved to the same column. `CorrelatedLowering` built its
binding set per call, and `execute_select` calls it once per projection item,
so that sharing was lost:
SELECT (SELECT SUM(w) FROM t2 WHERE t2.k = t1.k) AS a,
(SELECT SUM(w) FROM t2 WHERE t2.k = t1.k) + 1 AS b FROM t1
emitted two independent row-index + join_where + group_by + join-back
pipelines where it previously emitted one. That is a runtime cost, not a
plan-time one.
The binding set is now owned by the caller and passed in, so the passes over
one frame share it. Which passes may share is a real constraint, not a
detail: a binding is only reusable while the materialised column is still
present and still means the same thing per row. Filtering and semi/anti joins
preserve both, so WHERE, the SELECT list and HAVING share one set --
restoring the old behaviour, including across the filter. ORDER BY runs after
the final projection has dropped those columns and `process_where` is also
reached from DELETE, so both keep their own; `SubqueryBindings` carries the
rule.
This costs back the `changed` flag removed in the previous commit: with a
shared set it is no longer derivable, because reusing an existing binding
rewrites the expression without recording a new one.
Verified by counting decorrelation pipelines in the query plan -- 2 before,
1 after, for each of the repeated-subquery shapes -- and pinned by a test
that also checks distinct subqueries still get one pipeline each, and that
EXISTS and a scalar subquery over the same inner query do not share (one
yields a value column, the other a boolean flag).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comments that narrate why the surrounding system looks the way it does go stale as it changes, and this series had accumulated a lot of them: comments naming other functions and describing their behaviour, explaining why a lowering is safe, or recording what a change fixed. That reasoning belongs in the commit messages, which don't have to stay in sync with the code. Removes ~80 lines of it. What stays: what a type or function *is*, invariants a caller has to respect to use an API correctly (notably when a `SubqueryBindings` set may be shared), local notes about the line they sit on, and test comments saying what a test pins -- including why the set-op ORDER BY test has no reference-engine comparison, since that shapes the test itself. No code change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
The uncompressed lib size after this PR is 60.8612 MB. |
`SQLContext` is generic over its frame type, so the bare annotation on the helper's parameter left mypy without a type argument and, in turn, unable to resolve `.explain()` on the execute result. `make lint` runs mypy in strict mode over `tests`, so this failed CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The earlier sweep only covered the commits added on top of the original conformance work; this applies the same rule to the rest of it. Cut throughout: comments naming another function and describing what it does, and comments recording current behaviour of code elsewhere. The `agg_output_root` header is the clearest case for why -- it stated that "`COUNT(*)` lowers to `len().cast(Int64)`", which this very PR changed. Trimmed rather than deleted where a comment was carrying its function's own contract: `disambiguate_output_names` keeps the naming rule and which two cases deliberately still collide, dropping the justification for each; `visit_in_list_fallback` keeps the shape it builds and that it carries three-valued logic, dropping the explanation of why Polars' operators give it for free. Kept: external-spec facts that don't drift with the code, such as `GROUP_CONCAT` (SQLite) rejecting DISTINCT alongside a separator where `STRING_AGG`/`LISTAGG` accept it, and SQL treating an unknown join condition as false. No code change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ritchie46
marked this pull request as ready for review
July 31, 2026 14:50
Contributor
|
The uncompressed lib size after this PR is 60.8615 MB. |
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.
Heavily AI generated. Will do a very thorough review pass. AI was forbidden to touch any core infrastructure, in fact nothing else than
polars-sql. It is essentially a mapping task from SQLParser.rs to Polars LazyFrame DSL.Ran against a distilled result of https://github.com/apache/datafusion/blob/main/datafusion/sqllogictest/README.md and raised the pass rate from 48.2% to 99.9%.
1100 new lines lines are tests.
Details
SQL conformance improvements for polars-sqlThis PR is the distilled result of a SQL-conformance effort that ran polars-sql against the SQLite sqllogictest corpus (~9,300 externally-authored query records), raising the pass rate from 48.2% to 99.9% with PostgreSQL as the dialect-alignment target. The measurement harness and vendored corpus live on a separate branch; this PR contains only the resulting engine changes and tests. All changes are confined to crates/polars-sql/src and py-polars/tests/unit/sql — no other crates touched, no new dependencies.
New capabilities
Behavior fixes
SUMandCORRaggregates return NULL for all-null inputs, addTOTAL#28475 — flagged for review.Tests
Python-only, in py-polars/tests/unit/sql (~1,100 added lines; the suite runs 818 tests green). New and upgraded tests use the existing assert_sql_matches helper to validate results. Error paths use pytest.raises; the EXISTS semi/anti-join and IN-list fast paths are asserted via plan text.
🤖 Generated with Claude Code Fable as orchestrator and different agents that were reviewed by Fable.