From 7b97786ab7be002bfc55155bcfbd6dc9379ce084 Mon Sep 17 00:00:00 2001 From: "claude@clouddev1" Date: Fri, 8 May 2026 10:09:24 +0000 Subject: [PATCH] B2/C2: column drop / rename / change-type DSL commands Closes B2 (rebuild-table reused outside relationships) and C2 (full add/drop/rename/change-type column operations). * drop column [from] [table] : - ALTER TABLE DROP COLUMN (SQLite 3.35+) + metadata cleanup in __rdbms_playground_columns. - Refuses PK columns and columns involved in a declared relationship (drop the relationship first). * rename column [in] [table] : to - ALTER TABLE RENAME COLUMN (SQLite 3.25+); SQLite cascades the rename through FK declarations on other tables. - Mirrors the new name into both metadata tables (__rdbms_playground_columns, __rdbms_playground_relationships) so describes stay accurate after a rename. - Refuses identity rename and name collisions. * change column [in] [table] : () - Routes through the rebuild_table primitive (ADR-0013) since SQLite ALTER doesn't support type changes. INSERT INTO new SELECT FROM old; STRICT typing enforces cell compatibility, transaction rolls back on mismatch. - Refuses PK columns, relationship-involved columns, `serial` target, and no-op same-type changes. Adds 20 tests (parser + db layer); updates the in-app help listing. Both prepositions independently optional in each new command, matching `add column`'s grammar shape. Total: 449 passing, 0 failing, 0 skipped (up from 429). Clippy clean. Known spec gap: column-type-change conversion compatibility is not yet documented (currently relies on SQLite STRICT errors); follow-up will close this. --- docs/requirements.md | 34 +- src/app.rs | 3 + src/db.rs | 722 +++++++++++++++++++++++++++++++++++++++++++ src/dsl/command.rs | 35 +++ src/dsl/parser.rs | 176 ++++++++++- src/runtime.rs | 12 + 6 files changed, 962 insertions(+), 20 deletions(-) diff --git a/docs/requirements.md b/docs/requirements.md index 89e4f6e..d81565e 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -26,10 +26,9 @@ repo is pushed). ## Test baseline -After Iterations 5 + 6 (export/import + --resume + persistent -input history + migration scaffold): **408 passing, 0 failing, -0 skipped** (`cargo test`). Clippy clean with the nursery -lint group enabled. +After B2/C2 (column drop / rename / change-type): **449 +passing, 0 failing, 0 skipped** (`cargo test`). Clippy clean +with the nursery lint group enabled. --- @@ -112,11 +111,14 @@ lint group enabled. - [ ] **C1** Table operations: create / drop / rename. *(Progress: create + drop done; rename pending.)* -- [ ] **C2** Column operations: add / drop / rename / change - type, including the rebuild-table dance behind the scenes - where SQLite ALTER cannot do it directly. - *(Progress: add done; drop/rename/change-type pending — the - rebuild-table dance is the gating piece, B2.)* +- [x] **C2** Column operations: add / drop / rename / change + type. `drop column` and `rename column` use SQLite native + ALTER TABLE (3.35+ / 3.25+); `change column` routes through + the rebuild-table primitive since ALTER doesn't support + type changes. PK and relationship-involved columns are + refused with friendly messages (drop the relationship + first); SQLite STRICT enforces type compatibility on the + data copy during a type change. - [ ] **C3** Schema constraints: primary key (single and compound), foreign key with `ON DELETE` / `ON UPDATE` referential actions, indexes, `NOT NULL`, `UNIQUE`, `CHECK`, `DEFAULT`. @@ -166,12 +168,14 @@ lint group enabled. - [x] **B1** SQLite via `rusqlite`; all tables created `STRICT`; `PRAGMA foreign_keys = ON` per connection. *(Database accessed through a dedicated worker thread per ADR-0010.)* -- [ ] **B2** Schema evolution uses the rebuild-table technique - internally where SQLite `ALTER TABLE` cannot. - *(Progress: rebuild-table primitive landed (ADR-0013) and is - used by `add_relationship` / `drop_relationship`. Reuse for - column drops/renames/type changes pending; the primitive is - designed to support those without further architectural work.)* +- [x] **B2** Schema evolution uses the rebuild-table technique + internally where SQLite `ALTER TABLE` cannot — currently + the change-column-type code path. Add-column, drop-column, + and rename-column take the simpler ALTER TABLE route since + modern SQLite supports them natively; metadata sync into + `__rdbms_playground_columns` and + `__rdbms_playground_relationships` happens in the same + transaction either way. - [ ] **B3** Query timeout and cancellation supported (no cartesian-join-of-doom can hang the app). *(Progress: the worker-thread architecture is in place; the diff --git a/src/app.rs b/src/app.rs index fd296cf..1f50969 100644 --- a/src/app.rs +++ b/src/app.rs @@ -1109,6 +1109,9 @@ impl App { " create table with pk [:...]", " drop table ", " add column [to] [table] : ()", + " drop column [from] [table] : ", + " rename column [in] [table] : to ", + " change column [in] [table] : ()", " add 1:n relationship [as ] from

. to .", " [on delete ] [on update ] [--create-fk]", " drop relationship ", diff --git a/src/db.rs b/src/db.rs index cb2c1c9..4d18107 100644 --- a/src/db.rs +++ b/src/db.rs @@ -278,6 +278,26 @@ enum Request { source: Option, reply: oneshot::Sender>, }, + DropColumn { + table: String, + column: String, + source: Option, + reply: oneshot::Sender>, + }, + RenameColumn { + table: String, + old: String, + new: String, + source: Option, + reply: oneshot::Sender>, + }, + ChangeColumnType { + table: String, + column: String, + ty: Type, + source: Option, + reply: oneshot::Sender>, + }, ListTables { reply: oneshot::Sender, DbError>>, }, @@ -430,6 +450,61 @@ impl Database { recv.await.map_err(|_| DbError::WorkerGone)? } + pub async fn drop_column( + &self, + table: String, + column: String, + source: Option, + ) -> Result { + let (reply, recv) = oneshot::channel(); + self.send(Request::DropColumn { + table, + column, + source, + reply, + }) + .await?; + recv.await.map_err(|_| DbError::WorkerGone)? + } + + pub async fn rename_column( + &self, + table: String, + old: String, + new: String, + source: Option, + ) -> Result { + let (reply, recv) = oneshot::channel(); + self.send(Request::RenameColumn { + table, + old, + new, + source, + reply, + }) + .await?; + recv.await.map_err(|_| DbError::WorkerGone)? + } + + pub async fn change_column_type( + &self, + table: String, + column: String, + ty: Type, + source: Option, + ) -> Result { + let (reply, recv) = oneshot::channel(); + self.send(Request::ChangeColumnType { + table, + column, + ty, + source, + reply, + }) + .await?; + recv.await.map_err(|_| DbError::WorkerGone)? + } + pub async fn list_tables(&self) -> Result, DbError> { let (reply, recv) = oneshot::channel(); self.send(Request::ListTables { reply }).await?; @@ -725,6 +800,52 @@ fn handle_request(conn: &Connection, persistence: Option<&Persistence>, req: Req ty, )); } + Request::DropColumn { + table, + column, + source, + reply, + } => { + let _ = reply.send(do_drop_column( + conn, + persistence, + source.as_deref(), + &table, + &column, + )); + } + Request::RenameColumn { + table, + old, + new, + source, + reply, + } => { + let _ = reply.send(do_rename_column( + conn, + persistence, + source.as_deref(), + &table, + &old, + &new, + )); + } + Request::ChangeColumnType { + table, + column, + ty, + source, + reply, + } => { + let _ = reply.send(do_change_column_type( + conn, + persistence, + source.as_deref(), + &table, + &column, + ty, + )); + } Request::ListTables { reply } => { let _ = reply.send(do_list_tables(conn)); } @@ -1257,6 +1378,291 @@ fn do_add_column( Ok(description) } +/// Drop a column from a table. +/// +/// Uses SQLite's native `ALTER TABLE … DROP COLUMN` +/// (available since SQLite 3.35) so we get the engine's +/// constraint checks for free; SQLite refuses if the column +/// is part of the PK, has a UNIQUE constraint, is referenced +/// in a CHECK, or is used in an FK. In addition we run two +/// up-front checks so the user gets friendly messages +/// before SQLite refuses: +/// +/// - Refuse PK columns (the dominant case the user might +/// try). +/// - Refuse columns involved in a declared relationship +/// (per `__rdbms_playground_relationships`). Drop the +/// relationship first. +fn do_drop_column( + conn: &Connection, + persistence: Option<&Persistence>, + source: Option<&str>, + table: &str, + column: &str, +) -> Result { + let schema = read_schema(conn, table)?; + let col_info = schema + .columns + .iter() + .find(|c| c.name == column) + .ok_or_else(|| DbError::Sqlite { + message: format!("no such column: {table}.{column}"), + kind: SqliteErrorKind::NoSuchColumn, + })?; + if col_info.primary_key { + return Err(DbError::Unsupported(format!( + "cannot drop primary-key column `{table}.{column}`. \ + Drop the table or change the primary key first." + ))); + } + let rel_count: i64 = conn + .query_row( + &format!( + "SELECT COUNT(*) FROM {REL_TABLE} \ + WHERE (parent_table = ?1 AND parent_column = ?2) \ + OR (child_table = ?1 AND child_column = ?2);" + ), + rusqlite::params![table, column], + |row| row.get(0), + ) + .map_err(DbError::from_rusqlite)?; + if rel_count > 0 { + return Err(DbError::Unsupported(format!( + "cannot drop `{table}.{column}` while a relationship \ + references it; drop the relationship first." + ))); + } + + let tx = conn + .unchecked_transaction() + .map_err(DbError::from_rusqlite)?; + let ddl = format!( + "ALTER TABLE {tbl} DROP COLUMN {col};", + tbl = quote_ident(table), + col = quote_ident(column), + ); + debug!(ddl = %ddl, "drop_column"); + tx.execute_batch(&ddl).map_err(DbError::from_rusqlite)?; + tx.execute( + &format!( + "DELETE FROM {META_TABLE} WHERE table_name = ?1 AND column_name = ?2;" + ), + [table, column], + ) + .map_err(DbError::from_rusqlite)?; + let description = do_describe_table(conn, table)?; + let changes = Changes { + schema_dirty: true, + rewritten_tables: vec![table.to_string()], + ..Changes::default() + }; + finalize_persistence(conn, persistence, source, &changes)?; + tx.commit().map_err(DbError::from_rusqlite)?; + Ok(description) +} + +/// Rename a column. +/// +/// Uses SQLite's native `ALTER TABLE … RENAME COLUMN` +/// (available since SQLite 3.25), which automatically +/// updates references in views, triggers, and FK +/// declarations on other tables. We mirror the rename into +/// our two metadata tables so they don't drift. +fn do_rename_column( + conn: &Connection, + persistence: Option<&Persistence>, + source: Option<&str>, + table: &str, + old: &str, + new: &str, +) -> Result { + let schema = read_schema(conn, table)?; + if !schema.columns.iter().any(|c| c.name == old) { + return Err(DbError::Sqlite { + message: format!("no such column: {table}.{old}"), + kind: SqliteErrorKind::NoSuchColumn, + }); + } + if old == new { + // Nothing to do; refusing keeps behaviour + // predictable rather than appearing to "succeed" + // with no effect. + return Err(DbError::Unsupported(format!( + "rename: new name is identical to the existing one (`{old}`)." + ))); + } + if schema.columns.iter().any(|c| c.name == new) { + return Err(DbError::Unsupported(format!( + "column `{table}.{new}` already exists; pick a different name." + ))); + } + + let tx = conn + .unchecked_transaction() + .map_err(DbError::from_rusqlite)?; + let ddl = format!( + "ALTER TABLE {tbl} RENAME COLUMN {old_c} TO {new_c};", + tbl = quote_ident(table), + old_c = quote_ident(old), + new_c = quote_ident(new), + ); + debug!(ddl = %ddl, "rename_column"); + tx.execute_batch(&ddl).map_err(DbError::from_rusqlite)?; + + // Mirror the rename into __rdbms_playground_columns. + tx.execute( + &format!( + "UPDATE {META_TABLE} SET column_name = ?1 \ + WHERE table_name = ?2 AND column_name = ?3;" + ), + [new, table, old], + ) + .map_err(DbError::from_rusqlite)?; + // Mirror into __rdbms_playground_relationships on + // BOTH sides — a column may be the parent endpoint or + // the child endpoint (or, for self-referencing tables, + // both). + tx.execute( + &format!( + "UPDATE {REL_TABLE} SET parent_column = ?1 \ + WHERE parent_table = ?2 AND parent_column = ?3;" + ), + [new, table, old], + ) + .map_err(DbError::from_rusqlite)?; + tx.execute( + &format!( + "UPDATE {REL_TABLE} SET child_column = ?1 \ + WHERE child_table = ?2 AND child_column = ?3;" + ), + [new, table, old], + ) + .map_err(DbError::from_rusqlite)?; + + let description = do_describe_table(conn, table)?; + let changes = Changes { + schema_dirty: true, + rewritten_tables: vec![table.to_string()], + ..Changes::default() + }; + finalize_persistence(conn, persistence, source, &changes)?; + tx.commit().map_err(DbError::from_rusqlite)?; + Ok(description) +} + +/// Change a column's type. +/// +/// SQLite's `ALTER TABLE` cannot change column types, so +/// this routes through the rebuild-table primitive +/// (ADR-0013). The new schema differs from the old in +/// exactly one column's `sqlite_type` and `user_type`; data +/// copies via `INSERT INTO new SELECT FROM old`. STRICT +/// typing means any cell value that doesn't fit the new +/// type triggers a SQLite error and the transaction rolls +/// back. +/// +/// V1 refuses two cases that would require cascading +/// changes outside the local table: +/// +/// - PK columns (FKs in other tables target this column; +/// changing its type would invalidate their target +/// types per ADR-0011). +/// - Columns involved in a declared relationship +/// (parent or child side) — drop the relationship +/// first. +/// +/// The `serial` type is also refused as a target since +/// `serial` carries auto-increment semantics that only +/// apply at create-table time. +fn do_change_column_type( + conn: &Connection, + persistence: Option<&Persistence>, + source: Option<&str>, + table: &str, + column: &str, + ty: Type, +) -> Result { + if ty == Type::Serial { + return Err(DbError::Unsupported( + "the 'serial' type carries auto-increment primary-key semantics \ + that only apply at create-table time; pick a different target \ + type (e.g. `int`)." + .to_string(), + )); + } + let old_schema = read_schema(conn, table)?; + let col_info = old_schema + .columns + .iter() + .find(|c| c.name == column) + .ok_or_else(|| DbError::Sqlite { + message: format!("no such column: {table}.{column}"), + kind: SqliteErrorKind::NoSuchColumn, + })?; + if col_info.primary_key { + return Err(DbError::Unsupported(format!( + "cannot change the type of primary-key column `{table}.{column}` \ + in v1; PK type changes would cascade to FK target types in \ + other tables." + ))); + } + let rel_count: i64 = conn + .query_row( + &format!( + "SELECT COUNT(*) FROM {REL_TABLE} \ + WHERE (parent_table = ?1 AND parent_column = ?2) \ + OR (child_table = ?1 AND child_column = ?2);" + ), + rusqlite::params![table, column], + |row| row.get(0), + ) + .map_err(DbError::from_rusqlite)?; + if rel_count > 0 { + return Err(DbError::Unsupported(format!( + "cannot change type of `{table}.{column}` while a relationship \ + references it; drop the relationship first." + ))); + } + if col_info.user_type == Some(ty) { + return Err(DbError::Unsupported(format!( + "column `{table}.{column}` is already `{ty}`." + ))); + } + + // Build new_schema: same as old, but the target column's + // user_type / sqlite_type are updated. PK and notnull + // flags carry over unchanged. + let mut new_schema = old_schema.clone(); + let target = new_schema + .columns + .iter_mut() + .find(|c| c.name == column) + .expect("column existence checked above"); + target.user_type = Some(ty); + target.sqlite_type = ty.sqlite_strict_type().to_string(); + + let ty_keyword = ty.keyword(); + rebuild_table(conn, table, &old_schema, &new_schema, |tx| { + tx.execute( + &format!( + "UPDATE {META_TABLE} SET user_type = ?1 \ + WHERE table_name = ?2 AND column_name = ?3;" + ), + [ty_keyword, table, column], + ) + .map_err(DbError::from_rusqlite)?; + let changes = Changes { + schema_dirty: true, + rewritten_tables: vec![table.to_string()], + ..Changes::default() + }; + finalize_persistence(tx, persistence, source, &changes)?; + Ok(()) + })?; + + do_describe_table(conn, table) +} + fn do_list_tables(conn: &Connection) -> Result, DbError> { let mut stmt = conn .prepare( @@ -3040,6 +3446,322 @@ mod tests { } } + // --- drop_column / rename_column / change_column_type --- + + #[tokio::test] + async fn drop_column_removes_column_and_data() { + let db = db(); + make_id_table(&db, "T").await; + db.add_column("T".to_string(), "Score".to_string(), Type::Int, None) + .await + .unwrap(); + db.insert( + "T".to_string(), + None, + vec![Value::Number("42".to_string())], + None, + ) + .await + .unwrap(); + + let desc = db + .drop_column("T".to_string(), "Score".to_string(), None) + .await + .unwrap(); + let names: Vec<_> = desc.columns.iter().map(|c| c.name.as_str()).collect(); + assert_eq!(names, vec!["id"]); + + // Row data still accessible (id was preserved); the + // dropped column is gone from the projection. + let data = db.query_data("T".to_string(), None).await.unwrap(); + assert_eq!(data.columns, vec!["id".to_string()]); + assert_eq!(data.rows.len(), 1); + } + + #[tokio::test] + async fn drop_column_refuses_primary_key() { + let db = db(); + make_id_table(&db, "T").await; + let err = db + .drop_column("T".to_string(), "id".to_string(), None) + .await + .unwrap_err(); + assert!(matches!(err, DbError::Unsupported(_)), "got {err:?}"); + let msg = format!("{err}"); + assert!(msg.to_lowercase().contains("primary"), "{msg}"); + } + + #[tokio::test] + async fn drop_column_refuses_column_in_a_relationship() { + let db = db(); + // Customers(id PK) ← Orders(cust_id FK) + make_id_table(&db, "Customers").await; + make_id_table(&db, "Orders").await; + db.add_column( + "Orders".to_string(), + "cust_id".to_string(), + Type::Int, + None, + ) + .await + .unwrap(); + db.add_relationship( + None, + "Customers".to_string(), + "id".to_string(), + "Orders".to_string(), + "cust_id".to_string(), + ReferentialAction::NoAction, + ReferentialAction::NoAction, + false, + None, + ) + .await + .unwrap(); + // Try to drop the FK column on the child side. + let err = db + .drop_column("Orders".to_string(), "cust_id".to_string(), None) + .await + .unwrap_err(); + assert!(matches!(err, DbError::Unsupported(_)), "got {err:?}"); + assert!(format!("{err}").contains("relationship")); + } + + #[tokio::test] + async fn drop_column_for_missing_column_errors() { + let db = db(); + make_id_table(&db, "T").await; + let err = db + .drop_column("T".to_string(), "Ghost".to_string(), None) + .await + .unwrap_err(); + match err { + DbError::Sqlite { kind, .. } => assert_eq!(kind, SqliteErrorKind::NoSuchColumn), + other => panic!("unexpected error: {other:?}"), + } + } + + #[tokio::test] + async fn rename_column_updates_schema_and_metadata() { + let db = db(); + make_id_table(&db, "T").await; + db.add_column("T".to_string(), "Old".to_string(), Type::Text, None) + .await + .unwrap(); + let desc = db + .rename_column("T".to_string(), "Old".to_string(), "New".to_string(), None) + .await + .unwrap(); + let names: Vec<_> = desc.columns.iter().map(|c| c.name.as_str()).collect(); + assert_eq!(names, vec!["id", "New"]); + // user_type metadata was preserved through the rename. + let new_col = desc.columns.iter().find(|c| c.name == "New").unwrap(); + assert_eq!(new_col.user_type, Some(Type::Text)); + } + + #[tokio::test] + async fn rename_column_propagates_to_relationship_metadata() { + let db = db(); + make_id_table(&db, "Customers").await; + make_id_table(&db, "Orders").await; + db.add_column( + "Orders".to_string(), + "cust_id".to_string(), + Type::Int, + None, + ) + .await + .unwrap(); + db.add_relationship( + None, + "Customers".to_string(), + "id".to_string(), + "Orders".to_string(), + "cust_id".to_string(), + ReferentialAction::NoAction, + ReferentialAction::NoAction, + false, + None, + ) + .await + .unwrap(); + // Rename on the child side; SQLite cascades the FK + // declaration, and we mirror that into our metadata. + db.rename_column( + "Orders".to_string(), + "cust_id".to_string(), + "buyer_id".to_string(), + None, + ) + .await + .unwrap(); + let orders = db + .describe_table("Orders".to_string(), None) + .await + .unwrap(); + let outbound = orders + .outbound_relationships + .iter() + .find(|r| r.local_column == "buyer_id"); + assert!( + outbound.is_some(), + "expected outbound rel on `buyer_id`, got {:?}", + orders.outbound_relationships, + ); + + // Same from the parent perspective via inbound. + let customers = db + .describe_table("Customers".to_string(), None) + .await + .unwrap(); + let inbound = customers + .inbound_relationships + .iter() + .find(|r| r.other_column == "buyer_id"); + assert!( + inbound.is_some(), + "expected inbound rel referencing `buyer_id`, got {:?}", + customers.inbound_relationships, + ); + } + + #[tokio::test] + async fn rename_column_refuses_collision() { + let db = db(); + make_id_table(&db, "T").await; + db.add_column("T".to_string(), "A".to_string(), Type::Text, None) + .await + .unwrap(); + db.add_column("T".to_string(), "B".to_string(), Type::Text, None) + .await + .unwrap(); + let err = db + .rename_column("T".to_string(), "A".to_string(), "B".to_string(), None) + .await + .unwrap_err(); + assert!(matches!(err, DbError::Unsupported(_)), "got {err:?}"); + } + + #[tokio::test] + async fn rename_column_refuses_identity_rename() { + let db = db(); + make_id_table(&db, "T").await; + db.add_column("T".to_string(), "A".to_string(), Type::Text, None) + .await + .unwrap(); + let err = db + .rename_column("T".to_string(), "A".to_string(), "A".to_string(), None) + .await + .unwrap_err(); + assert!(matches!(err, DbError::Unsupported(_)), "got {err:?}"); + } + + #[tokio::test] + async fn change_column_type_works_for_compatible_data() { + let db = db(); + make_id_table(&db, "T").await; + db.add_column("T".to_string(), "Score".to_string(), Type::Text, None) + .await + .unwrap(); + // Insert numeric-looking strings. + for v in ["1", "2", "3"] { + db.insert( + "T".to_string(), + Some(vec!["Score".to_string()]), + vec![Value::Text(v.to_string())], + None, + ) + .await + .unwrap(); + } + let desc = db + .change_column_type("T".to_string(), "Score".to_string(), Type::Int, None) + .await + .unwrap(); + let score = desc.columns.iter().find(|c| c.name == "Score").unwrap(); + assert_eq!(score.user_type, Some(Type::Int)); + // Data preserved via SQLite's coercion (TEXT '1' → INTEGER 1). + let data = db.query_data("T".to_string(), None).await.unwrap(); + assert_eq!(data.rows.len(), 3); + } + + #[tokio::test] + async fn change_column_type_refuses_pk() { + let db = db(); + make_id_table(&db, "T").await; + let err = db + .change_column_type("T".to_string(), "id".to_string(), Type::Text, None) + .await + .unwrap_err(); + assert!(matches!(err, DbError::Unsupported(_)), "got {err:?}"); + } + + #[tokio::test] + async fn change_column_type_refuses_serial_target() { + let db = db(); + make_id_table(&db, "T").await; + db.add_column("T".to_string(), "A".to_string(), Type::Int, None) + .await + .unwrap(); + let err = db + .change_column_type("T".to_string(), "A".to_string(), Type::Serial, None) + .await + .unwrap_err(); + assert!(matches!(err, DbError::Unsupported(_)), "got {err:?}"); + } + + #[tokio::test] + async fn change_column_type_refuses_relationship_column() { + let db = db(); + make_id_table(&db, "Customers").await; + make_id_table(&db, "Orders").await; + db.add_column( + "Orders".to_string(), + "cust_id".to_string(), + Type::Int, + None, + ) + .await + .unwrap(); + db.add_relationship( + None, + "Customers".to_string(), + "id".to_string(), + "Orders".to_string(), + "cust_id".to_string(), + ReferentialAction::NoAction, + ReferentialAction::NoAction, + false, + None, + ) + .await + .unwrap(); + let err = db + .change_column_type( + "Orders".to_string(), + "cust_id".to_string(), + Type::Text, + None, + ) + .await + .unwrap_err(); + assert!(matches!(err, DbError::Unsupported(_)), "got {err:?}"); + } + + #[tokio::test] + async fn change_column_type_no_op_to_same_type_errors() { + let db = db(); + make_id_table(&db, "T").await; + db.add_column("T".to_string(), "A".to_string(), Type::Int, None) + .await + .unwrap(); + let err = db + .change_column_type("T".to_string(), "A".to_string(), Type::Int, None) + .await + .unwrap_err(); + assert!(matches!(err, DbError::Unsupported(_)), "got {err:?}"); + } + #[tokio::test] async fn describe_missing_table_returns_no_such_table() { let db = db(); diff --git a/src/dsl/command.rs b/src/dsl/command.rs index aed0a9d..fdb8a52 100644 --- a/src/dsl/command.rs +++ b/src/dsl/command.rs @@ -44,6 +44,35 @@ pub enum Command { column: String, ty: Type, }, + /// Remove a column from a table. Refused if the column is + /// part of the primary key or is involved in a declared + /// relationship — drop the relationship first. + DropColumn { + table: String, + column: String, + }, + /// Rename a column. SQLite handles cascading renames in + /// FK references on other tables; the executor mirrors + /// the change into our `__rdbms_playground_columns` and + /// `__rdbms_playground_relationships` metadata tables in + /// the same transaction. + RenameColumn { + table: String, + old: String, + new: String, + }, + /// Change a column's type. Implemented via the + /// rebuild-table primitive (ADR-0013) since SQLite's + /// ALTER TABLE does not support type changes. Refused + /// for PK columns and for columns involved in a declared + /// relationship — those would require cascading FK + /// type updates the v1 surface deliberately doesn't + /// expose. + ChangeColumnType { + table: String, + column: String, + ty: Type, + }, /// Establish a 1:n relationship: parent_table.parent_column /// is the primary-key side; child_table.child_column is the /// foreign-key side. `name` is optional — when `None`, the @@ -144,6 +173,9 @@ impl Command { Self::CreateTable { .. } => "create table", Self::DropTable { .. } => "drop table", Self::AddColumn { .. } => "add column", + Self::DropColumn { .. } => "drop column", + Self::RenameColumn { .. } => "rename column", + Self::ChangeColumnType { .. } => "change column", Self::AddRelationship { .. } => "add relationship", Self::DropRelationship { .. } => "drop relationship", Self::ShowTable { .. } => "show table", @@ -166,6 +198,9 @@ impl Command { | Self::ShowTable { name } | Self::ShowData { name } => name, Self::AddColumn { table, .. } + | Self::DropColumn { table, .. } + | Self::RenameColumn { table, .. } + | Self::ChangeColumnType { table, .. } | Self::Insert { table, .. } | Self::Update { table, .. } | Self::Delete { table, .. } => table, diff --git a/src/dsl/parser.rs b/src/dsl/parser.rs index bbba41d..9a8b904 100644 --- a/src/dsl/parser.rs +++ b/src/dsl/parser.rs @@ -136,12 +136,10 @@ fn command_parser<'a>() // and `add column T: c (text)` all parse identically. // Matches the convention elsewhere in the DSL where bare // identifiers are accepted in unambiguous positions. - let optional_to = keyword_ci("to").or_not(); - let optional_table = keyword_ci("table").or_not(); let add_column = keyword_ci("add") .ignore_then(keyword_ci("column")) - .ignore_then(optional_to) - .ignore_then(optional_table) + .ignore_then(optional_keyword("to")) + .ignore_then(optional_keyword("table")) .ignore_then(identifier()) .then_ignore(just(':').padded()) .then(identifier()) @@ -150,6 +148,45 @@ fn command_parser<'a>() .then_ignore(just(')').padded()) .map(|((table, column), ty)| Command::AddColumn { table, column, ty }); + // `drop column [from] [table] : `. Both + // prepositions independently optional, matching the + // `add column` shape for symmetry. + let drop_column = keyword_ci("drop") + .ignore_then(keyword_ci("column")) + .ignore_then(optional_keyword("from")) + .ignore_then(optional_keyword("table")) + .ignore_then(identifier()) + .then_ignore(just(':').padded()) + .then(identifier()) + .map(|(table, column)| Command::DropColumn { table, column }); + + // `rename column [in] [table] : to `. + let rename_column = keyword_ci("rename") + .ignore_then(keyword_ci("column")) + .ignore_then(optional_keyword("in")) + .ignore_then(optional_keyword("table")) + .ignore_then(identifier()) + .then_ignore(just(':').padded()) + .then(identifier()) + .then_ignore(keyword_ci("to")) + .then(identifier()) + .map(|((table, old), new)| Command::RenameColumn { table, old, new }); + + // `change column [in] [table] : ()`. + // Same shape as `add column` — the column-and-type + // tuple in parens — but the verb is `change`. + let change_column = keyword_ci("change") + .ignore_then(keyword_ci("column")) + .ignore_then(optional_keyword("in")) + .ignore_then(optional_keyword("table")) + .ignore_then(identifier()) + .then_ignore(just(':').padded()) + .then(identifier()) + .then_ignore(just('(').padded()) + .then(type_keyword()) + .then_ignore(just(')').padded()) + .map(|((table, column), ty)| Command::ChangeColumnType { table, column, ty }); + let add_relationship = add_relationship_parser(); let drop_relationship = drop_relationship_parser(); @@ -169,10 +206,16 @@ fn command_parser<'a>() choice(( create_table, + // `drop column` and `drop relationship` come before + // `drop table` because both are more specific — + // chumsky's `choice` tries each in order. + drop_column, + drop_relationship, drop_table, add_column, add_relationship, - drop_relationship, + rename_column, + change_column, // Order: `show data` before `show table` because both // start with `show` and the longer keyword is checked // first via this ordering. @@ -582,6 +625,13 @@ fn type_keyword<'a>() }) } +/// `keyword_ci(kw).or_not()` packaged for readability. +fn optional_keyword<'a>( + kw: &'static str, +) -> impl Parser<'a, &'a str, (), extra::Err>> + Clone { + keyword_ci(kw).or_not().map(|_| ()) +} + /// Case-insensitive keyword matcher. Consumes leading and /// trailing whitespace and, importantly, requires a word /// boundary so `create` does not match a prefix of `created`. @@ -732,6 +782,122 @@ mod tests { ); } + // --- drop column / rename column / change column --- + + #[test] + fn drop_column_simple() { + assert_eq!( + ok("drop column from table Customers: Email"), + Command::DropColumn { + table: "Customers".to_string(), + column: "Email".to_string(), + } + ); + } + + #[test] + fn drop_column_accepts_bare_identifiers() { + // Both prepositions independently optional, matching + // `add column`'s shape. + assert_eq!( + ok("drop column Customers: Email"), + Command::DropColumn { + table: "Customers".to_string(), + column: "Email".to_string(), + } + ); + assert_eq!( + ok("drop column from Customers: Email"), + Command::DropColumn { + table: "Customers".to_string(), + column: "Email".to_string(), + } + ); + assert_eq!( + ok("drop column table Customers: Email"), + Command::DropColumn { + table: "Customers".to_string(), + column: "Email".to_string(), + } + ); + } + + #[test] + fn rename_column_simple() { + assert_eq!( + ok("rename column in table Customers: OldName to NewName"), + Command::RenameColumn { + table: "Customers".to_string(), + old: "OldName".to_string(), + new: "NewName".to_string(), + } + ); + } + + #[test] + fn rename_column_accepts_bare_identifiers() { + assert_eq!( + ok("rename column Customers: A to B"), + Command::RenameColumn { + table: "Customers".to_string(), + old: "A".to_string(), + new: "B".to_string(), + } + ); + assert_eq!( + ok("rename column in Customers: A to B"), + Command::RenameColumn { + table: "Customers".to_string(), + old: "A".to_string(), + new: "B".to_string(), + } + ); + assert_eq!( + ok("rename column table Customers: A to B"), + Command::RenameColumn { + table: "Customers".to_string(), + old: "A".to_string(), + new: "B".to_string(), + } + ); + } + + #[test] + fn change_column_simple() { + assert_eq!( + ok("change column in table Customers: Score (int)"), + Command::ChangeColumnType { + table: "Customers".to_string(), + column: "Score".to_string(), + ty: Type::Int, + } + ); + } + + #[test] + fn change_column_accepts_bare_identifiers() { + assert_eq!( + ok("change column Customers: Score (real)"), + Command::ChangeColumnType { + table: "Customers".to_string(), + column: "Score".to_string(), + ty: Type::Real, + } + ); + } + + #[test] + fn change_column_keywords_are_case_insensitive() { + assert_eq!( + ok("CHANGE COLUMN IN TABLE Customers: Score (TEXT)"), + Command::ChangeColumnType { + table: "Customers".to_string(), + column: "Score".to_string(), + ty: Type::Text, + } + ); + } + #[test] fn drop_table_simple() { assert_eq!( diff --git a/src/runtime.rs b/src/runtime.rs index 7477b9f..65c0c8b 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -1011,6 +1011,18 @@ async fn execute_command_typed( .add_column(table, column, ty, src) .await .map(|d| CommandOutcome::Schema(Some(d))), + Command::DropColumn { table, column } => database + .drop_column(table, column, src) + .await + .map(|d| CommandOutcome::Schema(Some(d))), + Command::RenameColumn { table, old, new } => database + .rename_column(table, old, new, src) + .await + .map(|d| CommandOutcome::Schema(Some(d))), + Command::ChangeColumnType { table, column, ty } => database + .change_column_type(table, column, ty, src) + .await + .map(|d| CommandOutcome::Schema(Some(d))), Command::AddRelationship { name, parent_table,