From 7e4bc122be9dabfa03ee2b9b315214d2300ac131 Mon Sep 17 00:00:00 2001 From: "claude@clouddev1" Date: Fri, 12 Jun 2026 14:03:00 +0000 Subject: [PATCH] fix(completion): treat a bare in-scope table alias as an alias, not an unknown column (#31) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A bare table alias typed where a column is expected — `… GROUP BY o`, with `o` aliasing `FROM Orders o` — was a blind spot: completion offered nothing for `o`, and the hint panel called the in-scope alias an unknown column (`no such column o on table Orders, ...`). Completion now offers each FROM source's qualifier (alias-if-present-else table-name) at a bare sql_expr_ident slot, folded into the column candidate list; on an exact-qualifier partial the alias source steps aside so the diagnostic can surface. The bare-reference diagnostic arm emits a targeted `alias_used_as_column` / `table_used_as_column` hint ("`o` is a table alias — write `o.` ...") after the projection-alias check, so ORDER-BY alias refs still win and a genuine unknown column still reports `unknown_column`. Two guards keep the qualified-form advice correct: SQL only (role `sql_expr_ident`, so the DSL `expr_column` path keeps `unknown_column` since the DSL has no `table.column` syntax) and effective-qualifier match (alias-if-present-else-table, so an aliased source referenced by its shadowed real name falls through rather than being advised as `name.`). The diagnostic is a drop-in replacement for `unknown_column` at the same span/Error severity, so verdict/overlay/hint paths are unchanged. ADR-0032 Amendment 3; +10 tests. --- docs/adr/0032-sql-select-grammar.md | 70 ++++++++++++ docs/adr/README.md | 2 +- src/completion.rs | 110 ++++++++++++++++++ src/dsl/walker/mod.rs | 169 ++++++++++++++++++++++++++++ src/friendly/keys.rs | 2 + src/friendly/strings/en-US.yaml | 6 + src/input_render.rs | 59 ++++++++++ 7 files changed, 417 insertions(+), 1 deletion(-) diff --git a/docs/adr/0032-sql-select-grammar.md b/docs/adr/0032-sql-select-grammar.md index 35930a7..21bfe43 100644 --- a/docs/adr/0032-sql-select-grammar.md +++ b/docs/adr/0032-sql-select-grammar.md @@ -1480,6 +1480,76 @@ accumulators), the per-keystroke re-walk (ADR-0027's debounced cadence), and the ORDER BY no-fixup-needed clarification. +## Amendment 3 — bare table aliases in expression slots (2026-06-12) + +Issue #31. A bare in-scope table alias typed where the grammar +expects a column — `… GROUP BY o`, with `o` aliasing +`FROM Orders o` — was a blind spot in two surfaces: + +- **Completion (§10).** §10.5 narrows columns *past* a + `qualifier .`, but the bare-ident slot before the dot offered + only columns and function names, never the aliases themselves. + A learner mid-typing `o` toward `o.` got no Tab help. +- **Diagnostics (§11.2).** §11.2 added `projection_alias_misplaced` + for a *projection* alias used in a forbidden clause, but a bare + *table* alias fell through to the generic `unknown_column` + bare-reference check (§11.2's `matched.len() == 0` arm), which + reported `no such column \`o\` on table \`Orders, …\`` — calling + an in-scope alias an unknown column. + +### What changes + +1. **Completion offers in-scope FROM qualifiers at a bare + `sql_expr_ident` slot** (one not already past a `qualifier .`). + Each binding contributes its *qualifier* — the alias if it has + one, else the table name (an aliased source must be referenced + by its alias). Folded into the existing `IdentSource::Columns` + candidate list so it sorts / dedups / colours uniformly. When + the partial *exactly* matches an in-scope qualifier the alias + source steps aside: discoverability is already served, and + suppressing sibling aliases lets the diagnostic below surface + (rather than being hidden by the `typing_over_diag` path). + +2. **A bare ident matching an in-scope qualifier now emits a + targeted diagnostic** instead of `unknown_column`, checked in + the `matched.len() == 0` arm *after* the projection-alias check + (so an ORDER-BY projection-alias reference still wins). It is a + drop-in replacement at the same span and `Error` severity — only + the message text changes — so the validity verdict, token + overlay, and hint-panel paths behave exactly as they did for + `unknown_column`: + - `diagnostic.alias_used_as_column` — `` `o` is a table alias — + write `o.` to reference one of its columns `` (the + binding has an alias), or + - `diagnostic.table_used_as_column` — same shape, "is a table" + (an un-aliased table source). + + Two guards keep the qualified-form advice correct (both covered + by regression tests): + - **SQL only.** The branch fires only for `role == + "sql_expr_ident"`. The DSL `Expr` (role `expr_column`) reaches + the same arm but has no `table.column` syntax, so a DSL bare + table-name ref keeps the generic `unknown_column` — advising + the qualified form there would be wrong. + - **Effective-qualifier match.** It matches the binding's + *effective qualifier* — the alias if present, else the table + name — not the table name independently. An aliased source + must be referenced by its alias (`FROM a x … GROUP BY a` is + invalid SQL), so the shadowed real name `a` falls through to + `unknown_column` rather than being advised as `a.`. + This mirrors the completion side's qualifier rule exactly. + + A genuine unknown column (matching no alias, table, or column) + still reports `unknown_column` verbatim. + + The message tail is deliberately clause-neutral ("to reference + one of its columns") rather than GROUP-BY-specific, because the + bare-reference arm fires across the projection, `WHERE`, + `GROUP BY`, and `HAVING`. + +This is an additive refinement of §10 and §11.2; no grammar node +changes. + ## See also - ADR-0005 — the ten-type vocabulary §10 resolves back to. diff --git a/docs/adr/README.md b/docs/adr/README.md index 101ee05..0ded724 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -37,7 +37,7 @@ This directory contains the project's ADRs, recorded per - [ADR-0029 — Column constraints (NOT NULL / UNIQUE / CHECK / DEFAULT)](0029-column-constraints.md) — **Accepted**, the four column-level constraints declared in the column-spec suffix (`create table` / `add column`) and modified on existing columns via `add constraint …` / `drop constraint …`; a pre-flight dry-run guards populated columns; `CHECK` reuses the ADR-0026 expression grammar via `Subgrammar` (`C3`) - [ADR-0030 — Advanced mode: the standard-SQL surface](0030-advanced-mode-sql-surface.md) — **Accepted**, SQL added as grammar *within the unified grammar tree* (ADR-0024), not a separate batch parser — so SQL gets the same completion / highlighting / hints / parse-errors as the DSL; mode gates the SQL forms; DDL routes through the typed `Command` executor (metadata + type vocabulary preserved), DML and `SELECT` execute as validated SQL; engine-neutral posture, the DSL→SQL teaching echo; supersedes ADR-0001's `sqlparser-rs` reservation; phased plan (`Q1` / `Q2` / `Q4`); **§13 OOS-2 (EXPLAIN of advanced SQL) superseded by ADR-0039** - [ADR-0031 — The SQL expression grammar](0031-sql-expression-grammar.md) — **Accepted**, the stratified SQL expression grammar fragment commissioned by ADR-0030 §3: a single precedence ladder (`OR`/`AND`/`NOT`, the comparison/`LIKE`/`IN`/`BETWEEN`/`IS NULL` predicate set, arithmetic incl. `||`, function calls, `CASE`) — the superset of ADR-0026's DSL `WHERE` grammar, authored as a parallel fragment so simple mode is untouched; pure validation, builds **no** AST (consumers run/store SQL as text per ADR-0030 §4/§6); reuses ADR-0026's `Subgrammar` recursion + depth cap unchanged; subquery expressions and qualified column refs deferred to ADR-0030 Phase 2; **status note (2026-05-30)** records that ADR-0022 Amendment 6 layers a curated known-function list on the `sql_expr_ident` slot (§1) for completion + the typing-time typo hint (issues #15/#16) — the grammar itself is unchanged, and the no-validation-allowlist posture stands -- [ADR-0032 — The full SQL `SELECT` grammar](0032-sql-select-grammar.md) — **Accepted**, the Phase-2 grammar commissioned by ADR-0030 §3: full `SELECT` with `INNER`/`LEFT`/`RIGHT`/`FULL OUTER`/`CROSS` joins, `GROUP BY`/`HAVING`, all four set ops (`UNION`/`UNION ALL`/`INTERSECT`/`EXCEPT`), `WITH` and `WITH RECURSIVE` CTEs, `LIMIT … OFFSET`, `DISTINCT`, `t.*`, and bare-alias projection (lifting Phase-1 §4.2); additive extensions to ADR-0031's `sql_expr` for scalar subqueries, `IN (SELECT …)`, `[NOT] EXISTS`, and qualified column refs (redeeming ADR-0031 §7 OOS-1/OOS-2); grammar-recursion via `Subgrammar(&SQL_SELECT_COMPOUND)` reuses ADR-0026's `MAX_SUBGRAMMAR_DEPTH = 64` cap unchanged; **softens ADR-0030 §8's "ambient assistance comes for free" claim**: completion scope needs new `WalkContext` accumulators (a `from_scope_stack` of `ScopeFrame`s holding `from_scope` / `cte_bindings` / `projection_aliases`), a **new walker node variant `Node::ScopedSubgrammar(&Node)`** as the push/pop trigger (existing `Node::Subgrammar` unchanged so DSL `Expr` and `sql_expr` recursion are unaffected), qualified-prefix completion narrowing, body-projection-derived CTE column resolution (so `SELECT *` and explicit-projection CTE bodies both yield real column completion past `cte_alias.|`), and a **post-walk fixup pass** that re-resolves projection-list identifier highlighting/validity once `FROM` is parsed (the projection-before-FROM problem); classifies every Phase-2 validation case against ADR-0027's ERROR/WARNING guideline (§11): five new `diagnostic.*` keys for parse-time-detectable cases (unknown qualifier, ambiguous column, projection-alias misplaced, CTE/compound arity mismatch) plus eight `engine.*` translation keys; a MatchedPath-walking predicate-warnings variant that closes the Phase-1 gap where SQL `WHERE` expressions emitted no `LIKE`-on-numeric / `= NULL` / type-mismatch warnings (ADR-0027 Amendment 1 finally extends to the SQL surface); adds a worker-side post-prepare type-resolution pass via engine column-origin metadata so bare column refs recover their playground type (partially lifting Phase-1 §4.5, the bool→0/1 case) — `Cargo.toml` gains `column_metadata` to rusqlite features (verified against pinned 0.39.0); `__rdbms_*` rejection extended to every new table-source slot; Amendment 1 narrows §12's resolution rule from a grammar-side structural classification to "trust the engine's column-origin metadata verbatim" after an empirical probe showed origin metadata follows through non-recursive CTEs, scalar subqueries, derived tables, set ops, and joins — the one structural exception is recursive CTE result columns, which return None and stay typeless; Amendment 2 records that §10.6's "rewrite the highlight class" prescription is realised via the two-pass schema-existence diagnostic + the renderer's diagnostic-overlay path (no separate per-byte rewrite step needed; no new HighlightClass variant), and that the projection-before-FROM completion narrowing has been improved by an `src/completion.rs` look-ahead probe when the leading walk's `from_scope` is empty but the full input parses +- [ADR-0032 — The full SQL `SELECT` grammar](0032-sql-select-grammar.md) — **Accepted**, the Phase-2 grammar commissioned by ADR-0030 §3: full `SELECT` with `INNER`/`LEFT`/`RIGHT`/`FULL OUTER`/`CROSS` joins, `GROUP BY`/`HAVING`, all four set ops (`UNION`/`UNION ALL`/`INTERSECT`/`EXCEPT`), `WITH` and `WITH RECURSIVE` CTEs, `LIMIT … OFFSET`, `DISTINCT`, `t.*`, and bare-alias projection (lifting Phase-1 §4.2); additive extensions to ADR-0031's `sql_expr` for scalar subqueries, `IN (SELECT …)`, `[NOT] EXISTS`, and qualified column refs (redeeming ADR-0031 §7 OOS-1/OOS-2); grammar-recursion via `Subgrammar(&SQL_SELECT_COMPOUND)` reuses ADR-0026's `MAX_SUBGRAMMAR_DEPTH = 64` cap unchanged; **softens ADR-0030 §8's "ambient assistance comes for free" claim**: completion scope needs new `WalkContext` accumulators (a `from_scope_stack` of `ScopeFrame`s holding `from_scope` / `cte_bindings` / `projection_aliases`), a **new walker node variant `Node::ScopedSubgrammar(&Node)`** as the push/pop trigger (existing `Node::Subgrammar` unchanged so DSL `Expr` and `sql_expr` recursion are unaffected), qualified-prefix completion narrowing, body-projection-derived CTE column resolution (so `SELECT *` and explicit-projection CTE bodies both yield real column completion past `cte_alias.|`), and a **post-walk fixup pass** that re-resolves projection-list identifier highlighting/validity once `FROM` is parsed (the projection-before-FROM problem); classifies every Phase-2 validation case against ADR-0027's ERROR/WARNING guideline (§11): five new `diagnostic.*` keys for parse-time-detectable cases (unknown qualifier, ambiguous column, projection-alias misplaced, CTE/compound arity mismatch) plus eight `engine.*` translation keys; a MatchedPath-walking predicate-warnings variant that closes the Phase-1 gap where SQL `WHERE` expressions emitted no `LIKE`-on-numeric / `= NULL` / type-mismatch warnings (ADR-0027 Amendment 1 finally extends to the SQL surface); adds a worker-side post-prepare type-resolution pass via engine column-origin metadata so bare column refs recover their playground type (partially lifting Phase-1 §4.5, the bool→0/1 case) — `Cargo.toml` gains `column_metadata` to rusqlite features (verified against pinned 0.39.0); `__rdbms_*` rejection extended to every new table-source slot; Amendment 1 narrows §12's resolution rule from a grammar-side structural classification to "trust the engine's column-origin metadata verbatim" after an empirical probe showed origin metadata follows through non-recursive CTEs, scalar subqueries, derived tables, set ops, and joins — the one structural exception is recursive CTE result columns, which return None and stay typeless; Amendment 2 records that §10.6's "rewrite the highlight class" prescription is realised via the two-pass schema-existence diagnostic + the renderer's diagnostic-overlay path (no separate per-byte rewrite step needed; no new HighlightClass variant), and that the projection-before-FROM completion narrowing has been improved by an `src/completion.rs` look-ahead probe when the leading walk's `from_scope` is empty but the full input parses; **Amendment 3, 2026-06-12** (issue #31): a bare in-scope **table alias** at an expression slot (`… GROUP BY o`, `o` aliasing `FROM Orders o`) is no longer a blind spot — completion now offers each FROM source's qualifier (alias-if-present-else-table-name) at a bare `sql_expr_ident` slot (folded into the column candidate list; the alias source steps aside on an exact-qualifier partial so the diagnostic can surface), and the `matched.len()==0` bare-reference arm emits a targeted `diagnostic.alias_used_as_column` / `diagnostic.table_used_as_column` ("`o` is a table alias — write `o.` …") instead of the misleading `unknown_column` (a drop-in replacement at the same span/`Error` severity, so verdict/overlay/hint paths are unchanged), checked after the projection-alias check so ORDER-BY alias refs still win; two guards keep the advice correct — **SQL-only** (`role == "sql_expr_ident"`, so the DSL `expr_column` path keeps `unknown_column` since the DSL has no `table.column` syntax) and **effective-qualifier match** (alias-if-present-else-table, so an aliased source referenced by its shadowed real name falls through rather than being advised as `name.`); a genuine unknown column still reports `unknown_column` - [ADR-0033 — The full SQL DML grammar (`INSERT` / `UPDATE` / `DELETE`)](0033-sql-dml-grammar.md) — **Accepted** (implemented + verified through sub-phase 3k, 2026-05-23; phase-exit report `docs/handoff/20260523-phase-3-verification.md`), the Phase-3 grammar commissioned by ADR-0030 §3: single- and multi-row `INSERT` (incl. `INSERT … SELECT` recursing through ADR-0032's `SQL_SELECT_COMPOUND`), `UPDATE` with `SET` assignment list, `DELETE`, all three optionally followed by `RETURNING projection_list`, plus full `ON CONFLICT … DO NOTHING / DO UPDATE` UPSERT on INSERT; **fixes the DSL-vs-SQL dispatch architecture for shared entry words (`insert`/`update`/`delete`)**: SQL-first / DSL-fallback in Advanced mode via a `Choice(SQL_shape, DSL_shape)` per shape, gated by a new walker capability `Node::Guard(fn)` — a zero-byte-consumption gating node that fails the enclosing Seq with a `ValidationError`; carries `Command::SqlInsert` / `SqlUpdate` / `SqlDelete` variants and `do_sql_*` worker handlers each of which knows the target table (for re-persistence) and the `returning: bool` flag (for DataResult routing); `shortid` auto-fill mirrors the DSL `do_insert` mechanism via worker post-fill; SQL DELETE produces the same per-relationship cascade summary the DSL DELETE does (ADR-0014 parity); three new walker diagnostics (`insert_arity_mismatch` ERROR, `auto_column_overridden` WARNING, `not_null_missing` WARNING) with positive + negative tests each; OOS list explicitly carves out `DEFAULT VALUES` (the project's planned seed feature), SQLite-specific `OR REPLACE` / `OR IGNORE` / `OR ABORT` / `OR FAIL` / `OR ROLLBACK` prefixes, `UPDATE FROM` multi-table updates, and WITH-prefixed DML; the `excluded` keyword inside `ON CONFLICT DO UPDATE` is a deliberate carve-out from ADR-0030 §7's engine-neutral posture (no standard-SQL UPSERT spelling exists that SQLite and PostgreSQL share); eleven phased sub-phases each with explicit exit gates + written DA gate, opening with the dispatch mechanism before any DML grammar lands; initial DA review recorded seven critiques that were resolved before status moved to Proposed; **Amendment 1 supersedes §2's dispatch mechanism**: the originally-chosen `Node::Guard(fn)` + `Choice(SQL_shape, DSL_shape)` was found during 3a to be unworkable as framed (any guard-in-`Choice` mechanism forces a `walk_choice` change — `walk_choice` only falls through on `NoMatch`, so Simple-mode valid-DSL would wrongly surface "this is SQL", and `walk_seq` treats a `NoMatch` past `idx 0` as a hard `Failed`, breaking Advanced-mode DSL fall-through); replaced by **category-grouped, mode-aware dispatch** in `walker::walk` (each `REGISTRY` entry tagged `CommandCategory::{Simple, Advanced}`, generalising the existing whole-command `is_advanced_only` gate), shared entry words carrying a node in both groups, no `Node::Guard` and no `walk_choice`/`walk_seq` change, advanced-mode completion SQL-first with DSL as a full-line fallback; **Amendment 2 (sub-phase 3f) supersedes §7's cascade mechanism**: the WHERE-injected per-child pre-count rested on a premise that was factually wrong about the DSL handler (which detects cascades by before/after row-count diffing inside a transaction, not by `Expr`-derived pre-count subqueries) and would have broken the §2 parity promise by reporting `SET NULL` the DSL path doesn't; replaced by mirroring `do_delete`'s count-diff exactly (verbatim DELETE executes, child-count diff observes the cascade — `ON DELETE CASCADE` row removals only, SET NULL deferred for both paths to preserve parity), which shares the render-layer formatter for free via `CommandOutcome::Delete` and **withdraws risk R2** (no WHERE-byte extraction, no N+1 subquery); **Amendment 3 (sub-phase 3j) records the command-identity model and defers the execution-mode side-channel**: a command is the typed outcome of a *mode-rooted* grammar path and its identity is intrinsic (Advanced mode tries SQL first, falls back to the *Simple* DSL command when no SQL branch matches a token, e.g. `delete … --all-rows`; note `update … --all-rows` does *not* fall back — the SQL `SET` expression eats `--all-rows`, harmless since the engine treats it as a comment); **Simple mode commits the DSL candidate for shared words** so the *real* DSL error surfaces, and when that line would also run in advanced mode the rendering layer **combines** them — DSL error **plus** an `advanced_mode.also_valid_sql` pointer ("… (valid as SQL in advanced mode)") — keeping the actionable DSL fix while pointing at advanced mode; bare "this is SQL" is reserved for entry words with no DSL form (`select`/`with`); a fully-overlapping input (`insert … values …`) legitimately yields *two distinct commands* (`Command::Insert` typed-AST vs `Command::SqlInsert` validated-text) that do the same thing but execute differently (ADR-0030 §4), so each is tested in the mode that produces it; **corrects the plan's 3j exit-gate premise** that the DSL DML tests run in Simple mode (they call `parse_command`, which defaults to Advanced) — the real invariant is "Simple-mode behaviour unchanged, Advanced mode SQL-first, DSL grammar tested in Simple mode, both variants tested in their producing mode", with §6/§7 parity keeping the paths observably equivalent; and **defers to its own future ADR** the execution-time mode side-channel (three-way `Mode`: simple/advanced/advanced-one-shot threaded through `Action`→worker, for mode-dependent *output* like echoing generated SQL) — today only the *rendering* side-channel `OutputLine.mode_at_submission` exists, and the three-way distinction is not required for Phase 3 dispatch correctness; **Amendment 4, 2026-05-27** (design agreed, pending impl): **reverses Amendment 3's `update … --all-rows` counter-example as a bug** — surfaced by the ADR-0038 echo design. The walker has no `--` comment support (it lexes two minus operators) while the engine treats `--` as a comment, so `update T set x=42 --all-rows` was silently parsed as the expression `42 - -all - rows` over **non-existent columns** `all`/`rows` (an ADR-0027 "flag-if-it-will-fail" case) and matched `SqlUpdate`. Decision: the `--all-rows` sequence makes the SQL `UPDATE` shape **fail**, so dispatch **falls back to the DSL `Update { AllRows }`** — symmetry with `delete … --all-rows`; no `--` comment feature introduced (trailing comments stay unsupported). Inverts `sql_dml_e2e.rs::e2e_update_all_rows_in_advanced_does_not_fall_back_to_dsl`; mechanism settled test-first in the build; folded into the ADR-0038 effort (makes `update … --all-rows` echoable); **Amendment 5, 2026-05-28** (implemented + verified, user-confirmed): `advanced_mode.also_valid_sql` (the cross-mode pointer from Amendment 3) fires on **validity**, not just **parse** — *"valid"* meaning `input_verdict_in_mode(input, schema, Mode::Advanced) == None` in the ADR-0027 sense (parse succeeds *and* no Warning/Error diagnostic from any pass). Surfaced by **issue #1**: a positional `INSERT INTO T VALUES (…)` (no column list) with a value count that didn't match the target's column count parsed in advanced but failed at the engine, so the syntactic-only Amendment-3 gate promised a mode switch that wouldn't help. Closes the gap by (a) extending `dml_insert_arity_diagnostics` (§8.1, previously Form A only — its own doc-comment deferred Form B) to also check the no-column-list form against the schema's column count, emitting a new `diagnostic.insert_arity_mismatch_form_b` ERROR per offending tuple, and (b) refactoring `advanced_alternative_note` to read the validity verdict instead of running its own bespoke check — any static diagnostic added to the pipeline in the future automatically participates in the pointer gate. Side benefit: the `[ERR]` validity indicator now lights up at typing time for the reported scenario, no longer needing a submit to learn the line is wrong. Tests pinned: `insert_form_b_arity_mismatch_under_supply_fires` / `_over_supply_fires` / `_match_is_silent` / `_unknown_table_is_silent` (walker); `ambient_hint_omits_advanced_pointer_when_form_b_value_count_wouldnt_match` (gate); `simple_mode_submit_of_sql_construct_appends_advanced_pointer` (pointer still fires for genuine SQL-only constructs against a known schema). Amendment 3's "would parse in advanced mode" should henceforth be read as a synonym for "valid in advanced mode" in this stricter sense; the user-confirmed behavioural change is exactly the issue #1 bug case (no other input flips its pointer state) - [ADR-0034 — `history.log` as a complete command journal; replay reads success-only](0034-history-journal-and-replay-filter.md) — **Accepted**, resolves a three-way tension in `history.log`'s roles found while implementing ADR-0033 3f: (1) the persistent log is success-only while the in-memory Up/Down recall ring records *every* submission (success or failure, "so users can recall and edit typo'd commands"), and the ring is re-seeded from the log on project open — so **failed commands are recallable within a session but silently lost across sessions**; (2) replay wants the state-building (successful) commands while recall wants everything typed, which one success-only file cannot serve; (3) `replay history.log` never actually worked — `run_replay` parses each whole line through the DSL parser with no understanding of the `||` record shape, so a real log fails on line 1, and **no test ever fed the pipe format to replay** (the `replay_history_log_records_subcommands_only` test only checks what replay *writes*, never replays the log as input). Decision: `history.log` becomes a **complete journal** — every submission recorded, tagged `ok`/`err` via the status field the format already reserved (ADR-0015 §5) — and **each consumer filters**: hydration reads all records (cross-session recall matches in-session), replay reads `ok` only (and learns the journal format, while still accepting bare-command `.commands` scripts; detection by the leading timestamp+status prefix so a `|` inside a bare command isn't misread). Successful commands stay journalled transactionally by the worker; failed commands are journalled `err` best-effort from the runtime/app error path (a parse failure never reaches the worker). Amends ADR-0006's "successfully executed" wording and ADR-0015 §5 ("status always `ok`") / §12 (hydration). Code deferred to two tracked test-first sub-tasks (journal-failures+filtering; replay-parses-journal-format); existing all-`ok` logs need no migration; **implemented 2026-05-24** (plan `docs/plans/20260524-adr-0034-history-journal.md`); **Amendment 1 (2026-05-24): replay filters out app-lifecycle commands** — a working `replay history.log` (the §3 fix) exposed that the journal also records `save as`/`load`/`new`/`export`/`import`/`rebuild`/`mode` (which would panic the worker dispatch or abort the replay), so replay now re-applies **only** schema/data write commands and **skips** every `Command::App` + nested `Command::Replay`; **all skips continue** (never abort — reversing the prior nested-`replay` refusal, so a journal containing a once-run `replay` needs no hand-editing, and the infinite-loop footgun is closed by construction), with a `[skip]` **warning** on `import` and nested-`replay` skips (their omission can leave replayed state incomplete) and silent skips for the rest; `replay.error_nested` removed, `replay.skipped_import`/`replay.skipped_replay` added, `ReplayCompleted` carries `warnings` - [ADR-0035 — Advanced-mode SQL DDL](0035-advanced-mode-sql-ddl.md) — **Accepted** (design agreed 2026-05-24; validated end-to-end by sub-phases 4a/4a.2/4a.3/4b `CREATE TABLE` (incl. foreign keys) + 4c `DROP TABLE [IF EXISTS]` + 4d `CREATE [UNIQUE] INDEX` / `DROP INDEX [IF EXISTS]` + 4e `ALTER TABLE` add/drop/rename column + 4f `ALTER TABLE … ALTER COLUMN TYPE` + 4g `ALTER TABLE` add/drop constraint + add FK + 4h `ALTER TABLE … RENAME TO` + 4i verification sweep (completion merge + simple/advanced completion colour + describe of table-level constraints + self-ref FK indicator + CREATE-TABLE help/usage), implemented 2026-05-25/26 — **Phase 4 complete**; **Amendment 1, 2026-05-26**: drop a composite UNIQUE via a derived, engine-neutral `unique_` name that reuses the existing `DROP CONSTRAINT ` grammar — no new syntax, no metadata, §4g anonymity intact; `describe` shows the name; dropping a UNIQUE-covered *column* now refuses with that name + the drop command), **Phase 4** of the ADR-0030 roadmap (peer of 0031/0032/0033) and **clarifies ADR-0030 §4**. Advanced-mode `CREATE`/`DROP`/`ALTER TABLE` + `CREATE`/`DROP INDEX` get their **own per-statement commands** (`SqlCreateTable`/`SqlAlterTable`/`SqlDropTable`/`SqlCreateIndex`/`SqlDropIndex`), like DML's `Sql*` set — but unlike DML they **execute *structurally*, not verbatim** (raw execution would lose the playground's types, named relationships, and `STRICT`; "verbatim" was a DML convenience, not a rule). Handlers **reuse the low-level schema/metadata helpers** where the operation matches simple mode and **stand alone where the SQL surface is richer** (clarity over forced refactoring); simple mode is untouched (additive). Dispatch: `create`/`drop` reuse ADR-0033 Amendment 1's category-grouped mode-aware dispatch (SQL-first, simple fallback); `alter` is a new advanced-only entry word. Full surface (no pre-emptive cuts, `Q4`): `CREATE TABLE` with column + table constraints, single/compound `PRIMARY KEY`, inline + table-level `FOREIGN KEY` → **named relationships** (one statement = one command = **one undo step**, ADR-0006); `ALTER TABLE` add/drop/rename column, `ALTER COLUMN TYPE`, add/drop constraint, add FK, **`RENAME TO`** (advanced-only table rename — new low-level op renaming the table + its CSV + the relationship and table-CHECK metadata, closing the rename half of `C1`); `CREATE [UNIQUE] INDEX` / `DROP INDEX`. Type slot accepts the ten playground keywords **and** standard-SQL aliases (`integer`→`int`, `varchar`→`text`, `timestamp`→`datetime`, …; length args accepted-and-ignored; no engine type names in/out — ADR-0030 §5). `CHECK`/`DEFAULT` reuse ADR-0031 `sql_expr`. **Pre-implementation `/runda` refinements (2026-05-24, user-confirmed):** `CREATE TABLE`/`DROP TABLE` **admit `IF [NOT] EXISTS`** (no-op-that-succeeds-with-a-note — a near-universal cross-vendor idiom, reclassified *into* scope, not engine-specific); `INTEGER PRIMARY KEY` maps to a **plain `int`** PK, *not* auto-increment (`serial` stays the sole auto-increment type). **Column-type-conversion is unified** (ADR-0017 engine, mode-appropriate policy): clean auto-converts and incompatible/own-type-static cases refuse in both modes, but a **lossy** change refuses-by-default in simple mode (`--force-conversion` opts in) while advanced mode **performs it with a loss note** and relies on **`undo` as the safety net** — no force flag, no dropping to simple mode (a payoff of shipping ADR-0006 first). OOS: views/triggers/txn-control/PRAGMA/etc. (ADR-0030 §3), the Postgres `USING` clause, and the DSL→SQL teaching echo (ADR-0030 Phase 5). Sub-phases 4a–4i, plus **4a.2** (per-column `CHECK`/`DEFAULT` via raw `sql_expr` text — `sql_expr` is validate-only, no `Expr` AST — + composite `UNIQUE(a,b)`; no new internal table) and **4a.3** (table-level/multi-column `CHECK`, landed via the new `__rdbms_playground_table_checks` metadata table because SQLite has no PRAGMA for CHECK; the builder tells a table-level CHECK from a column-level one by element position) and **4b** (foreign keys — inline `REFERENCES` + table-level `FOREIGN KEY` → ADR-0013 named relationships in the create transaction, one undo step; self-references + bare `REFERENCES ` supported, user-confirmed) and **4c** (`DROP TABLE [IF EXISTS]` → `SqlDropTable`, reusing `do_drop_table`; `IF EXISTS` is a no-op-with-note via `DropOutcome::Skipped`) and **4d** (`CREATE [UNIQUE] INDEX [IF NOT EXISTS] [] ON (cols)` → `SqlCreateIndex` and `DROP INDEX [IF EXISTS] ` → `SqlDropIndex`, reusing `do_add_index`/`do_drop_index`; **`CREATE UNIQUE INDEX` admitted** — ADR-0025 **Amendment 1** — via an additive `IndexSchema.unique` flag that round-trips through `project.yaml` and rebuild, with `[unique]` markers in the structure view + items panel, while simple-mode `add unique index` stays deferred; `IF [NOT] EXISTS` reuses the 4c skip path; `create`/`drop` each gain a *second* advanced node, exercising the all-candidates dispatch) and **4e** (`ALTER TABLE` add/drop/rename column → `SqlAlterTable`; `alter` is a new advanced-**only** entry word, runtime-decomposed to the existing `do_add_column`/`do_drop_column`/`do_rename_column` — no new worker layer; `do_add_column` extended to consume raw `default_sql`/`check_sql` so ADD COLUMN reaches CREATE-TABLE constraint parity; drop/rename refuse a column any CHECK references (table-level AND column-level, incl. a column's own self-check on rename) — the 4a.3 deferral, via a raw-CHECK-text tokenizer in the shared executors, so it guards both surfaces and fixes a latent rename-drift bug; SQL DROP COLUMN refuses an index-covered column with no `--cascade` spelling; the column executors + `do_add_index` gained an internal-`__rdbms_*`-table guard — all user-confirmed) and **4f** (`ALTER TABLE … ALTER COLUMN TYPE` → a fourth `AlterTableAction`, runtime-decomposed to the existing `change_column_type` with `ChangeColumnMode::ForceConversion` — which **is** the §7 advanced policy: lossy converts *with a note* (no force flag), incompatible + ADR-0017 static refusals (`↔ blob`, same-type, `date ↔ datetime`, non-`int → serial`) still refuse, while **`int → serial` is allowed** (auto-fills nulls + UNIQUE, ADR-0018 §8 — the §7 "→serial refused" summary is looser than the code); the builder discriminates the fourth branch by the **`type` keyword** (unique — ADD COLUMN's type is an ident), the type slot reuses `SQL_TYPE`; the internal-`__rdbms_*` guard was folded into `do_change_column_type`, closing the simple `change column` exposure too — user-confirmed) and **4g** (`ALTER TABLE … ADD [CONSTRAINT ] (CHECK | UNIQUE | FOREIGN KEY)` + `DROP CONSTRAINT `; ADD = CHECK + composite UNIQUE + FK, with `ADD PRIMARY KEY` and a *named* UNIQUE refused — composite UNIQUE is anonymous in our model; each ADD reuses a low-level path (table-CHECK/UNIQUE rebuild with a dry-run guard; FK → `add_relationship`, bare `REFERENCES

` → parent single-PK), DROP CONSTRAINT resolves the name to a table-CHECK then a child-side FK; **named table-CHECKs round-trip** via a nullable `name` column on `__rdbms_playground_table_checks` (**rebuild-only** arrival — pre-4g projects gain it on `rebuild`, a named add on an un-upgraded project is refused with a friendly "rebuild first" message) *and* a `project.yaml` `check_constraints` extension to an `{expr, name}` mapping (the bare-string form still reads); the internal-`__rdbms_*` guard was folded into `do_add_constraint`/`do_add_relationship`, completing that guard class — all user-confirmed) and **4h** (`ALTER TABLE … RENAME TO` — the one genuinely new low-level op, `do_rename_table`: a native engine rename plus one-transaction reconciliation of every metadata row naming the table (`__rdbms_playground_columns`, **both ends** of `__rdbms_playground_relationships`, `__rdbms_playground_table_checks`), the CSV file (the existing rewrite+delete path — no new persistence method), and **CHECK text that qualifies a column with the old table name** (`T.age`→`U.age`, a planning-`/runda` finding — the engine rewrites the live CHECK but the stored text would drift and break a fresh rebuild; `rewrite_check_table_qualifier` keeps them in step); grammar splits the `rename` verb into one branch with an inner Choice on a distinct second keyword (`column` vs `to`), the new-name slot mirroring the `CREATE TABLE` name slot; refuses same-name / existing-target / `__rdbms_*` / non-existent, with **case-insensitive** collision checks behind an engine-neutral pre-check (a finished-slice `/runda` finding — the engine matches names case-insensitively); auto-named indexes *and* relationships keep their stale names (only table-name columns update — §6 scope); one undo step; advanced-only, closing the rename half of `C1` — all user-confirmed) and **4i** (the verification sweep that completes Phase 4: the shared-entry-word completion merge + the simple-vs-advanced completion colour-when-mixed with Both→Advanced→Simple block ordering; `describe` of table-level composite UNIQUE + table CHECK; the self-ref FK pre-submit indicator fix; and the CREATE-TABLE help/usage skeleton refresh). **All of Phase 4 (4a–4i) is shipped.** Each sub-phase has exit + DA gates; **Amendment 2, 2026-05-27** (design agreed, pending impl): a **standard-first dialect stance** (refines ADR-0030's "standard SQL" posture — ISO spelling is canonical + echoed where one exists; a vendor shorthand may be *accepted* but isn't canonical; where ISO offers none, *one* documented vendor spelling is a deliberate extension) + an `ALTER COLUMN` **constraint gap-fill** surfaced by the ADR-0038 echo design: makes ISO `ALTER COLUMN … SET DATA TYPE` the canonical type-change verb with `TYPE` retained as a synonym (**reverses §4f's "no `SET DATA TYPE`"**), and adds `SET/DROP DEFAULT` (ISO) + `SET/DROP NOT NULL` (the one documented extension — ISO has no in-place NOT-NULL verb; PostgreSQL's chosen for being type-independent), all **rebuild-backed via the existing ADR-0029 `do_add_constraint`/`do_drop_constraint` executors** (dry-run + internal-table guards free, no new worker layer), reaching simple↔advanced constraint-mod **parity for NOT NULL + DEFAULT**; the **rebuild stays hidden** (Category-1 engine detail, ADR-0038). Residual gap left open + flagged: dropping a **column-level (anonymous) UNIQUE/CHECK** (no portable name — same class as Am1's parallel gap), which ADR-0038's catalogue marks "no headline echo" diff --git a/src/completion.rs b/src/completion.rs index 38bf3bd..10b319a 100644 --- a/src/completion.rs +++ b/src/completion.rs @@ -753,6 +753,51 @@ pub fn candidates_at_cursor_with_in_mode( ); } + // Source 1.95: in-scope table aliases (issue #31). At a bare + // `sql_expr_ident` slot — one *not* already past a `qualifier .` + // (handled by §10.5 column narrowing) — the partial may be a + // FROM-source the learner is mid-typing as a qualifier + // (`group by o` → `o.`). Offer each binding's *qualifier*: + // its alias if it has one, else the table name (an aliased source + // must be referenced by its alias, not the raw table name). This + // makes aliases Tab-discoverable and — since a non-empty candidate + // set overlapping the partial suppresses the under-cursor error + // (the `typing_over_diag` path) — keeps the alias from flashing as + // a bogus "unknown column" while typing. Mixed into `identifiers` + // so it sorts/dedups/colours uniformly with column candidates. + let alias_candidates: Vec = + if has_sql_expr_slot && prefix_qualifier.is_none() { + // Once the partial *exactly* matches an in-scope qualifier, + // discoverability is served — the learner has a whole alias + // in hand and now needs the "add `.column`" hint + // (`diagnostic.alias_used_as_column`), not sibling aliases + // that merely share the prefix. Offering them would also let + // the `typing_over_diag` path suppress that very hint. So in + // the exact-match case we emit no alias candidates and let + // the targeted diagnostic surface. + let partial_is_exact_alias = resolution_from_scope.iter().any(|b| { + let q = b.alias.as_deref().unwrap_or(b.table.as_str()); + q.eq_ignore_ascii_case(&partial_prefix) + }); + if partial_is_exact_alias { + Vec::new() + } else { + let mut out: Vec = Vec::new(); + for binding in resolution_from_scope { + let qualifier = + binding.alias.as_deref().unwrap_or(binding.table.as_str()); + if matches_prefix(qualifier) + && !out.iter().any(|q| q.eq_ignore_ascii_case(qualifier)) + { + out.push(qualifier.to_string()); + } + } + out + } + } else { + Vec::new() + }; + // Source 2: schema identifiers — accumulated across every // matching schema-listable `Ident { source }` expectation. // `NewName` / `Types` / `Free` sources don't query the @@ -788,6 +833,10 @@ pub fn candidates_at_cursor_with_in_mode( }) .filter(|name| matches_prefix(name)) .collect(); + // Fold in the in-scope alias qualifiers (Source 1.95). They are + // already prefix-filtered; dedup against any column of the same + // spelling happens via the shared sort/dedup below. + identifiers.extend(alias_candidates); identifiers.sort(); identifiers.dedup(); // If an identifier shares its name with a keyword candidate @@ -1930,6 +1979,67 @@ mod tests { cache } + fn two_table_alias_cache() -> SchemaCache { + use crate::dsl::types::Type; + let mut cache = schema_with_table("a", &[("id", Type::Int), ("name", Type::Text)]); + cache.tables.push("b".to_string()); + cache.columns.push("total".to_string()); + cache.table_columns.insert( + "b".to_string(), + vec![ + TableColumn::new("id", Type::Int), + TableColumn::new("total", Type::Real), + ], + ); + cache + } + + #[test] + fn bare_expr_slot_offers_in_scope_aliases() { + // Issue #31: at a bare SQL-expression slot (here GROUP BY) the + // in-scope FROM aliases are Tab-discoverable, so a learner can + // reach `o.` without guessing the alias. + let cache = two_table_alias_cache(); + let input = "select a.id from a o join b z on o.id = z.id group by "; + let cs = cands_with(input, input.len(), &cache); + assert!(cs.contains(&"o".to_string()), "alias `o` must be offered; got {cs:?}"); + assert!(cs.contains(&"z".to_string()), "alias `z` must be offered; got {cs:?}"); + } + + #[test] + fn bare_expr_slot_narrows_aliases_by_partial_prefix() { + // A partial that prefix-matches several aliases offers each; + // an exact match (`o`) is the learner's whole alias — no + // sibling-alias noise, so the `alias_used_as_column` hint can + // surface instead (issue #31). + let cache = two_table_alias_cache(); + let input = "select a.id from a aa join b ab on aa.id = ab.id group by a"; + let cs = cands_with(input, input.len(), &cache); + assert!(cs.contains(&"aa".to_string()), "alias `aa` must be offered; got {cs:?}"); + assert!(cs.contains(&"ab".to_string()), "alias `ab` must be offered; got {cs:?}"); + + // Exact-alias partial: the alias source steps aside. + let exact = "select aa.id from a aa join b ab on aa.id = ab.id group by aa"; + let cs2 = cands_with(exact, exact.len(), &cache); + assert!( + !cs2.iter().any(|c| c == "ab"), + "an exact-alias partial must not surface sibling aliases; got {cs2:?}", + ); + } + + #[test] + fn alias_not_offered_after_a_qualifier_dot() { + // Past `o.` the §10.5 column-narrowing owns the slot; aliases + // are not candidates there. + let cache = two_table_alias_cache(); + let input = "select a.id from a o join b z on o.id = z.id group by o."; + let cs = cands_with(input, input.len(), &cache); + assert!( + !cs.iter().any(|c| c == "o" || c == "z"), + "aliases must not be offered after a qualifier dot; got {cs:?}", + ); + } + #[test] fn update_set_offers_only_current_table_columns() { use crate::dsl::types::Type; diff --git a/src/dsl/walker/mod.rs b/src/dsl/walker/mod.rs index f32ffa1..3723f20 100644 --- a/src/dsl/walker/mod.rs +++ b/src/dsl/walker/mod.rs @@ -1163,6 +1163,60 @@ fn schema_existence_diagnostics( // Allowed-clause alias ref — silent. continue; } + // Issue #31: the bare ident is itself an + // in-scope FROM source — a table alias + // (`o` from `FROM Orders o`) or, when the + // source is un-aliased, the table name. The + // learner means a *column* of that source + // (`o.`); calling it an "unknown + // column" misleads. Point at the qualified + // form. + // + // Two guards keep the advice correct: + // - SQL only (`role == "sql_expr_ident"`): + // the DSL `Expr` (role `expr_column`) + // has no `table.column` syntax, so the + // qualified-form advice would be wrong; + // it keeps the generic unknown_column. + // - Match the *effective qualifier* + // (alias if present, else table name), + // not the table name independently. An + // aliased source must be referenced by + // its alias — `FROM Orders o … Orders` + // is invalid SQL, so it must NOT be + // advised as `Orders.`. Mirrors + // the completion side's qualifier rule. + let qualifier_binding = (role + == "sql_expr_ident") + .then(|| { + bindings.iter().find(|b| { + let q = b + .alias + .as_deref() + .unwrap_or(b.table.as_str()); + q.eq_ignore_ascii_case(&item.text) + }) + }) + .flatten(); + if let Some(binding) = qualifier_binding { + let key = if binding.alias.is_some() { + "diagnostic.alias_used_as_column" + } else { + "diagnostic.table_used_as_column" + }; + diagnostics.push(Diagnostic { + severity: Severity::Error, + span: item.span, + message: crate::friendly::translate( + key, + &[( + "name", + &item.text as &dyn std::fmt::Display, + )], + ), + }); + continue; + } let table_arg = if bindings.len() == 1 { bindings[0].table.clone() } else { @@ -6330,6 +6384,121 @@ mod tests { ); } + // ---- Issue #31 — bare table alias / table used as a column ---- + + #[test] + fn bare_table_alias_in_group_by_is_alias_hint_not_unknown_column() { + // Issue #31: `… GROUP BY o` where `o` aliases a FROM source. + // The learner means `o.`; the diagnostic must point + // at the qualified form, NOT call `o` an unknown column. + let schema = two_table_schema(); + let diags = diag_keys( + "select a.id from a o join b on a.id = b.id group by o", + &schema, + ); + assert!( + diags + .iter() + .any(|d| d.contains("`o` is a table alias") && d.contains("o.")), + "expected alias_used_as_column hint; got {diags:?}", + ); + assert!( + !diags.iter().any(|d| d.contains("no such column")), + "unknown_column must not fire for an in-scope alias; got {diags:?}", + ); + } + + #[test] + fn bare_table_alias_in_projection_is_alias_hint() { + // The same applies outside GROUP BY — a bare alias in the + // projection (`SELECT o …`) is equally not a column. + let schema = two_table_schema(); + let diags = + diag_keys("select o from a o join b on a.id = b.id", &schema); + assert!( + diags.iter().any(|d| d.contains("`o` is a table alias")), + "expected alias_used_as_column hint in projection; got {diags:?}", + ); + } + + #[test] + fn bare_unaliased_table_used_as_column_is_table_hint() { + // An un-aliased FROM source referenced bare gets the + // table-form hint (qualify with the table name). + let schema = two_table_schema(); + let diags = diag_keys("select id from a group by a", &schema); + assert!( + diags + .iter() + .any(|d| d.contains("`a` is a table") && d.contains("a.")), + "expected table_used_as_column hint; got {diags:?}", + ); + } + + #[test] + fn genuine_unknown_column_still_reports_no_such_column() { + // Regression guard: the alias branch must not swallow a + // genuine typo. `nope` matches no alias, no table, no column. + let schema = two_table_schema(); + let diags = diag_keys( + "select a.id from a o join b on a.id = b.id group by nope", + &schema, + ); + assert!( + diags.iter().any(|d| d.contains("no such column") && d.contains("nope")), + "a genuine unknown column must still report no such column; got {diags:?}", + ); + } + + #[test] + fn aliased_table_referenced_by_real_name_is_not_table_hint() { + // DA guard (issue #31): a source aliased as `x` must be + // referenced by the alias — `FROM a x … GROUP BY a` is invalid + // SQL, so we must NOT advise `a.`. The branch matches + // the *effective qualifier* (the alias when present), so `a` + // (the now-shadowed table name) falls through to the generic + // unknown_column rather than wrong qualified-form advice. + let schema = two_table_schema(); + let diags = diag_keys( + "select x.id from a x join b on x.id = b.id group by a", + &schema, + ); + assert!( + diags.iter().any(|d| d.contains("no such column") && d.contains("`a`")), + "an aliased table referenced by its real name must fall through to \ + unknown_column; got {diags:?}", + ); + assert!( + !diags.iter().any(|d| d.contains("is a table")), + "must not advise `a.` when `a` is aliased as `x`; got {diags:?}", + ); + } + + #[test] + fn dsl_bare_table_name_in_where_keeps_unknown_column() { + // DA guard (issue #31): the alias/table hint is SQL-only + // (role `sql_expr_ident`). The DSL `Expr` (role `expr_column`) + // has no `table.column` syntax, so advising the qualified form + // would be wrong. A DSL bare table-name ref stays the generic + // unknown_column it was before issue #31. + let schema = + schema_with("Customers", &[("id", Type::Int), ("Name", Type::Text)]); + for input in [ + "show data Customers where Customers = 5", + "update Customers set Name = 'x' where Customers = 5", + ] { + let diags = diag_keys_simple(input, &schema); + assert!( + diags.iter().any(|d| d.contains("no such column")), + "DSL bare table ref must stay unknown_column for {input:?}; got {diags:?}", + ); + assert!( + !diags.iter().any(|d| d.contains("is a table")), + "DSL must not get SQL qualified-form advice for {input:?}; got {diags:?}", + ); + } + } + // ---- ADR-0032 §11.2 — compound_arity_mismatch ---- #[test] diff --git a/src/friendly/keys.rs b/src/friendly/keys.rs index b26d01e..57e2ed3 100644 --- a/src/friendly/keys.rs +++ b/src/friendly/keys.rs @@ -38,6 +38,7 @@ /// `(key, expected_placeholders)`. Sorted by key for grep-ability. pub const KEYS_AND_PLACEHOLDERS: &[(&str, &[&str])] = &[ // ---- Pre-submit diagnostics (ADR-0027) ---- + ("diagnostic.alias_used_as_column", &["name"]), ("diagnostic.ambiguous_column", &["column", "qualifiers"]), ("diagnostic.auto_column_overridden", &["column", "type"]), ("diagnostic.compound_arity_mismatch", &["op", "left_n", "right_n"]), @@ -63,6 +64,7 @@ pub const KEYS_AND_PLACEHOLDERS: &[(&str, &[&str])] = &[ ("diagnostic.not_null_missing", &["column"]), ("diagnostic.like_numeric", &["column", "type"]), ("diagnostic.projection_alias_misplaced", &["alias", "clause"]), + ("diagnostic.table_used_as_column", &["name"]), ("diagnostic.type_mismatch", &["column", "type"]), ("diagnostic.unknown_column", &["name", "table"]), ("diagnostic.unknown_qualifier", &["qualifier"]), diff --git a/src/friendly/strings/en-US.yaml b/src/friendly/strings/en-US.yaml index 8bfb9c5..5999ff8 100644 --- a/src/friendly/strings/en-US.yaml +++ b/src/friendly/strings/en-US.yaml @@ -640,6 +640,12 @@ diagnostic: unknown_qualifier: "no such table or alias in scope: `{qualifier}`" ambiguous_column: "`{column}` is ambiguous — appears in {qualifiers}" projection_alias_misplaced: "alias `{alias}` cannot be used in {clause} — aliases are not bound until after `SELECT`'s projection list" + # Issue #31: a bare table alias / table name used where the grammar + # expects a column (e.g. `GROUP BY o`). The name *is* in scope — it + # is the alias of a FROM source — so calling it an "unknown column" + # misleads. Point the learner at the qualified `alias.column` form. + alias_used_as_column: "`{name}` is a table alias — write `{name}.` to reference one of its columns" + table_used_as_column: "`{name}` is a table — write `{name}.` to reference one of its columns" cte_arity_mismatch: "CTE `{cte}` declares {declared} columns but its body has {actual}" compound_arity_mismatch: "`{op}` requires both sides to have the same number of columns — left has {left_n}, right has {right_n}" duplicate_cte: "duplicate `WITH` table name: `{name}`" diff --git a/src/input_render.rs b/src/input_render.rs index 47efe00..cf11299 100644 --- a/src/input_render.rs +++ b/src/input_render.rs @@ -1765,6 +1765,65 @@ mod tests { cache } + fn issue31_join_cache() -> crate::completion::SchemaCache { + use crate::completion::{SchemaCache, TableColumn}; + use crate::dsl::types::Type; + let mut cache = SchemaCache::default(); + let tables: &[(&str, &[(&str, Type)])] = &[ + ("Customers", &[("id", Type::Serial), ("name", Type::Text)]), + ( + "Products", + &[("id", Type::Serial), ("name", Type::Text), ("price", Type::Decimal)], + ), + ( + "OrderLines", + &[ + ("id", Type::Serial), + ("order_id", Type::Int), + ("product_id", Type::Int), + ("count", Type::Int), + ], + ), + ( + "Orders", + &[("id", Type::Serial), ("customer_id", Type::Int), ("date", Type::Date)], + ), + ]; + for (t, cols) in tables { + cache.tables.push((*t).to_string()); + let tc: Vec = + cols.iter().map(|(n, ty)| TableColumn::new(*n, *ty)).collect(); + for c in &tc { + cache.columns.push(c.name.clone()); + } + cache.table_columns.insert((*t).to_string(), tc); + } + cache + } + + #[test] + fn issue31_group_by_partial_alias_shows_alias_hint() { + // Issue #31 end-to-end: the manual-testing query ended in + // `… group by o`, where `o` aliases `Orders`. The ambient + // hint must guide the learner to `o.`, not claim + // `o` is an unknown column. + let cache = issue31_join_cache(); + let input = "select c.name as customer_name, o.id as order_id, o.date, sum(ol.count*p.price) as total from Orders o join OrderLines ol on o.id=ol.order_id join Products p on p.id=ol.product_id join Customers c on c.id=o.customer_id group by o"; + match ambient_hint_in_mode(input, input.len(), None, &cache, Mode::Advanced) { + Some(AmbientHint::Prose(p)) => { + assert!( + p.contains("`o` is a table alias") && p.contains("o."), + "expected the alias hint; got: {p:?}", + ); + assert!( + !p.contains("no such column"), + "must not show the misleading unknown-column message; got: {p:?}", + ); + } + other => panic!("expected a Prose alias hint; got: {other:?}"), + } + } + #[test] fn ambient_hint_at_insert_first_value_shows_int_prose() { use crate::dsl::types::Type;