Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 22 additions & 9 deletions docs/ENGINE_REFACTORING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) | — |
75 changes: 75 additions & 0 deletions docs/SQL_PARITY.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
9 changes: 9 additions & 0 deletions tests/comparison/corpus/02_where.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
8 changes: 8 additions & 0 deletions tests/comparison/corpus/06_ctes_setops.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
67 changes: 67 additions & 0 deletions tests/comparison/corpus/07_grouping.toml
Original file line number Diff line number Diff line change
@@ -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"
Loading