diff --git a/docs/handoff/20260519-handoff-22.md b/docs/handoff/20260519-handoff-22.md new file mode 100644 index 0000000..a19dd88 --- /dev/null +++ b/docs/handoff/20260519-handoff-22.md @@ -0,0 +1,259 @@ +# Session handoff — 2026-05-19 (22) + +Twenty-second handover. **Interim** handoff: this session +finished **ADR-0028** (handed off in handoff-21), then started +**ADR-0029** — column constraints (`NOT NULL` / `UNIQUE` / +`CHECK` / `DEFAULT`). ADR-0029 is **written, accepted, and +implemented through commit 4 of 6**; commits 5–6 are planned +in full detail in §4 and not yet built. A fresh session can +implement them from this file + the ADR. + +## §1. State at handoff + +**Branch:** `main`. Working tree clean. **8 commits** since +handoff-21 (`02234e6`), all local — push asynchronously, not +blocking. + +``` + docs: handoff 22 — ADR-0029 through commit 4 +942222b constraints: CHECK — check () at create/add (ADR-0029) +58d8958 add column: column constraints — NOT NULL/UNIQUE/DEFAULT (§6) +12395a9 create table: column constraints — NOT NULL/UNIQUE/DEFAULT grammar +a60e879 db: column-constraint infrastructure — NOT NULL/UNIQUE/DEFAULT +eff2ee8 refactor: ColumnSpec / AddColumn carry constraint fields (scaffolding) +7bfd213 docs: ADR-0029 — column constraints +59351e0 docs: move indexes/query-plans out of the deferred list +02234e6 docs: handoff 21 — ADR-0028 complete +``` + +**Tests:** **1201 passing, 0 failing, 1 ignored** (`cargo +test`). The ignored test is the long-standing `` ```ignore `` +doc-test in `src/friendly/mod.rs`. Typing-surface matrix: +**174 cells**, unchanged this session. + +**Clippy:** clean (`cargo clippy --all-targets -- -D +warnings`, nursery group). + +## §2. Done before ADR-0029 + +- **ADR-0028 (query plans / `explain`)** — finished and fully + documented in **handoff-21**. Read that for context. +- **`docs: move indexes/query-plans out of the deferred + list`** (`59351e0`) — a CLAUDE.md tidy-up only. + +## §3. ADR-0029 — what it is, what's done + +Read **`docs/adr/0029-column-constraints.md`** — it is the +spec, and it has been kept current (the two deviations below +are already folded in). The feature: the four column-level +constraints, declared in a suffix after a column's `(type)` +group and — for commit 5 — modifiable on existing columns. + +**Commits 1–4 (done):** + +1. `eff2ee8` — `ColumnSpec` / `Command::AddColumn` gained the + four constraint fields (`not_null` / `unique` / + `default: Option` / `check: Option`), all + defaulting off; `Database::add_column` now takes a + `ColumnSpec`. `ColumnSpec::new(name, ty)` is the + unconstrained constructor. 110 call sites updated. +2. `a60e879` — the db layer honours the `ColumnSpec` + constraints: `column_constraints_sql`, `do_create_table` + emits `NOT NULL`/`UNIQUE`/`DEFAULT`; `ReadColumn` gained + `default_sql`; `schema_to_ddl` emits `DEFAULT` (so the + rebuild primitive preserves it); `ColumnDescription` + + `do_describe_table` (now sourced from `read_schema`) + + `constraints_display`. +3. `12395a9` — the `create table` constraint-suffix grammar + (`not null` / `unique` / `default `); + `build_create_table` collects per-column constraints; §9 + redundancy check; `project.yaml` round-trip + (`ColumnSchema` gained `not_null` / `default`). +4. `58d8958` — `add column` with the same suffix; the §6 + routing (`unique` / `not_null`-without-default route + through the rebuild primitive via + `do_add_constrained_column_via_rebuild`); §6 pre-flight + refusals. +5. `942222b` — `CHECK`. `check ( )` reuses the + ADR-0026 expression grammar via `Subgrammar`. The parsed + `Expr` is compiled once to inline SQL (`compile_check_sql`) + and stored in that form everywhere — a `check_expr` column + in `__rdbms_playground_columns`, `ColumnSchema.check`, the + column DDL. + +### Three decisions / discoveries this session (the ADR +### already reflects all three) + +1. **`create table … with pk …` makes *every* listed column + a primary-key column** — there is no simple-mode syntax + for a non-PK column at create time (recorded in + `docs/simple-mode-limitations.md`). Consequence: the + `create table` constraint suffix is realistically only + useful for `default` / `check`; `not null` / `unique` + there are §9 redundancy errors. The real home for + `not null` / `unique` is `add column` and (commit 5) + `add constraint`. +2. **`CHECK` is stored as compiled SQL, not DSL text** + (user-ratified; ADR §7/§8 updated). Double-quoted + identifiers, consistent with ADR-0028's `explain` display + SQL. No `Expr`→text renderer, no re-parser. +3. **A `CHECK` on a `serial`/`shortid` column is + create-table-only** — `add column … (serial) … check` is + refused with a friendly message (the auto-fill rebuild + path does not thread it). + +### Key reusable pieces (commits 1–4 built these) + +All in `src/dsl/grammar/ddl.rs` unless noted: + +- `COLUMN_CONSTRAINT` — the `Choice` over the four individual + constraint nodes (`NOT_NULL_CONSTRAINT`, `UNIQUE_CONSTRAINT`, + `DEFAULT_CONSTRAINT`, `CHECK_CONSTRAINT`). + `COLUMN_CONSTRAINT_SUFFIX` is `Repeated{min:0}` of it. The + `add constraint` command (commit 5) reuses the individual + nodes. +- `collect_column_constraints(path)` → `(bool, bool, + Option, Option)` — scans a single-column + path's constraint keywords. `consume_check_expr` — extracts + a `check ( … )` expression (paren-depth aware). +- `redundant_pk_constraint(column, constraint)` — the + ADR-0029 §9 friendly error. +- `src/db.rs`: `column_constraints_sql`, `default_sql_literal`, + `compile_check_sql`, `insert_column_metadata`, + `read_schema_for_specs`, `do_add_constrained_column_via_rebuild`. + `ReadColumn` carries `notnull` / `unique` / `default_sql` / + `check`; `schema_to_ddl` emits all four — **so the rebuild + primitive already round-trips every constraint.** + +## §4. ADR-0029 commits 5–6 — the plan + +### Commit 5 — `add constraint` / `drop constraint` (ADR §2.2, §5) + +The surface (the user chose `add constraint ` over a +bare `add `, for grammar-hierarchy uniformity): + +``` +add constraint not null to . +add constraint unique to . +add constraint default to . +add constraint check ( ) to . +drop constraint not null from . +drop constraint unique from . +drop constraint default from . +drop constraint check from . +``` + +**Grammar (`src/dsl/grammar/ddl.rs`):** add a `constraint` +form to the `ADD` and `DROP` command `Choice`s. The word +`constraint` after `add` / `drop` discriminates it (distinct +from `column` / `index` / `table` / `relationship` / `1`). +After `add constraint`, reuse the existing `COLUMN_CONSTRAINT` +Choice for the ``; after `drop constraint`, a +`Choice` of `not null` / `unique` / `default` / `check` +keyword-only nodes (no payload). The dotted `.` — +reuse the `Ident '.' Ident` shape from `add 1:n relationship +from

.` (same file). `build_add` / `build_drop` +discriminate forms by the word after the entry verb — add a +`Some("constraint")` arm to each. + +**AST (`src/dsl/command.rs`):** +```rust +Command::AddConstraint { table: String, column: String, constraint: Constraint } +Command::DropConstraint { table: String, column: String, kind: ConstraintKind } +pub enum Constraint { NotNull, Unique, Default(Value), Check(Expr) } +pub enum ConstraintKind { NotNull, Unique, Default, Check } +``` + +**GOTCHA — the two new `Command` variants break every +exhaustive `match Command`** (exactly ADR-0028's gotcha 3). +Add arms to: `Command::verb()` and `target_table()` +(`command.rs`); `execute_command_typed` (`runtime.rs`); +`build_translate_context` (`app.rs` — map to an `Operation`, +e.g. a new `Operation::AddConstraint` or reuse an existing +one); `command_kind_label` (`tests/typing_surface/mod.rs`). +Land grammar + AST + worker in **one commit** (a green +intermediate is impossible otherwise). + +**DB worker (`src/db.rs`):** `Request::AddConstraint` / +`DropConstraint`; `Database::add_constraint` / +`drop_constraint`; `do_add_constraint` / `do_drop_constraint`. +Both return `TableDescription` (the post-rebuild structure) → +route as `CommandOutcome::Schema(Some(desc))` → +`AppEvent::DslSucceeded` (the existing auto-show path; **no +new AppEvent needed**). + +`do_add_constraint` steps: +1. `read_schema(conn, table)`; find the target column. +2. **§9 redundancy** (execution-time, since the parser has no + schema): if the column is a PK column, reject `not null` + (always) and `unique` (single-column PK only) — see ADR §9. +3. **§5 dry-run** for `not null` / `unique` / `check` (skip + for `default`): scan the existing rows; refuse with a + learner-friendly pretty-table of offending rows if any. + *Mirror how `do_change_column_type` surfaces its ADR-0017 + dry-run refusal table* — that is the existing precedent + for "scan data, refuse with a table." `add not null` → + rows `WHERE col IS NULL`; `add unique` → non-NULL + duplicate groups; `add check` → rows `WHERE NOT ()`. +4. Build `new_schema` = the read schema with the target + `ReadColumn`'s field set (`notnull` / `unique` / + `default_sql` via `default_sql_literal` / `check` via + `compile_check_sql`), then `rebuild_table(...)`. The + `metadata_updates` closure updates the `check_expr` + metadata column when the constraint is a `CHECK` (the + other three are pragma-recoverable and need no metadata). + +`do_drop_constraint`: §9 check (cannot drop a PK-implied +`not null`, or `unique` from a single-column PK — friendly +error); build `new_schema` with the field cleared; rebuild. +No dry-run (removing a constraint cannot violate data). +`drop check` also NULLs the `check_expr` metadata. + +**Catalog:** `parse.usage.add_constraint` / +`parse.usage.drop_constraint` in `en-US.yaml` *and* +`keys.rs` (the `keys_validate_against_catalog` test enforces +the pair). The §5 refusal messages can be `DbError::Unsupported` +detail strings for now (consistent with §6's commit-4 +messages) or dedicated catalog keys. + +### Commit 6 — friendly errors + matrix + verification + +- **CHECK-violation friendly error** (ADR §10): an `insert` / + `update` that violates a `CHECK` makes SQLite report + `CHECK constraint failed`; add a `friendly`-layer catalog + entry translating it (`NOT NULL` / `UNIQUE` are already + enriched — see `runtime.rs` `enrich_dsl_failure`). +- **Typing-surface matrix cells** in `tests/typing_surface/` + for the constraint grammar — `create table … not null`, + `add column … check (…)`, `add constraint … to T.col`, + etc. Follow the matrix-snapshot discipline (handoff-17→21): + a new cell's snapshot is created with + `INSTA_UPDATE=always cargo test --test typing_surface_matrix + ` then reviewed before committing (`cargo insta` is + not installed on this machine). +- Full suite green vs. the 1201 baseline; clippy clean. +- Final handoff (handoff-23) and tick `C3` in + `docs/requirements.md`. + +## §5. How to take over + +1. **Read this file, then `docs/adr/0029-column-constraints.md`** + (the spec — current), then **handoff-21** (ADR-0028). +2. **Read `CLAUDE.md`** — working-style rules. +3. **Run `cargo test`** — 1201 passing, 0 failing, 1 ignored. +4. **Run `cargo clippy --all-targets -- -D warnings`** — clean. +5. **Implement ADR-0029 commit 5** per §4 — grammar + AST + + worker in one commit (the `match Command` breakage forces + it). Then commit 6. The §4 anchors and gotchas are this + session's; the dry-run precedent to copy is + `do_change_column_type`. + +### Note on mechanical churn + +Adding a field to `ColumnSpec` / `ColumnDescription` / +`ReadColumn` / `ColumnSchema` breaks 20–110 construction +sites, almost all in test code. This session delegated those +purely-mechanical sweeps to a general-purpose sub-agent +(`mode: bypassPermissions`) twice — it is the right tool for +that drudgery. Commit 5 adds no such field, so it should not +recur; but if commit 6 does, delegate it.