test(parity): corpus tiers 08-10; file P13-P26; pin DuckDB - #42
Merged
Conversation
The P-log was exhausted apart from P3, so new findings now come from widening the corpus rather than working the backlog. Three tiers seeded with the three divergences found while planning, each paired with the control cases that locate the defect rather than merely observing it. 83 -> 96 cases. P13 - trailing unparsed tokens are silently discarded. Filed from `ORDER BY ... NULLS LAST LIMIT 3` returning all 20 rows, but the root cause is much broader: there is no NULLS handling in src/sql/ at all, and the parser silently ignores everything after the first token it cannot place. A nonsense token proves it - `... ORDER BY amount DESC FROBNICATE LIMIT 3` runs clean and drops the LIMIT. Any typo, or any clause we don't support, degrades into a different query that succeeds. Pinned directly as an OURS_ONLY case, since the reference engine correctly rejects it. P14 - an ungrouped aggregate over an empty set returns no row where standard SQL returns exactly one (COUNT -> 0, others -> NULL). The grouped form is already correct, and a control case pins that, so the fix stays narrow. P15 - QUALIFY rejects an inline window function. Controls show QUALIFY works fine against a SELECT-list alias and the same window expression evaluates fine in the SELECT list, which locates the defect in ExpressionLifter - it walks the SELECT list only, so an inline window fn in QUALIFY is never hoisted and reaches the WHERE evaluator raw. Fix site is expression_lifter, not qualify_to_where_transformer, despite the name. Also records the expected corpus churn for P13: fixing the parser moves the NULLS cases DIFFER -> GAP, not to AGREE, since NULLS ordering is a separate missing feature. A hard error is the correct intermediate state. R-log updated: expression_lifter now has a parity case behind it, the two transformers migrated since the table was written are marked done, and P15 is added to R3's list of confirmed live bugs from unvisited branches. Parity contract holds at 96 cases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds data/null_edges.csv, a 12-row fixture built specifically to expose NULL behaviour - every other corpus CSV is NULL-free, so NULL semantics were simply unassertable. It carries NULLs in a sort key, a group/partition key, a string column, a join key, and an entirely-NULL column, plus deliberate ties, and a unique never-NULL `id` as a total-order tiebreak. Verified before building on it that BOTH engines see the same NULLs in all five nullable columns - otherwise these cases would be testing the CSV reader rather than the SQL engine. Two harness properties shaped the design and are recorded in the tier header: normalize.py canonicalises "" to NULL so the ''-vs-NULL distinction is invisible to the comparison, and has_order_by() is a substring check so an OVER(ORDER BY) alone forces ordered comparison - every case therefore needs a total ordering. Five new divergences, each pinned with the baseline that isolates it: P16 - ORDER BY <ordinal> is silently ignored; rows come back in insertion order with no error. The integer is evaluated as a constant so every row compares equal. Unambiguous bug: ordinals are standard SQL. P17 - default NULL placement differs on ASC. We sort NULL as the minimum value (SQLite/MySQL); DuckDB pins NULLS LAST in both directions. The rules coincide on DESC and diverge on ASC. Standard SQL leaves this implementation-defined, so it is filed as a decision, not a defect. P18 - `= NULL` matches NULL rows instead of yielding UNKNOWN. Produces extra rows, which is the more dangerous direction. P19 - NOT IN does not exclude NULLs, though `<>` does. The inconsistency inside our own NULL handling is what makes this a bug rather than a stance. P20 - `||` treats NULL as an empty string while arithmetic correctly propagates NULL. Defensible as coercion-first (Oracle agrees), but the internal inconsistency with `+` is not. Filed as a decision. Cases are built one-variable-at-a-time: the multi-key, alias, expression and two-direction ordering cases filter NULLs out of the sort key so they test ordering machinery rather than re-testing P17, and all four AGREE. 83 -> 123 cases; contract holds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…n NULL ordering Where the SQL standard leaves a choice open, we follow DuckDB rather than making a case-by-case judgement: NULLs sort last in both directions, plus explicit NULLS FIRST/LAST from P13 stage 2. Notes that this is a user-visible change to ORDER BY over NULL-bearing data, and which corpus cases flip when it lands. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both implementation-defined findings so far (P17, P20) resolved the same way, so the preamble now states it as policy: where the standard leaves a choice open, match the reference engine. Diverging stays available but has to be argued on design grounds and recorded under Deferred. Notes the consequence — pinning implementation-defined cases makes DuckDB's version part of the contract, so it should be bumped deliberately rather than floating — and records CONCAT() as the escape hatch if empty-string coercion turns out to matter for messy-data exploration. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Window functions were a large, working, completely untested surface - the corpus had no OVER clause anywhere. First pass produces six findings, four of them silent, plus fifteen baselines pinning the parts that are correct. P21 - window functions are evaluated BEFORE the WHERE clause, so they see the unfiltered row set. The identical query without a WHERE agrees with DuckDB exactly, which proves partitioning and the functions themselves are fine and the defect is purely pipeline position. Affects essentially every real window query, silently. Pinned through COUNT(*), ROW_NUMBER and a derived table. P22 - FIRST_VALUE, NTH_VALUE, NTILE, PERCENT_RANK and CUME_DIST return NULL for every row rather than erroring. LAST_VALUE, LAG, LEAD, ROW_NUMBER, RANK, DENSE_RANK and aggregate-OVER all work, so this is five missing functions, not a missing family. Making an unknown window function a hard error is worth doing independently of implementing them. P23 - LAG/LEAD honour the offset but drop the third (default) argument. P24 - a RANGE frame is treated as ROWS, including the implicit default frame when a window has ORDER BY and no explicit frame. Only detectable because the fixture has ties; on distinct keys ROWS and RANGE coincide. P25 - a window's ORDER BY accepts only a plain column, though the outer ORDER BY handles expressions fine. P26 - a window over an aggregate is rejected by the GROUP BY validity check. Same pipeline-position confusion as P21 from the other end. CLAUDE.md documents a CTE workaround for this, so it was known in practice but never filed. Two near-misses recorded in the comments because they show how the tier could have concluded the opposite: SUM-over-partition AGREES under the same WHERE that exposes P21 (the filtered rows carry NULLs that SUM ignores), and ROWS/RANGE coincide entirely without ties. Also CHECKED and clear: the batch window path (SQL_CLI_BATCH_WINDOW, on by default) agrees with its fallback across 12 queries, so these cases test one behaviour rather than two. 123 -> 150 cases; contract holds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Pins the reference engine in pyproject.toml [dependency-groups].test and points CI at that group instead of `uv pip install duckdb`, so the version lives in one place. Verified 1.5.5 first: all 150 cases stay in their existing buckets, zero drift, so this is the highest pin available without reworking anything already agreed. Why pin at all: the corpus now contains cases where the SQL standard leaves behaviour implementation-defined and we follow the reference engine by policy (P17 NULL ordering, P24 frame defaults). Those cases encode DuckDB's current choices, so an unpinned bump could redden the gate for reasons unrelated to our engine and point at the wrong place while doing it. Also qualifies that policy, which as written read too absolutely: DuckDB is a reference point, not a specification. The goal is to stop being accidentally different, not to reproduce DuckDB exactly — where a difference is a genuine DuckDB idiosyncrasy we can mark it WON'T FIX and move on. The rule saves us re-litigating ambiguous cases; it is not a commitment to chase quirks. And records where the effort is up to: discovery is paused. Two days took the corpus 83 -> 150 and the open findings 1 -> 15, several of which need a session apiece, so the effort moves to fixing. Adds a suggested fix order ranked by silent blast radius (P21 and P13 first), and notes that tier 10 is deliberately partial and wants finishing during a lull. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Discovery push against the DuckDB reference engine. No engine changes — this is corpus, docs and CI only, so the only job that can move is the parity gate.
Why now
The P-log was exhausted apart from P3, so new findings had to come from widening the corpus rather than working the backlog — which is how P9–P12 arrived in the first place. Tiers 08 (ordering & limiting), 09 (window functions & QUALIFY) and 10 (aggregate & NULL edges) were the uncovered surface.
Findings
Fourteen, ten of them silent — wrong answers with no error:
QUALIFYrejects an inline window functionORDER BY <ordinal>ignored — rows come back in insertion orderASC= NULLmatches NULL rows instead of yielding UNKNOWNNOT INdoesn't exclude NULLs, though<>does||treats NULL as empty string while+propagates itWHERELAG/LEADdrop the third (default) argumentRANGEframe treated asROWS, incl. the implicit default frameORDER BYaccepts only a plain columnGROUP BYcheckP21 is the widest. With a
WHEREpresent, windows see the unfiltered row set —COUNT(*) OVER (PARTITION BY team)reports pre-filter partition sizes. The identical query without theWHEREagrees with DuckDB exactly, which is what proves partitioning and the functions are correct and the defect is purely pipeline position.P13 turned out far broader than its symptom. It surfaced as
ORDER BY ... NULLS LAST LIMIT 3returning every row, but there is noNULLShandling insrc/sql/at all — the parser silently ignores everything after the first token it can't place.... ORDER BY amount DESC FROBNICATE LIMIT 3runs clean and drops theLIMIT. Any typo becomes a different query that succeeds.The fixture
data/null_edges.csv, 12 rows, purpose-built — every other corpus CSV is NULL-free, so NULL behaviour was unassertable. NULLs in a sort key, a partition key, a string column, a join key and an entirely-NULL column, plus deliberate ties and a unique never-NULLidas a total-order tiebreak.Verified before building on it that both engines see the same NULLs in all five nullable columns — otherwise these cases would test the CSV reader, not the engine.
Two harness properties shaped the design and are recorded in the tier headers:
normalize.pycanonicalises""to NULL so''-vs-NULL is invisible to the comparison, andhas_order_by()is a substring check soOVER (ORDER BY ...)alone forces ordered comparison — hence a total ordering on every case.Two near-misses worth reading
Both are in the tier comments, because they show how this could have concluded the opposite:
SUM(...) OVER (PARTITION BY ...)AGREEs under the sameWHEREthat exposes P21 — the filtered-out rows carry NULL scores thatSUMignores anyway.COUNT(*)is the discriminating probe; a SUM-only tier would have declared windows fine.ROWSandRANGEcoincide entirely without ties, so P24 was undetectable before this fixture existed.The general pattern: every finding got sharper from a control case.
FROBNICATEturned a clause gap into a parser-wide finding;<>working correctly is what makesNOT INa bug rather than a stance;qualify_select_list_aliasmoved P15's fix site to a different file entirely.Checked and clear
Window evaluation has two code paths (
SQL_CLI_BATCH_WINDOW, batch on by default). Ran 12 window queries through both — no mismatches, so these cases test one behaviour rather than two. Ruled out rather than assumed.Decisions and policy
P17 and P20 are both cases the SQL standard leaves implementation-defined. Both resolved as follow the reference engine, now stated once in the doc preamble instead of as two precedents — with the qualification that DuckDB is a reference point, not a specification: the goal is to stop being accidentally different, not to reproduce DuckDB, and genuine idiosyncrasies can be ⚪ WON'T FIX.
That makes DuckDB's version part of the contract, so it is now pinned at 1.5.5 in
pyproject.tomlwith CI resolving from that group. Verified zero bucket drift from 1.5.4 before pinning.What this does not do
No fixes. Every finding is annotated with
expect, so the parity gate stays green — that is what the contract is for. Discovery is now paused in favour of fixing;SQL_PARITY.mdopens with the fix order, ranked by silent blast radius rather than P-number, on the principle that a loud failure is cheaper than a quiet one.Tier 10 is deliberately partial — flagged in the doc so it isn't mistaken for complete.
STDDEV, DISTINCT aggregates,FILTERand empty-vs-all-NULL are unexamined.🤖 Generated with Claude Code