rdbms-playground/docs/plans/20260525-adr-0035-sql-ddl-4f.md

# Plan: ADR-0035 Phase 4, sub-phase 4f — `ALTER TABLE … ALTER COLUMN TYPE`

Add `ALTER TABLE <T> ALTER COLUMN <col> TYPE <type>` as a fourth
`AlterTableAction` (`AlterColumnType`), runtime-decomposed to the
existing `do_change_column_type` executor with the **advanced-mode
policy** (ADR-0035 §7): lossy conversions are **performed with a note**,
no force flag; static-refused (`→serial`, `↔blob`) and incompatible
conversions still refuse (ADR-0017, shared by both modes). This is the
smallest 4e-style slice — almost entirely reuse.

The single new executor change is folding the internal-`__rdbms_*` guard
into `do_change_column_type` (the 4e follow-up), so the **simple `change
column` surface** is closed too.

## 1. Baseline (at handoff)

- Tests: **1854 passing, 0 failing, 0 skipped, 1 ignored**; clippy clean.
  Branch `main`, HEAD `bbc2e34` (4e). 4f starts here. (4a–4e are
  local-only commits since `origin/main`.)

## 2. Decisions (settled — ADR §7/§13 4f + user-confirmed 2026-05-25)

1. **Advanced lossy = `ChangeColumnMode::ForceConversion`** — no new
   mode, no new note. `ForceConversion` already *is* the §7 advanced
   policy: `static_refusal` refuses `→serial`/`↔blob`; the dry-run
   refuses incompatible cells; lossy cells are transformed; the
   `ClientSideNote { transformed, lossy }` is surfaced. The existing
   `client_side.transformed_lossy` catalog note (*"N row(s) transformed;
   M of those discarded information (lossy)…"*) is engine-neutral and
   reads correctly for advanced — it is the §7 "N values converted with
   loss" note. (Update the stale `// fires under --force-conversion`
   comment in `en-US.yaml` to mention advanced ALTER COLUMN TYPE too.)
2. **No SQL spelling for `--force-conversion` / `--dont-convert`**
   (ADR §7/§12). Advanced always performs the conversion (relying on
   `undo` as the safety net — a payoff of shipping ADR-0006 first). The
   Postgres `USING <expr>` clause is **not** adopted (§12).
3. **Static-refused + incompatible reuse the executor** (ADR-0017,
   shared by both modes). **Verified `static_refusal` (type_change.rs)
   refuses:** same-type (`already X`); **non-int → `serial`** (only
   `int → serial` is allowed); any `↔ blob`; direct `date ↔ datetime`
   (route via text); and any pair not in the matrix. An *incompatible*
   per-cell conversion → the existing diagnostic. **NOTE — ADR-0035 §7's
   "static-refused (→serial …)" summary is looser than the code:
   `int → serial` IS allowed** (ADR-0018 §8 — auto-fills null cells with
   generated values, adds UNIQUE if non-PK), so advanced
   `ALTER COLUMN c TYPE serial` on an `int` column is a *supported*
   operation, not a refusal. The implementer must test `int → serial`
   (allowed) and `text → serial` / `↔ blob` (refused) — do NOT "fix"
   int→serial to refuse.
4. **`TYPE` keyword only** — `ALTER COLUMN <col> TYPE <type>` (ADR §4).
   The Postgres `SET DATA TYPE` synonym is **not** added (kept minimal;
   could be a later alias).
5. **Internal-`__rdbms_*` guard folded into `do_change_column_type`**
   (user-confirmed 2026-05-25) — closes the simple `change column`
   exposure alongside the parse-time guard the SQL ALTER table slot
   already gives. `do_add_constraint` / `do_add_relationship` stay
   flagged for **4g**.

## 3. Phase 1 — Requirements checklist (4f)

Grammar / dispatch:
- [ ] `ALTER TABLE <T> ALTER COLUMN <col> TYPE <type>` parses in
  advanced mode → `SqlAlterTable { AlterColumnType { column, ty } }`;
  `<type>` accepts the SQL type vocabulary + aliases (reuse `SQL_TYPE`).
- [ ] The action `Choice` now has **four** branches (add/drop/rename/
  alter-column), each leading on a distinct concrete keyword — trap-safe.
  The other three still parse; `alter` remains advanced-only; the table
  slot still rejects `__rdbms_*` at parse.
- [ ] Trailing `;` tolerated.

Execution (reuse `do_change_column_type` with `ForceConversion`):
- [ ] **Clean** conversion (e.g. `int → text`) auto-converts; one undo
  step; auto-show.
- [ ] **Lossy** conversion (e.g. `real → int`, `3.7 → 3`) is **performed**
  and surfaces the `[client-side] … discarded information (lossy)` note
  (no force flag, no refusal) — the §7 advanced behaviour.
- [ ] **Incompatible** conversion is refused (friendly, engine-neutral)
  — e.g. `text → int` where a cell holds a non-numeric string.
- [ ] **Static-refused** refused: `text → serial` (non-int source),
  `↔ blob`, `date ↔ datetime` direct, same-type. **And** `int → serial`
  is **allowed** (auto-fills nulls, adds UNIQUE — ADR-0018 §8): test it
  as a supported conversion, not a refusal.
- [ ] FK-involved column type change refuses per the existing executor
  rules (outbound/inbound), via the SQL path too.
- [ ] The converted column round-trips and **survives rebuild** (the
  `user_type` metadata update is the existing path).

Internal-table guard (both surfaces):
- [ ] `do_change_column_type` refuses an internal `__rdbms_*` table (as
  "no such table"); the simple `change column` surface is closed. A test
  on the simple surface.

### Testing
- [ ] **Tier 1** (extend `sql_alter_table_tests` in `ddl.rs`): the
  alter-column-type parse (incl. a type alias) → `AlterColumnType`; the
  four-branch dispatch still routes add/drop/rename correctly.
- [ ] **Tier 3** (extend `tests/sql_alter_table.rs`, via `run_replay`):
  clean convert (`int → text`); lossy convert (`real → int`) performs —
  assert the value was converted (e.g. `3.7` stored as `3`); incompatible
  refused; `text → serial` (and/or `↔ blob`) refused; **`int → serial`
  allowed** (auto-fill); rebuild survival; one undo step. (Assert the
  lossy *data* conversion as the primary signal; the `[client-side] …
  lossy` note is rendered through the shared ChangeColumn path.)
- [ ] **Internal guard** (extend `tests/column_op_guards.rs`): simple
  `change column` on `__rdbms_*` refused.
- [ ] **Catalog** lockstep + vocab audit for the refreshed
  `sql_alter_table` help/usage (it now lists the alter-column-type form).

## 4. Architecture & design

### 4.1 Command (`src/dsl/command.rs`)
- `AlterTableAction` gains `AlterColumnType { column: String, ty: Type }`
  (small variant — no boxing needed; the boxed `AddColumn` already
  dominates the enum's size).

### 4.2 Grammar (`src/dsl/grammar/ddl.rs`)
- New action branch `AT_ALTER_COLUMN = Seq[ Word("alter"),
  Word("column"), COLUMN_NAME, Word("type"),
  super::sql_create_table::SQL_TYPE ]`. Add it to `AT_ACTION_CHOICES`
  (now `[AT_ADD_COLUMN, AT_DROP_COLUMN, AT_RENAME_COLUMN,
  AT_ALTER_COLUMN]`). Leads on the concrete `alter` keyword (distinct
  from add/drop/rename) — trap-safe. The action's `alter` is the 4th
  token (the entry-word `alter` is consumed by dispatch first).
- `build_sql_alter_table`: add a branch. Discriminate on the **`type`
  keyword** (`path.contains_word("type")`) — unique to ALTER COLUMN TYPE
  (ADD COLUMN's type is a `col_type` *ident*, not the literal `type`
  keyword). Extract `column = require_ident("column_name")` and the
  target `Type` from the `col_type` ident (`Type::from_sql_name`) /
  the `double precision` two-word case — mirror the type-extraction in
  `build_alter_add_column_spec`. **Order the `type` check before
  add/rename/drop.**

### 4.3 Worker / executor (`src/db.rs`)
- **No new method.** Add the internal-table guard:
  `reject_internal_table_name(table)?;` at the top of
  `do_change_column_type` (mirrors the 4e column executors).
- `ForceConversion`'s lossy path + `ClientSideNote` are unchanged.

### 4.4 Runtime (`src/runtime.rs`)
- Extend the `SqlAlterTable` action match:
  `AlterColumnType { column, ty } => database.change_column_type(table,
  column, ty, ChangeColumnMode::ForceConversion, src).await
  .map(CommandOutcome::ChangeColumn)` — the same outcome the simple
  `change column` uses (its event/app rendering surfaces the client-side
  lossy note).

### 4.5 app.rs failure-translate + typing_surface
- `app.rs build_translate_context`: the `SqlAlterTable` arm's inner
  `match action` gains `AlterColumnType { column, .. } =>
  (Operation::ChangeColumnType, Some(table), Some(column))`.
- `typing_surface` already labels `SqlAlterTable`; no change (the action
  is internal to the variant).

### 4.6 Catalog (`keys.rs` + `en-US.yaml`)
- **Refresh** the existing `help.ddl.sql_alter_table` +
  `parse.usage.sql_alter_table` bodies to add the
  `alter table <T> alter column <col> type <type>` line. No new keys.
- Update the stale `client_side` `// fires under --force-conversion`
  comment to note advanced ALTER COLUMN TYPE also triggers the lossy
  note (comment only).

## 5. Phase 2 — Candidate approaches

**(M1) Reuse `do_change_column_type` with `ForceConversion`** *(lead)* —
the mode already encodes the §7 advanced policy exactly; the lossy note
already exists and is engine-neutral. Zero executor logic beyond the
internal-table guard.
**(M2) Add a new `ChangeColumnMode::Advanced` variant** — *rejected.*
It would duplicate `ForceConversion`'s behaviour; the only conceivable
difference (note wording) is already neutral and correct. No new variant
earns its keep. (If a future need to distinguish "user forced" from
"advanced auto" arises — e.g. for telemetry — revisit then.)

**(G1) A fourth action branch leading on `alter`, discriminated in the
builder by the `type` keyword** *(lead)* — consistent with the three
4e branches; `type` is an unambiguous discriminator.
**(G2) A separate `SQL_ALTER_COLUMN` command/node** — *rejected.* The
`AlterTableAction` enum is the established extension point (built for
this in 4e); a separate command would fork the `alter table` dispatch.

## 6. Phase 3 — Selection vs the checklist

M1 + G1 satisfy every §3 item: parse (G1 four-branch), the §7 tiers
(clean/lossy/incompatible/static-refused — all from the reused executor
+ `ForceConversion`), the lossy note (existing `transformed_lossy`),
rebuild survival + undo parity (the executor's existing rebuild path),
the internal-table guard (one-line add, both surfaces). No requirement
unmet, no new model/persistence. **Selected: M1 + G1.**

## 7. Devil's Advocate review of this plan

- **Fork escalated?** The only 4f fork — the internal-table guard on
  `do_change_column_type` — was put to the user (2026-05-25, "fix in 4f,
  both surfaces"). The §7 lossy policy and the no-force-flag/`USING`
  exclusions are ADR-settled, not autonomous. ✓
- **Is `ForceConversion` truly the §7 advanced policy? — verified in
  code.** `run_change_column_with_dry_run`: incompatible cells refuse in
  both modes; lossy cells refuse **only when `mode != ForceConversion`**
  (db.rs:4575) — so under `ForceConversion` they are performed and
  counted (`ClientSideNote.lossy`), firing `client_side.transformed_lossy`
  (engine-neutral, no `--force-conversion` in the rendered text).
  `static_refusal` was read in full (the §2.3 list) — **correcting the
  plan's first draft**: `int → serial` is *allowed*, not refused. Matches
  the §7 intent once that nuance is captured. ✓
- **Grammar trap / discriminator?** Four concrete-keyword-led branches;
  the builder keys on the `type` keyword (unique — ADD COLUMN's type is
  an ident). A parse test for all four branches + the alias case is on
  the checklist. The implementer must **probe** the `type`-keyword
  discriminator and the SQL_TYPE extraction (mirror
  `build_alter_add_column_spec`) — low-risk, 4e-proven patterns. ✓
- **Lossy note actually fires on the SQL path?** The e2e (`run_replay`)
  lossy test asserts it; the note path is `CommandOutcome::ChangeColumn`
  → the existing app rendering, shared with simple `change column`. ✓
- **Undo parity?** One `change_column_type` call = one snapshot (the
  executor's existing rebuild is one undo step). A per-action undo check
  in the e2e. ✓
- **Anything dropped?** `SET DATA TYPE` synonym and the `--force/--dont`
  SQL spellings are *deliberately* out (stated, ADR-backed). The broader
  internal-table guard (`do_add_constraint` / `do_add_relationship`)
  stays a **4g** follow-up — flagged, not silent. ✓
- **Help/usage refresh?** The `sql_alter_table` skeleton is *refreshed*
  (existing node, existing keys) to list the new form — not deferred to
  4i (that deferral is only for the *CREATE TABLE* skeleton). ✓

## 8. Out of 4f scope (tracked, not dropped)
- `ALTER TABLE` add/drop constraint + add FK (**4g**) — and the internal
  guard on `do_add_constraint` / `do_add_relationship` rides there.
- `ALTER TABLE … RENAME TO` table rename (**4h**).
- `SET DATA TYPE` synonym; the Postgres `USING <expr>` clause; any SQL
  spelling of `--force-conversion` / `--dont-convert` (ADR §7/§12).
- 4i verification sweep (the running deferral tail).

## 9. Implementation sequence (test-first)
1. **Internal-table guard (isolated, both surfaces).**
   `reject_internal_table_name` at the top of `do_change_column_type`;
   a Tier-3 test via the simple `change column` (`column_op_guards.rs`)
   → green. Lands the latent-bug fix on its own.
2. **Command + grammar + builder.** `AlterColumnType` variant; the
   `AT_ALTER_COLUMN` branch + the builder discriminator; Tier-1 parse +
   four-branch dispatch tests (probe the `type` discriminator first) →
   the exhaustive arms (app.rs) → green (parse only).
3. **Runtime + catalog.** Wire `AlterColumnType` →
   `change_column_type(…, ForceConversion)`; refresh the
   `sql_alter_table` help/usage; Tier-3 (`sql_alter_table.rs` via
   `run_replay`): clean / lossy+note / incompatible / static-refused /
   rebuild / undo → green.
4. **Full sweep** — `cargo test` (no regression from 1854) + `cargo
   clippy --all-targets -- -D warnings`.
5. **Docs** — ADR-0035 Status + §13 4f implemented; README; requirements
   Q1. Run `/runda` over the finished slice. Propose commit; wait for
   approval.

## 10. Exit gate
- All §3 items satisfied; four tiers green, zero skips; no regression
  from 1854; `/runda` / written-DA PASS; clippy clean; ADR-0035 §13 4f +
  README + requirements.md lockstep.