From 3c1a005eb5d8641c972f3267c3e4b8236bd4a25e Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sat, 18 Jul 2026 14:04:54 +0100 Subject: [PATCH] docs(parity): file P9-P12, found while surveying transformers for R2 The walker migration surfaced real wrong-result bugs, not just structural debt. Each was reproduced against the CLI and is now pinned by a corpus case, so they cannot regress further or be silently fixed. P9 -- HAVING silently mishandles an aggregate nested in a non-comparison operator. HavingAliasTransformer only matches FunctionCall/BinaryOp/Not, so an aggregate reached any other way is never rewritten to its alias. The predicate then does not filter as written, and the result is wrong in BOTH directions with no error raised: HAVING COUNT(*) BETWEEN 1 AND 2 ref 1 row -> ours 4 rows (under-filters) HAVING COUNT(*) IN (4, 5) ref 2 rows -> ours 0 rows (over-filters) HAVING CASE WHEN COUNT(*) > 2 ... ref 2 rows -> ours 0 rows HAVING COUNT(*) > 2 works, which is why nobody noticed. The code comment at having_alias_transformer.rs:213 says the untransformed aggregate "will fail later" -- it does not fail, it returns wrong rows. P10 -- HAVING NOT (COUNT(*) > 2) errors in the arithmetic evaluator. Distinct from P9: here the aggregate IS rewritten correctly and the gap is downstream, so fixing P9 will not fix this. P11 -- a SELECT alias on the LHS of an IN subquery is never expanded ("Column 'dbl' not found"). WhereAliasExpander skips the same-scope operand along with the subquery. It is right not to descend into the subquery body; the bug is dropping the operand too. P12 -- WITH is rejected in every expression position tried. Found while trying to write a regression test for the cte_hoister migration that turned out to be inexpressible. Side effect: the ScalarSubquery / InSubquery arms of CTEHoister::hoist_from_expression are unreachable dead code today. Adds corpus tier 07 (grouping). The corpus had NO HAVING coverage at all before now, which is the direct reason P9 survived -- worth treating an absent test bucket as a finding in its own right. One trap worth recording: the first draft of having_in_list used IN (2, 3), which no group satisfies, so both engines returned 0 rows and the case passed for the wrong reason. Corrected to IN (4, 5) against the real group counts (5/10/4/1). Corpus grows 72 -> 83 cases; contract holds. Cross-links P9/P11 to R3 in ENGINE_REFACTORING.md, which now carries the evidence table. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/ENGINE_REFACTORING.md | 31 ++++++--- docs/SQL_PARITY.md | 75 +++++++++++++++++++++ tests/comparison/corpus/02_where.toml | 9 +++ tests/comparison/corpus/06_ctes_setops.toml | 8 +++ tests/comparison/corpus/07_grouping.toml | 67 ++++++++++++++++++ 5 files changed, 181 insertions(+), 9 deletions(-) create mode 100644 tests/comparison/corpus/07_grouping.toml diff --git a/docs/ENGINE_REFACTORING.md b/docs/ENGINE_REFACTORING.md index 1f8d6dc9..c757b145 100644 --- a/docs/ENGINE_REFACTORING.md +++ b/docs/ENGINE_REFACTORING.md @@ -128,16 +128,27 @@ feature work**, and so we can tell the difference between "this is awkward" and - **Impact:** A new expression variant is *silently* dropped by the formatter, the lifters, and the aggregate detector. The failure mode is a wrong answer or a no-op, not a build error. -- **Confirmed live bugs from this pattern** (both pre-existing, neither yet fixed): - - `expression_lifter::extract_column_references` never handles `MethodCall`, - `ChainedMethodCall`, `Unnest`, `InSubquery`, or the tuple subquery variants. - - `having_alias_transformer::collect_aggregates_in_having` handles only - `FunctionCall`, `BinaryOp`, `Not` — **an aggregate inside a `CASE` in a - `HAVING` clause is never found.** +- **Confirmed live bugs from this pattern.** These are not hypothetical — each + was reproduced against the CLI and is now pinned by a corpus case: + + | Parity entry | Symptom | Severity | + |---|---|---| + | [P9](SQL_PARITY.md) | `HAVING` with an aggregate inside `BETWEEN` / `IN` / `CASE` returns wrong rows, **silently, in both directions** | **wrong results, no error** | + | [P11](SQL_PARITY.md) | A `SELECT` alias on the LHS of an `IN` subquery → `Column not found` | hard error | + | *(fixed, PR #33)* | `ILIKE` inside `OVER (ORDER BY ...)` left unrewritten, reaching the executor as an unknown operator | hard error | + | *(fixed, PR #33)* | `INTO` inside `(a, b) IN (SELECT ...)` never removed | reaches executor | + + P9 is the one that matters most: `HAVING COUNT(*) BETWEEN 1 AND 2` returned + 4 rows where DuckDB returns 1, with no error. A catch-all turned a missing + match arm into a wrong answer. + +- **The corpus had no `HAVING` coverage at all** before 2026-07-18, which is why + P9 survived. Absence of a test bucket is itself a finding: when migrating a + transformer, check whether the clause it serves is represented in + `tests/comparison/corpus/`. - **Decision:** Fix by attrition through the R2 migration; each transformer that - moves onto `walk` loses its catch-all. Worth a follow-up to confirm the two - bugs above with failing tests before their transformers migrate, so the fix is - demonstrated rather than assumed. + moves onto `walk` loses its catch-all. Write the failing corpus case **before** + migrating the transformer, so the fix is demonstrated rather than assumed. ### R4 — Transformer test fixtures use ASTs the parser never produces - **Status:** 🔴 OPEN @@ -234,3 +245,5 @@ R5 dead code ─────── opportunistic |---|---|---| | 2026-07-18 | R1 partial: FROM sync helpers; four desync sites fixed; dead lifter deleted | #30 | | 2026-07-18 | R2 partial: `walk.rs` traversal helpers landed (additive) | #31 | +| 2026-07-18 | R2 group 1: the three boundary-crossing transformers migrated; two silent bugs fixed | #33 | +| 2026-07-18 | R3 evidence: P9–P12 filed after probing the engine; corpus gains tier 07 (grouping) | — | diff --git a/docs/SQL_PARITY.md b/docs/SQL_PARITY.md index b1e62231..dfefca3d 100644 --- a/docs/SQL_PARITY.md +++ b/docs/SQL_PARITY.md @@ -212,6 +212,81 @@ annotation be removed. land under the correct aliases and NULLs fall on the FROM side, in plain `cargo test` (independent of the DuckDB corpus). +### P9 — `HAVING` silently mishandles an aggregate nested in a non-comparison operator +- **Status:** 🔴 OPEN — **theme**, covers three corpus cases +- **Corpus:** `07_grouping.toml :: having_between` (DIFFER), `having_in_list` + (DIFFER), `having_case` (DIFFER). `having_comparison` / + `having_sum_comparison` AGREE. +- **Observed:** `HavingAliasTransformer` rewrites an aggregate in `HAVING` to + reference its computed alias, but its traversal handles only `FunctionCall`, + `BinaryOp` and `Not`. An aggregate reached through any other operator is never + rewritten, and the result is **silently wrong in both directions**: + + | Query | Correct | sql-cli | + |---|---|---| + | `HAVING COUNT(*) BETWEEN 1 AND 2` | 1 row | **4 rows** (under-filters) | + | `HAVING COUNT(*) IN (4, 5)` | 2 rows | **0 rows** (over-filters) | + | `HAVING CASE WHEN COUNT(*) > 2 THEN 1 ELSE 0 END = 1` | 2 rows | **0 rows** | + + No error is raised in any of these — the predicate simply doesn't do what it + says. `HAVING COUNT(*) > 2` works, which is why this went unnoticed. +- **Found:** 2026-07-18, while surveying transformers for the + [R2](ENGINE_REFACTORING.md) walker migration. **Not found by testing** — the + corpus had no `HAVING` coverage at all before this entry. +- **Root cause:** [R3](ENGINE_REFACTORING.md) — hand-rolled traversals ending in + a `_ => {}` catch-all. `collect_aggregates_in_having` and + `rewrite_having_expression` each miss 14 of the 24 expression variants. The + code comment at `having_alias_transformer.rs:213` acknowledges the untransformed + aggregate "will fail later"; in practice it does not fail, it returns wrong rows. +- **Decision:** **Fix** via the R2 migration — moving both functions onto + `walk::visit_children` / `map_children` retires the catch-all and covers every + variant. Constraint: the aggregate arm must *not* delegate to the walker, or it + would start recursing into aggregate arguments and break the deliberate + "no nested aggregates" invariant. + +### P10 — `HAVING NOT (...)` errors in the evaluator +- **Status:** 🔴 OPEN +- **Corpus:** `07_grouping.toml :: having_not` (GAP) +- **Observed:** `HAVING NOT (COUNT(*) > 2)` → + `Unsupported expression type for arithmetic evaluation: Not { ... }`. +- **Distinct from P9:** here the aggregate *is* rewritten correctly (the + transformer does handle `Not`), and the failure is downstream — the arithmetic + evaluator has no `Not` arm for a post-aggregation predicate. Fixing P9 will not + fix this. +- **Decision:** **Fix** — add the missing evaluator arm. Small and independent. + +### P11 — A `SELECT` alias is not expanded on the LHS of an `IN` subquery +- **Status:** 🔴 OPEN +- **Corpus:** `02_where.toml :: select_alias_in_in_subquery` (GAP) +- **Observed:** `SELECT symbol, price * 2 AS dbl FROM trades WHERE dbl IN + (SELECT ...)` → `Column 'dbl' not found`. The same alias resolves fine as the + LHS of a plain comparison or an `IN`-list, so this is specific to the subquery + form. +- **Root cause:** [R3](ENGINE_REFACTORING.md). `WhereAliasExpander` never matches + `InSubquery` / `NotInSubquery` / the tuple forms, so the **same-scope** LHS + operand is skipped along with the subquery. Note the expander is right not to + descend into the subquery *body* (different scope) — the bug is that it drops + the operand too. +- **Decision:** **Fix** via the R2 migration. `walk` visits `InSubquery`'s `expr` + while still treating the nested statement as a scope boundary, which is exactly + the required behaviour. + +### P12 — `WITH` is rejected in expression position +- **Status:** 🔴 OPEN +- **Corpus:** `06_ctes_setops.toml :: cte_in_expression_position` (GAP) +- **Observed:** `WHERE price > (WITH avg_cte AS (...) SELECT a FROM avg_cte)` → + `Parse error: Unexpected token in primary expression: With`. Rejected in every + expression position tried — scalar subquery, `BETWEEN` operand, `IN`-list + element, and tuple `IN` (which reports "Tuple IN requires a subquery on the + right"). DuckDB accepts a CTE inside a scalar subquery. +- **Found:** 2026-07-18, while trying to write a regression test for the + `cte_hoister` walker migration — the test could not be expressed. +- **Side effect worth noting:** the `ScalarSubquery` / `InSubquery` arms of + `CTEHoister::hoist_from_expression` are therefore **unreachable dead code** + today; expression-position CTE hoisting has never had an input. +- **Decision:** **Fix** — a parser change (accept `WITH` where a subquery is + already accepted). The hoister machinery to handle the result already exists. + --- ## Deferred / won't fix (intentional) diff --git a/tests/comparison/corpus/02_where.toml b/tests/comparison/corpus/02_where.toml index 4dab4769..afe9ae4f 100644 --- a/tests/comparison/corpus/02_where.toml +++ b/tests/comparison/corpus/02_where.toml @@ -69,3 +69,12 @@ sql = "SELECT region, amount FROM international_sales WHERE amount > (SELECT AVG id = "where_in_subquery_set" data = "trades.csv" sql = "SELECT symbol FROM trades WHERE symbol IN (SELECT symbol FROM trades WHERE price > 185)" + +[[case]] +id = "select_alias_in_in_subquery" +data = "trades.csv" +sql = "SELECT symbol, price * 2 AS dbl FROM trades WHERE dbl IN (SELECT price * 2 FROM trades WHERE price > 185)" +# P11: a SELECT alias used as the LHS of an IN-subquery is never expanded, so +# the query errors with "Column 'dbl' not found". The same alias works fine as +# the LHS of a plain comparison or an IN-list. +expect = "GAP" diff --git a/tests/comparison/corpus/06_ctes_setops.toml b/tests/comparison/corpus/06_ctes_setops.toml index 1a8412c4..e4cbc61a 100644 --- a/tests/comparison/corpus/06_ctes_setops.toml +++ b/tests/comparison/corpus/06_ctes_setops.toml @@ -45,3 +45,11 @@ data = "trades.csv" sql = "WITH RECURSIVE n(i) AS (SELECT 1 UNION ALL SELECT i + 1 FROM n WHERE i < 5) SELECT i FROM n" # Recursive CTEs are a known, considered, deferred design item (not supported). expect = "GAP" + +[[case]] +id = "cte_in_expression_position" +data = "trades.csv" +sql = "SELECT symbol, price FROM trades WHERE price > (WITH avg_cte AS (SELECT AVG(price) AS a FROM trades) SELECT a FROM avg_cte)" +# P12: WITH is rejected anywhere in expression position ("Unexpected token in +# primary expression: With"). DuckDB allows a CTE inside a scalar subquery. +expect = "GAP" diff --git a/tests/comparison/corpus/07_grouping.toml b/tests/comparison/corpus/07_grouping.toml new file mode 100644 index 00000000..ff52f1b7 --- /dev/null +++ b/tests/comparison/corpus/07_grouping.toml @@ -0,0 +1,67 @@ +# Tier 7 — GROUP BY and HAVING. +# +# Added 2026-07-18. Until now the corpus had no HAVING coverage at all, which +# is why the P9 family below went unnoticed: aggregates reached by anything +# other than a bare comparison are silently mishandled. See SQL_PARITY.md. + +[[case]] +id = "group_by_count" +data = "international_sales.csv" +sql = "SELECT region, COUNT(*) AS n FROM international_sales GROUP BY region ORDER BY region" + +[[case]] +id = "group_by_sum" +data = "international_sales.csv" +sql = "SELECT region, SUM(amount) AS total FROM international_sales GROUP BY region ORDER BY region" + +[[case]] +id = "group_by_multi_key" +data = "international_sales.csv" +sql = "SELECT region, product, COUNT(*) AS n FROM international_sales GROUP BY region, product ORDER BY region, product" + +# --- HAVING: the baseline that works --- + +[[case]] +id = "having_comparison" +data = "international_sales.csv" +sql = "SELECT region, COUNT(*) AS n FROM international_sales GROUP BY region HAVING COUNT(*) > 2 ORDER BY region" + +[[case]] +id = "having_sum_comparison" +data = "international_sales.csv" +sql = "SELECT region, SUM(amount) AS total FROM international_sales GROUP BY region HAVING SUM(amount) > 5000 ORDER BY region" + +# --- HAVING: P9 family — aggregate nested in a non-comparison operator --- + +[[case]] +id = "having_between" +data = "international_sales.csv" +sql = "SELECT region, COUNT(*) AS n FROM international_sales GROUP BY region HAVING COUNT(*) BETWEEN 1 AND 2 ORDER BY region" +# P9: aggregate inside BETWEEN is never rewritten to its alias, so the predicate +# does not filter — silently UNDER-filters and returns groups it should exclude. +expect = "DIFFER" + +[[case]] +id = "having_in_list" +data = "international_sales.csv" +sql = "SELECT region, COUNT(*) AS n FROM international_sales GROUP BY region HAVING COUNT(*) IN (4, 5) ORDER BY region" +# P9: aggregate inside IN is never rewritten — silently returns 0 rows. +# NB values must MATCH real group counts (5/10/4/1). An earlier draft used +# IN (2, 3), which no group satisfies, so both engines returned 0 rows and the +# case passed for the wrong reason. +expect = "DIFFER" + +[[case]] +id = "having_case" +data = "international_sales.csv" +sql = "SELECT region, COUNT(*) AS n FROM international_sales GROUP BY region HAVING CASE WHEN COUNT(*) > 2 THEN 1 ELSE 0 END = 1 ORDER BY region" +# P9: aggregate inside CASE is never rewritten — silently OVER-filters to 0 rows. +expect = "DIFFER" + +[[case]] +id = "having_not" +data = "international_sales.csv" +sql = "SELECT region, COUNT(*) AS n FROM international_sales GROUP BY region HAVING NOT (COUNT(*) > 2) ORDER BY region" +# P10: the aggregate IS rewritten here, but the evaluator has no arm for Not, +# so this errors rather than returning wrong rows. +expect = "GAP"