docs: 4f plan + session handoff 39
- docs/plans/20260525-adr-0035-sql-ddl-4f.md — ALTER TABLE ALTER COLUMN TYPE plan (/runda'd; forks resolved). Advanced lossy = ForceConversion (reuse do_change_column_type; existing transformed_lossy note); the internal-__rdbms_* guard folds into do_change_column_type both surfaces (user-confirmed); int->serial is ALLOWED (ADR-0018 §8), only non-int ->serial / blob / date<->datetime are static-refused. - docs/handoff/20260525-handoff-39.md — 4d/4e shipped; 4f is next (plan ready).
This commit is contained in:
claude@clouddev1
@@ -0,0 +1,256 @@
|
||||
# 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.
|
||||
Reference in New Issue
Block a user