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"