Repository navigation
test: cover the Polars SQL surface added since py-1.44.0 (30 sqlp tests) - #4628
Merged
Merged
Conversation
50c2cf1 bumped Polars to rev 9d5804d. 20 commits touched crates/polars-sql between upstream py-1.44.0 (9ae4e57f5) and that pin, and none of them had test coverage here: the previous bump (6d33fb4, to d84c1d4) added zero new test functions to tests/test_sqlp.rs, only adjusting existing ones for join-order fallout. So the whole py-1.44.0..9d5804d window was uncovered, not just the last few days of it. Adds 28 tests and 6 shared fixtures (sqlp suite 106 -> 134). Every expectation was captured by running the freshly rebuilt binary and pinning its actual output -- upstream's own tests drive SQLContext directly, so their SQL syntax transfers but their expected values do not (qsv layers CSV type inference, null rendering and float formatting on top). New syntax and functions: - GROUPING SETS / ROLLUP / CUBE, GROUPING(), GROUPING_ID() (#29278) - the bare date-part functions YEAR/QUARTER/MONTH/WEEK/DAY/ DAYOFMONTH/DAYOFWEEK/DAYOFYEAR/HOUR/MINUTE/SECOND (#29269) - APPROX_QUANTILE, 2-4 args (#29288) - typed DATE '...' / TIMESTAMP '...' literals (#29007) - date +/- integer arithmetic; Decimal non-equi joins (#29156) - parenthesized JOIN ... ON, and aliases inside those parens (#28967, #29158) - OVER on multi-argument aggregates (#29160) - NULLS FIRST/LAST inside a window's own ORDER BY (#29159) - EXISTS/NOT EXISTS, correlated scalar subqueries, CTE shadowing, case-insensitive relation names, ORDER BY over an unselected aggregate (#29006, #29010) Behavior changes pinned: - CAST(<string> AS DATE/TIME/TIMESTAMP) parses instead of casting, and is strict where TRY_CAST is not. Two distinct failures: some rows parse -> "conversion from `str` to `date` failed"; none parse -> "could not find an appropriate format to parse dates". (#28062, #28986) - Postgres scope strictness: an aliased relation's original name is out of scope, so SELECT t1.a FROM t1 AS f now errors. (#28937) - a scalar subquery's aggregate binds to its own relation. (#28939) - unaliased constants with GROUP BY are named literal, literal:1, literal:2 rather than colliding. (#29367) Cargo.toml enables polars' `approx_quantile` feature. Upstream cfg-gates the function and ships the feature only inside its docs-selection/full umbrellas, neither of which qsv uses, so sqlp rejected APPROX_QUANTILE outright before this. Verified empirically, not inferred: "unsupported function 'approx_quantile'". qsvmcp and qsvdp share the polars dep entry and both still build; qsvlite has no polars. Two traps worth recording, both of which produced a test that asserted nothing until it was fixed: - A projection of ONLY a nulled column writes the row as an empty line, which the test CSV reader then drops -- so a TRY_CAST assertion could not tell a nulled row from a missing one. The cast fixtures carry an `id` column for exactly this reason. - sqlp_join_non_equi_decimal originally joined on whole numbers, where an integer comparison gives the same answer and the test proved nothing about Decimal. It now uses fractional values where 1.05 < 1.10 is true but any integer truncation makes it false. sqlp has no --maintain-order and its join output order has been nondeterministic since 6d33fb4, so every join test orders by a unique key rather than relying on an incidental order; each was also confirmed byte-identical over 12 runs. APPROX_QUANTILE is sketch-based, so its values are pinned exactly only after confirming byte-identical output over 20 runs -- a tolerance band against QUANTILE_CONT would have hidden numeric drift. Verified: full suite 3,958 passed / 0 failed; cargo t sqlp 5x green; 13 mutation tests on the least-obvious assertions, all 13 caught; double-run-check clean; no new clippy warnings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rouping key roborev 4815, both findings LOW and both valid. Finding 1 - the two null rows in the #29159 window tests are peers, so asserting `x` before `y` relied on an order SQL does not define. Correct, and the same species of latent fragility the repo already fixed once in 6d33fb4 ("it held over 10-12 local runs, which is not a guarantee"). I could not make the order swap - stable across POLARS_MAX_THREADS 1/2/4/8/16 and with 40 tied null rows - so this was a latent risk rather than an active flake, but the streaming engine is now the default for collect and the tie is free to move. The suggested fix - add `grp` as a secondary window ORDER BY key - is NOT usable, which is the interesting part: * `OVER (ORDER BY a NULLS FIRST, grp)` -> hard error, "OVER does not (yet) support mixed NULLS FIRST/LAST ordering for ORDER BY" * `OVER (ORDER BY a DESC NULLS FIRST, grp)` -> hard error, "OVER does not (yet) support mixed asc/desc directions for ORDER BY" * `OVER (ORDER BY a NULLS LAST, grp)` -> accepted, and SILENTLY puts the nulls FIRST. Also with `grp NULLS LAST` spelled out. So a window ORDER BY honors NULLS FIRST/LAST only with a single key; a second key drops the clause. The identical two-key ORDER BY at the TOP level honors it, so this is specific to the window path - an upstream gap in #29159 itself. Applying the suggestion would therefore have inverted the very property under test. Instead the two global-window tests stop projecting `grp`: `a`, `rn`, `cnt` and `total` are identical whichever peer lands at rn 5 vs 6, so the assertion is a total order over what it actually asserts. The PARTITION BY test keeps `grp` and its exact order - one null per partition means no peers - with a comment saying why it may. Adds sqlp_window_order_by_multiple_keys_limitation to pin all of the above, so the workaround becomes available again the moment upstream fixes it. Finding 2 - grouping_fixture has no NULL in the data, so nothing exercised the distinction GROUPING()/GROUPING_ID() exist for. Correct. Adds sqlp_grouping_distinguishes_a_data_null with real NULL categories, where two output rows both print NULL in `category` and only GROUPING() separates them: g=0 is the genuine NULL group (total 12), g=1 is the ROLLUP grand total (total 15). Added as its own test rather than by editing grouping_fixture, which nine existing tests depend on. Verified: full suite 3,960 passed / 0 failed; cargo t sqlp 5x green (136); 5 mutation tests on the new and reworked assertions, all 5 caught; double-run-check clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ne key Filed the multi-key window ORDER BY defect found while addressing roborev 4815 as pola-rs/polars#29390, with the root cause: Expr::over_with_options (polars-plan/src/dsl/mod.rs:853-861) collapses several ORDER BY keys into a single as_struct(...), and a struct holding a null field is not itself null, so SortOptions::nulls_last has nothing to act on. `descending` survives the same path, which is why only the null placement is wrong. polars-sql's own parse_order_by_in_window is correct - it validates uniformity and derives one SortOptions - so the option is lost below the SQL layer. Measured: with two keys the null placement tracks only the direction (nulls first under ASC, last under DESC) no matter what the NULLS clause asks for, so 2 of the 4 direction x placement combinations are wrong and the other 2 agree only by coincidence. Single key is correct in all 4. - sqlp_window_order_by_multiple_keys_limitation now cites the issue and the root cause, so the next reader can check whether it is fixed rather than rediscover it. - The CHANGELOG bullet claimed NULLS FIRST/LAST in a window ORDER BY works, full stop. Scoped to a single sort key and points at the upstream issue - a reader would otherwise reasonably assume it holds with a tiebreak key, which is the exact trap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bullet lists 11 date-part functions and then said "Both are ISO-based", which reads as if it refers to the whole list. It means WEEK and DAYOFWEEK, so say so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Up to standards ✅🟢 Issues
|
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
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.
Context
50c2cf18fbumped Polars to rev9d5804d. 20 commits touchedcrates/polars-sqlbetween upstreampy-1.44.0(9ae4e57f5) and that pin, and none of them had test coverage here — the previous bump (6d33fb41f, tod84c1d4) added zero new test functions totests/test_sqlp.rs, only adjusting existing ones for join-order fallout. So the wholepy-1.44.0..9d5804dwindow was uncovered, not just the last few days of it.This adds 30 tests and 7 shared fixtures (sqlp suite 106 → 136), plus one Cargo feature and the CHANGELOG entries for the surface that arrives with the bump.
Every expectation was captured by running the freshly rebuilt binary and pinning its actual output. Upstream's own tests drive
SQLContextdirectly, so their SQL syntax transfers but their expected values do not —qsv sqlplayers CSV type inference, null rendering and float formatting on top.New syntax and functions
GROUP BY GROUPING SETS/ROLLUP/CUBE,GROUPING(),GROUPING_ID()YEARQUARTERMONTHWEEKDAY/DAYOFMONTHDAYOFWEEKDAYOFYEARHOURMINUTESECONDAPPROX_QUANTILE, 2–4 argsDATE '…'/TIMESTAMP '…'literalsJOIN … ON, and aliases inside those parensOVERon multi-argument aggregatesNULLS FIRST/LASTinside a window's ownORDER BYEXISTS/NOT EXISTS, correlated scalar subqueries, CTE shadowing, case-insensitive relation names,ORDER BYover an unselected aggregateBehavior changes now pinned
CAST(<string> AS DATE/TIME/TIMESTAMP)parses instead of casting, and is strict whereTRY_CASTis not. Two distinct failures: some rows parse →conversion from `str` to `date` failed; none parse →could not find an appropriate format to parse dates(#28062, #28986).SELECT t1.a FROM t1 AS fnow errors (#28937).GROUP BYare namedliteral,literal:1,literal:2rather than colliding (#29367).The one non-test change
Cargo.tomlenables polars'approx_quantilefeature. Upstream cfg-gates the function and ships the feature only inside itsdocs-selection/fullumbrellas, neither of which qsv uses, sosqlprejectedAPPROX_QUANTILEoutright before this. Verified empirically, not inferred:unsupported function 'approx_quantile'.qsvmcpandqsvdpshare thepolarsdep entry and both still build;qsvlitehas no polars.A full sweep of
polars-sql's function registry at the pin confirmsapprox_quantileis the only gate qsv was missing — the complete set isapprox_quantile,bitwise,list_eval,nightly,rank; qsv haslist_evalandrank, andbitwiseis deliberately off.Found an upstream bug while hardening this
Addressing a review finding about tied peer rows in the
#29159tests turned up a real defect, filed as pola-rs/polars#29390: a windowORDER BYhonorsNULLS FIRST/LASTonly with a single key. Add a second key and the clause is silently ignored — null placement then tracks only the sort direction.Positions of the two null rows under
ROW_NUMBER(), options spelled identically on every key:ORDER BYa NULLS LASTa NULLS FIRSTa DESC NULLS LASTa DESC NULLS FIRSTThe same two-key
ORDER BYat the top level is correct, so it is window-specific. Root cause:Expr::over_with_options(polars-plan/src/dsl/mod.rs:853-861) collapses several keys into oneas_struct(...), and a struct holding a null field is not itself null, soSortOptions::nulls_lasthas nothing to act on.descendingsurvives the same path, which is why only null placement breaks.sqlp_window_order_by_multiple_keys_limitationpins all of it, so the assertions fail the moment upstream fixes it. The CHANGELOG bullet is scoped to a single sort key accordingly.Two traps worth knowing, both of which produced a test that asserted nothing until fixed
TRY_CASTassertion could not tell a nulled row from a missing one. The cast fixtures carry anidcolumn for exactly this reason.sqlp_join_non_equi_decimaloriginally joined on whole numbers, where an integer comparison gives the same answer and the test proved nothing about Decimal. It now uses fractional values where1.05 < 1.10is true but any integer truncation makes it false.Out of scope, recorded so nobody re-investigates:
ARRAY_INNER_PRODUCT/ARRAY_DOT_PRODUCT(#29033) needs the Array dtype, which qsv does not enable, and there is no path from CSV input to an Array-typed column throughsqlp.Order determinism
sqlphas no--maintain-orderand its join output order has been nondeterministic since6d33fb41f, so every join test orders by a unique key rather than relying on an incidental order, and each was confirmed byte-identical over 12 runs. The window tests that have peer rows project only determinate columns instead of adding a tiebreak key — see the upstream bug above for why a tiebreak is not available.APPROX_QUANTILEis sketch-based, so its values are pinned exactly only after confirming byte-identical output over 20 runs; a tolerance band againstQUANTILE_CONTwould have hidden numeric drift.Verification
cargo t sqlp5× consecutively: 136/136 each runscripts/double-run-check.py --checkcleancargo +nightly fmtcleancargo checkonqsvmcpandqsvdpboth pass with the new featureNo docs regeneration needed — no docopt USAGE change, so no
--generate-help-mdand no--update-mcp-skills.🤖 Generated with Claude Code