Skip to content
Open
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
71 changes: 70 additions & 1 deletion docs/ENGINE_REFACTORING.md
Original file line number Diff line number Diff line change
Expand Up @@ -789,6 +789,73 @@ feature work**, and so we can tell the difference between "this is awkward" and
(`price > quantity`, `price + 0 > quantity` over `trades_100000.csv`, inline
QUALIFY) are the baseline.

### R14 — A column reference is resolved by several hand-written copies
- **Status:** 🔴 OPEN — filed 2026-10-01 by [P58](SQL_PARITY.md#p58). **Not
to start before R13 is finished** (slice 4 tail, slice 6); slice 1 below is
a no-op pin and can go any time.
- **Observed:** `ExecutionContext::resolve_column_index` (`query_engine.rs:109`)
describes itself as *the* resolver "used by all SQL clauses". Probing
`alias."dotted.name"` (P58) found at least five places that resolve a column
reference, each written separately:

| Site | Alias map | Qualified then bare | How a dotted name is treated | Error |
|---|---|---|---|---|
| `ExecutionContext::resolve_column_index` (`query_engine.rs:109`) | yes | yes ([P49](SQL_PARITY.md#p49) fallback) | literal name first, then as qualified | `qualified_column_not_found` |
| `ArithmeticEvaluator`, column arm (`arithmetic_evaluator.rs` ~210) | its own lookup | yes | — | same helper |
| `ArithmeticEvaluator::evaluate_column` (unqualified) | — | — | **splits at the last dot first**, literal second | own "did you mean" |
| `resolve_select_columns` (`query_engine.rs` ~2966) | **none** — passes the raw prefix as "resolved" | yes, case-insensitive bare match | — | same helper, but with no aliases it says *Unknown table or alias 'l'* for `SELECT l.nosuch FROM countries l`, where `l` plainly is in scope |
| ORDER BY inline copy — [R11](#r11) | ignores `table_prefix` | — | literal first since [P34](SQL_PARITY.md#p34) | own |
| `hash_join::find_column_index` (`hash_join.rs:443`) | **ignores the prefix** | — | **strips at the last dot** — breaks `ON a."name.common" = …` ([P58](SQL_PARITY.md#p58)) | own; own case-insensitivity |
| GROUP BY keys (`group_by_expressions.rs:96`) | via the evaluator | via the evaluator | via the evaluator | **swallowed to NULL** ([P60](SQL_PARITY.md#p60)) |

Not yet examined: `window_context.rs` (`resolve_sort_columns`, 28
`get_column_index` calls), `subquery_executor.rs`, `data_view.rs`.
- **The upstream half — `ColumnRef` gets flattened back into text.** The
parser produces a structured `ColumnRef { table_prefix, name, quote_style }`,
but several consumers rebuild `format!("{prefix}.{name}")` and re-split it:
hash join (`extract_simple_column_name`), the SELECT-list path, and
`parse_identifier_list` in the parser itself (CTE column lists, window
`PARTITION BY`). Once flattened, `l` + `name.common` and `l.name` + `common`
are the same string, and every re-split has to guess. P34 and P58 are both
that guess going wrong. The parser has three separate `ident . ident` readers
too (`parse_primary`, `parse_identifier_list`, `parse_column_reference`) —
P58 fixed only the first.
- **Impact:** every resolution rule — alias handling, quoting, case
sensitivity, the P49 strictness decision, the error wording — has to be
changed in N places, and the history says it isn't: P34 fixed ORDER BY's copy,
P46 made WHERE use the shared one, `column_resolution_error` unified the
*message* at three sites; the join copy got none of those. It is also what
[P3](SQL_PARITY.md#p3)'s scope spine would have to thread through: correlated
resolution needs one place to ask "which scope owns this name?".
- **Same family as [R11](#r11) and [R13](#r13).** R13 gives an expression one
*meaning*; R14 gives a name one *binding*. R11 becomes a slice of this entry.
- **Target shape:** one function, `resolve(&ColumnRef, &Scope) -> Result<usize>`,
where `Scope` carries the table, its aliases and the case mode. It takes the
structured reference, never a dotted string; no consumer formats
`prefix.name`. The error is built from the scope, so it knows the aliases.
- **Slices, in the R13 pattern:**
1. **Pin, no engine change.** A clause × reference-shape matrix —
SELECT, WHERE, GROUP BY, HAVING, ORDER BY, JOIN ON, PARTITION BY ×
`col`, `t.col`, `"dotted.col"`, `t."dotted.col"`, `"t"."col"`, unknown
column, unknown prefix — with DuckDB-generated expectations, over
`countries.csv` and a two-table join. Records today's divergences
(P58 join, P59, P60, the misleading message) as a KNOWN list, as
`tests/evaluator_matrix_tests.rs` did.
2. **Stop flattening.** Hash join and the SELECT-list path take the
`ColumnRef`; `parse_identifier_list` returns `ColumnRef`s. Closes P58's
join half.
3. **Converge the copies** onto `resolve_column_index` (renamed if `Scope`
lands), one site per commit: evaluator, SELECT list, hash join, then
ORDER BY (R11 — note its recorded behaviour change for unquoted dotted
names).
4. **Decide strictness once** — [P49](SQL_PARITY.md#p49)'s unknown-prefix
fallback, and errors not swallowed (P60) — now that it is one place.
- **Explicitly not in scope:** correlated outer references
([P3](SQL_PARITY.md#p3) / [R7](#r7)) — but R14 is the groundwork that makes
them a scope lookup rather than another copy.
- **Found:** 2026-10-01. P58's one-branch parser fix worked in every clause
but JOIN ON, and the reason was a resolver nobody knew was separate.

---

## Sequencing
Expand All @@ -811,9 +878,10 @@ R4 fixtures ──────── adopt opportunistically, per transformer to
R5 dead code ─────── opportunistic
R8 legacy WHERE ──── independent; stage 2 is self-contained, do it in a lull
R10 Trilean ──────── DONE; closed P18/P19 (parity 125 → 129)
R11 ORDER BY resolver ─ independent; small, but a behaviour change — wants its own parity run
R11 ORDER BY resolver ─ independent; small, but a behaviour change — wants its own parity run; now slice 3 of R14
R12 aggregate registries ─ independent; step 1 is a provable no-op, do it before the next aggregate fix
R13 one evaluator ─── ACTIVE from 2026-09-13; slices 1 (pin), 2 (3VL), 3 (construction + case mode) DONE; 4 operators DONE (BETWEEN/IN, comparisons, IS NULL, LIKE); 5 (method calls) DONE → 4 tail (AND/OR/NOT/CASE) → 6
R14 one column resolver ─ after R13; slice 1 (pin) any time → 2 (stop flattening ColumnRef; P58 join) → 3 (converge copies, absorbs R11) → 4 (P49/P60 strictness) ──→ feeds P3 scope spine
```

**A note on ordering, from the P18/P19 work being next.** The WHERE evaluator
Expand Down Expand Up @@ -858,3 +926,4 @@ AGREE count — which makes it safe to land well before the semantics change.
| 2026-09-18 | R13 slice 4 (second part): WHERE comparisons delegate. Operand readers pinned first (third matrix); P53 and P54 filed; value evaluator's `DATETIME()` moved to local time before delegating. Closes P54 — 175 → **176 AGREE**. Part 1 found to have fixed P54 for IN/BETWEEN unnoticed | — |
| 2026-09-19 | R13 slice 4 (third part): IS NULL and LIKE delegate; one `sql_like` for both evaluators. P55 (LIKE compiled as an unescaped regex) filed and closed — 176 → **179 AGREE**; the value evaluator's exponential LIKE matcher replaced first; D2 (LIKE over a number matches its displayed text) decided; `EvaluationContext` removed | — |
| 2026-09-26 | R13 slice 5: method calls. Pinned 55 divergences across both evaluators; P56 (NULL receiver → FALSE) and P57 (WHERE refused all but five methods on a comparison's left) filed and closed; D3 decided (search methods follow `--case-insensitive`). Registry search functions fixed first (180), then WHERE delegated and ~700 lines of its arms and readers deleted — 179 → **183 AGREE** / 213 | — |
| 2026-10-01 | R14 filed from P58 (`alias."quoted col"`): parser fix landed on a branch and worked in every clause but JOIN ON, whose resolver strips at the last dot. Five resolver copies inventoried; P59 (quoted alias) and P60 (GROUP BY unknown column → NULL group) filed | — |
77 changes: 77 additions & 0 deletions docs/SQL_PARITY.md
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ Suggested fix order, by silent blast radius:
| ~~9e~~ | ~~[P41](#p41) `MODE` tie-break is random per run~~ | ✅ **Fixed 2026-09-06** — 152 → **156 AGREE** (four new cases). Small, as predicted, but not where it was filed: the named `ModeState` was a *shadowed* implementation and fixing it moved nothing. Reference does specify a rule and it is **first-occurrence**, not the "smallest value wins" this row proposed. Unblocked both example files, now FORMAL. Spun off [P42](#p42), [P43](#p43), [R12](ENGINE_REFACTORING.md#r12) |
| ~~9g~~ | ~~[P46](#p46) column-vs-column `WHERE` returns 0 rows~~ | ✅ **Fixed 2026-09-13** — 157 → **168 AGREE**. Wider than filed: *every* non-literal right-hand operand read as NULL (comparisons, `IN` items, `BETWEEN` bounds), and a literal on the left errored (`WHERE 1=0`). One resolver for all operands closed it. Did **not** need [P48](#p48) alongside, as this row predicted — the fix kept the comparison in the WHERE evaluator. Spun off [P49](#p49) (correlated outer refs bind to the inner table), accepted knowingly |
| **NEXT** | [R13](ENGINE_REFACTORING.md#r13) one expression evaluator — **the active workstream from 2026-09-13** | **Slices 1–2 done 2026-09-13** (value evaluator three-valued, 168 → 174 AGREE, closed P48/P50); **slice 3 done 2026-09-14** (one construction path; case-insensitive mode now honoured outside WHERE, parity unmoved); **slice 4 in progress** (WHERE arms delegate, one operator per commit: BETWEEN/IN 2026-09-15, comparisons 2026-09-18 closing [P54](#p54), IS NULL and LIKE 2026-09-19 closing [P55](#p55), 175 → 179 AGREE; every WHERE operator now delegates — AND/OR/NOT/CASE follow slice 5). Not a finding but the reason several are cheap to fix only once. WHERE and the value evaluator implement every boolean operator separately with different NULL rules, so the same predicate answers differently in WHERE, HAVING, SELECT and JOIN ON. **Working rule:** a parity fix that touches expression evaluation lands as an R13 slice, not a patch. [P52](#p52) (NULL join keys) is slice 6 |
| then | [P60](#p60) `GROUP BY` unknown column → NULL group; [P58](#p58) JOIN ON half | Filed 2026-10-01. P60 is silent and a one-line cause, so cheap; P58's join half is a resolver fix — take it as the first slice of [R14](ENGINE_REFACTORING.md#r14) rather than a patch. [P59](#p59) (quoted alias) rides along if convenient |
| then | [P47](#p47) `MIN`/`MAX` ranked by type | Silent and cheap, and outside the evaluator (aggregate registries — confirm the live one first, [R12](ENGINE_REFACTORING.md#r12)), so it can land alongside R13 without breaking the working rule |
| then | [P14](#p14), [P20](#p20), [P23](#p23) | Smaller, self-contained, decisions already taken. Was row 9b, then NEXT until the 2026-09-12 findings displaced it. **Check each against the R13 working rule first** — P20 (`\|\|` with NULL) is an operator in the value evaluator |
| 9f | [P42](#p42) `MODE` is numeric-only | Companion to [R12](ENGINE_REFACTORING.md#r12), and cheap if taken with it: the shadowed implementation already handles non-numerics and preserves type, so the fix is largely to stop the live path throwing away what it knows. Also buys the corpus its clearest tie-break case |
Expand Down Expand Up @@ -2579,6 +2580,82 @@ out of date.
`IndexOf`, `Trim`, `TrimStart` and `TrimEnd` and rejects everything else.
- **Found:** 2026-09-26, pinning method calls for R13 slice 5.

### P58 — A quoted column cannot follow an alias qualifier (`l."name.official"`)
- **Status:** 🟡 PARTIAL — expression positions fixed 2026-10-01 on branch
`fix/qualified-quoted-columns` (unmerged); **JOIN ON keys still fail**.
- **Corpus:** none yet. Add to `01_select.toml` (SELECT / WHERE forms) and
`04_joins.toml` (the ON form) with the fix — DuckDB 1.5.5 answers every form
below, checked by hand against `data/countries.csv`.
- **Observed:** `countries.csv` flattens its JSON into columns such as
`name.official`, which must be quoted. `SELECT "name.official" FROM countries`
works, but qualifying it — unavoidable in a self-join, which is where it came
from — failed to parse: `SELECT l."name.official" FROM countries l` →
*Expected identifier after '.'*.
- **Cause:** `parse_primary` (`src/sql/parser/expressions/primary.rs`) accepted
only a bare `Token::Identifier` after the dot.
- **Fixed so far:** that branch now also accepts `Token::QuotedIdentifier` and
builds a quoted `ColumnRef` carrying the prefix. Nothing downstream needed
changing for SELECT, WHERE, GROUP BY, ORDER BY or a CTE — verified with each.
- **Still failing — JOIN ON.** `... JOIN countries b ON a."name.common" =
b."name.common"` → *Column 'a.name.common' not found in either table*. The
join keeps a structured `ColumnRef`, but `hash_join::extract_simple_column_name`
flattens it back to the string `a.name.common` and `find_column_index` splits
that at the **last** dot, looking for a column called `common`. The same
shape [P34](#p34) fixed in ORDER BY, in a resolver copy P34 did not reach —
see [R14](ENGINE_REFACTORING.md#r14). Fix it there (pass the `ColumnRef`
through, not a string), not by special-casing quotes in the join.
- **Related:** [P59](#p59) (quoted *alias*), [P34](#p34), [R11](ENGINE_REFACTORING.md#r11),
[R14](ENGINE_REFACTORING.md#r14), [T19](TUI_FEATURES.md#t19) (completing the same shape).
- **Found:** 2026-10-01, field use — a self-join over `countries.csv` to list
each country's neighbours.

### P59 — A quoted table alias is not accepted (`"l".cca3`, `FROM t "l"`)
- **Status:** 🔴 OPEN — hard error
- **Corpus:** none yet; add to `01_select.toml` with the fix.
- **Observed:** DuckDB accepts the qualifier quoted as well as the column, and
a quoted alias in FROM. We fail each, in a different place:

| Query | Ours | DuckDB |
|---|---|---|
| `SELECT "l"."name.official" FROM countries l` | parse error: *Expected identifier after '.'* | `Aruba` |
| `SELECT "l".cca3 FROM countries l` | *Column '"l".cca3' not found* — parses, then fails to resolve | `ABW` |
| `SELECT l."cca3" FROM countries "l"` | parse error: *Unexpected 'l' after end of statement* | `ABW` |

- **Cause (to confirm):** the `Token::QuotedIdentifier` arm of `parse_primary`
never looks for a following `Dot`; the second row suggests a different path
(the SELECT-list fast path?) builds the name as text. FROM's alias parsing
accepts only a bare identifier.
- **Why it matters:** low on its own — few people quote an alias. But tools
that generate SQL (and DuckDB's own `EXPLAIN` output) quote everything, and
the second row is a silent mis-parse rather than a refusal.
- **Related:** [P58](#p58).
- **Found:** 2026-10-01, probing the P58 fix against DuckDB.

### P60 — `GROUP BY` an unknown column forms one NULL group instead of failing
- **Status:** 🔴 OPEN — **silent**
- **Corpus:** none yet; add to `07_grouping.toml` — DuckDB is an error, so the
case is `BOTH_ERR` once fixed, and `OURS_ONLY` today.
- **Observed:** any name that resolves to nothing — bare, qualified or quoted —
is grouped as NULL, and every row lands in that one group:

| Query | Ours | DuckDB |
|---|---|---|
| `SELECT COUNT(*) AS n FROM countries GROUP BY nosuch` | `nosuch=NULL, n=250` | *Referenced column "nosuch" not found* |
| `... FROM countries l GROUP BY l."no such"` | `no such=NULL, n=250` | *Table "l" does not have a column named "no such"* |

The same name in SELECT or ORDER BY is an error, so it is GROUP BY alone.
A one-character typo in a grouping key returns a plausible-looking total.
- **Cause:** `group_by_expressions.rs:96` evaluates each key with
`.unwrap_or(DataValue::Null)` — every evaluation error, not only an unknown
column, becomes NULL. Lines ~291 and ~309 have the same shape and want
checking with it.
- **Decision:** propagate the error. Check first whether anything relies on the
swallow (a key that errors on some rows but not others — e.g. a cast — would
now fail the query, which is DuckDB's behaviour too).
- **Related:** [R14](ENGINE_REFACTORING.md#r14) — the sweep there should look for
other `unwrap_or(Null)` around column resolution.
- **Found:** 2026-10-01, while testing P58 with a column name that does not exist.

---

## Deferred / won't fix (intentional)
Expand Down
43 changes: 42 additions & 1 deletion docs/TUI_FEATURES.md
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,9 @@ acting as an offer policy, so a column with 250 values offered nothing even
with a prefix typed. That is the shape to watch for — a limit applied at a
layer that cannot see the question being asked.

**Recommended order from here: T14 → T10**, then T5. **T7** (deletion) and **T12**
**Recommended order from here: T14 → T10**, then T5. **T19** (filed
2026-10-01 — `alias.<tab>` completes nothing) is item 1 of the list below
failing for every aliased query, so it competes with T14 for first. **T7** (deletion) and **T12**
(retire the `ParseState` fallback) stay droppable anywhere.

### What this editor is for (2026-09-12)
Expand Down Expand Up @@ -774,6 +776,45 @@ Older, non-living notes that still contain usable thinking:
- **Loose end:** the pin prefix (`📌 `) is still not budgeted, so pinned
headers can lose their last characters.

### T19 — Completion does not know table aliases
- **Status:** 🔴 OPEN — filed 2026-10-01 alongside [P58](SQL_PARITY.md#p58)
- **Where:** `src/sql/cursor_context.rs` (no notion of FROM-clause aliases),
`src/sql/cursor_aware_parser.rs` (`AfterColumn` → method suggestions)
- **Observed:** measured through `CursorAwareParser::get_completions` +
`apply_completion_to_text` against `data/countries.csv`, the same harness
as `tests/completion_schema.rs`:

| Typed (rest of query: `… from countries l`) | Context | Result |
|---|---|---|
| `select name.offi<tab>` (no alias) | `DottedColumn` | `"name.official"` ✓ |
| `select l.<tab>` | `AfterColumn` | `l.Contains('')` — `l` taken for a column, string methods offered |
| `select l.offi<tab>` | `AfterColumn` | nothing |
| `select l.name.offi<tab>` | `AfterColumn` | nothing |
| `select l."name.o<tab>` | `DottedColumn` | `l."name.official"` ✓ — **by accident**: the open-quote token shape never looks behind the quote |
| `select countries.offi<tab>` (table name, no alias) | `AfterColumn` | nothing |
| `… where l.reg<tab>` | `AfterColumn` | nothing |

So every query that aliases its table — and every self-join, where
aliasing is compulsory — loses column completion after `alias.`.
- **Wanted:** `l.offi<tab>` → `l."name.official"` (the quoting P58 now makes
legal), `l.<tab>` → the table's columns rather than methods, and the
same for `countries.` when the table name itself is the qualifier. Matching
should be a substring of the dotted name, as `name.offi` already is, so
`offi` reaches `name.official`.
- **What it needs:** the cursor analyzer already tokenizes the whole text up to
the cursor (T9); it does *not* look past the cursor, and the FROM clause
usually is past it (`select l.<tab> from countries l`). So: tokenize the
full statement once, collect `FROM`/`JOIN` table + alias pairs, and when the
token before the `Dot` names one, treat the text after the dot as a column
partial in that table's schema (T2). With only one table loaded the alias
map is trivial, but resolving it properly is what lets a join of two CTEs
complete each side's columns later.
- **Ordering vs. `"l"."col"`:** [P59](SQL_PARITY.md#p59) (quoted alias) is not
accepted by the engine yet, so the completer should emit `l."col"`, never
`"l"."col"`, until it is.
- **Tests:** add the table above to `tests/completion_schema.rs` as pinned
rows; `select l."name.o` is the one that must not regress.

### T14 — The registry knows every signature and the editor never shows one
- **Status:** 🔴 OPEN — opened 2026-09-12; the ergonomic half of T10
- **Where:** `src/sql/cursor_context.rs` (a new context), the status line in
Expand Down
Loading