diff --git a/core/translate/alter.rs b/core/translate/alter.rs index 0e2820ce9..5875b201f 100644 --- a/core/translate/alter.rs +++ b/core/translate/alter.rs @@ -1,10 +1,17 @@ use std::sync::Arc; -use turso_parser::{ast, parser::Parser}; +use turso_parser::{ + ast::{self, TableInternalId}, + parser::Parser, +}; use crate::{ function::{AlterTableFunc, Func}, - schema::Column, - translate::emitter::Resolver, + schema::{Column, Table}, + translate::{ + emitter::Resolver, + expr::{walk_expr, WalkControl}, + plan::{ColumnUsedMask, OuterQueryReference, TableReferences}, + }, util::normalize_ident, vdbe::{ builder::{CursorType, ProgramBuilder}, @@ -34,7 +41,9 @@ pub fn translate_alter_table( crate::bail_parse_error!("table {} may not be modified", table_name); } - if resolver.schema.table_has_indexes(table_name) && !resolver.schema.indexes_enabled() { + let table_indexes = resolver.schema.get_indices(table_name).collect::>(); + + if !table_indexes.is_empty() && !resolver.schema.indexes_enabled() { // Let's disable altering a table with indices altogether instead of checking column by // column to be extra safe. crate::bail_parse_error!( @@ -80,6 +89,16 @@ pub fn translate_alter_table( LimboError::ParseError(format!("no such column: \"{column_name}\"")) })?; + // Column cannot be dropped if: + // The column is a PRIMARY KEY or part of one. + // The column has a UNIQUE constraint. + // The column is indexed. + // The column is named in the WHERE clause of a partial index. + // The column is named in a table or column CHECK constraint not associated with the column being dropped. + // The column is used in a foreign key constraint. + // The column is used in the expression of a generated column. + // The column appears in a trigger or view. + if column.primary_key { return Err(LimboError::ParseError(format!( "cannot drop column \"{column_name}\": PRIMARY KEY" @@ -98,6 +117,65 @@ pub fn translate_alter_table( ))); } + for index in table_indexes.iter() { + // Referenced in regular index + if index + .columns + .iter() + .any(|col| col.pos_in_table == dropped_index) + { + return Err(LimboError::ParseError(format!( + "cannot drop column \"{column_name}\": indexed" + ))); + } + // Referenced in partial index + if index.where_clause.is_some() { + let mut table_references = TableReferences::new( + vec![], + vec![OuterQueryReference { + identifier: table_name.to_string(), + internal_id: TableInternalId::from(0), + table: Table::BTree(Arc::new(btree.clone())), + col_used_mask: ColumnUsedMask::default(), + }], + ); + let where_copy = index + .bind_where_expr(Some(&mut table_references), connection) + .expect("where clause to exist"); + let mut column_referenced = false; + walk_expr( + &where_copy, + &mut |e: &ast::Expr| -> crate::Result { + if let ast::Expr::Column { + table, + column: column_index, + .. + } = e + { + if *table == TableInternalId::from(0) + && *column_index == dropped_index + { + column_referenced = true; + return Ok(WalkControl::SkipChildren); + } + } + Ok(WalkControl::Continue) + }, + )?; + if column_referenced { + return Err(LimboError::ParseError(format!( + "cannot drop column \"{column_name}\": indexed" + ))); + } + } + } + + // TODO: check usage in CHECK constraint when implemented + // TODO: check usage in foreign key constraint when implemented + // TODO: check usage in generated column when implemented + + // References in VIEWs are checked in the VDBE layer op_drop_column instruction. + btree.columns.remove(dropped_index); let sql = btree.to_sql().replace('\'', "''"); diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index fc55eee0e..67435caec 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -7883,10 +7883,27 @@ pub fn op_drop_column( let conn = program.connection.clone(); + let normalized_table_name = normalize_ident(table.as_str()); + + let column_name = { + let schema = conn.schema.read(); + let table = schema + .tables + .get(&normalized_table_name) + .expect("table being ALTERed should be in schema"); + table + .get_column_at(*column_index) + .expect("column being ALTERed should be in schema") + .name + .as_ref() + .expect("column being ALTERed should be named") + .clone() + }; + conn.with_schema_mut(|schema| { let table = schema .tables - .get_mut(table) + .get_mut(&normalized_table_name) .expect("table being renamed should be in schema"); let table = Arc::make_mut(table); @@ -7899,6 +7916,31 @@ pub fn op_drop_column( btree.columns.remove(*column_index) }); + let schema = conn.schema.read(); + if let Some(indexes) = schema.indexes.get(&normalized_table_name) { + for index in indexes { + if index + .columns + .iter() + .any(|column| column.pos_in_table == *column_index) + { + return Err(LimboError::ParseError(format!( + "cannot drop column \"{column_name}\": indexed" + ))); + } + } + } + + for (view_name, view) in schema.views.iter() { + let view_select_sql = format!("SELECT * FROM {view_name}"); + conn.prepare(view_select_sql.as_str()).map_err(|e| { + LimboError::ParseError(format!( + "cannot drop column \"{}\": referenced in VIEW {view_name}: {}", + column_name, view.sql, + )) + })?; + } + state.pc += 1; Ok(InsnFunctionStepResult::Step) } @@ -7954,6 +7996,20 @@ pub fn op_alter_column( let conn = program.connection.clone(); let normalized_table_name = normalize_ident(table_name.as_str()); + let old_column_name = { + let schema = conn.schema.read(); + let table = schema + .tables + .get(&normalized_table_name) + .expect("table being ALTERed should be in schema"); + table + .get_column_at(*column_index) + .expect("column being ALTERed should be in schema") + .name + .as_ref() + .expect("column being ALTERed should be named") + .clone() + }; let new_column = crate::schema::Column::from(definition); conn.with_schema_mut(|schema| { @@ -7995,6 +8051,27 @@ pub fn op_alter_column( } }); + let schema = conn.schema.read(); + if *rename { + let table = schema + .tables + .get(&normalized_table_name) + .expect("table being ALTERed should be in schema"); + let column = table + .get_column_at(*column_index) + .expect("column being ALTERed should be in schema"); + for (view_name, view) in schema.views.iter() { + let view_select_sql = format!("SELECT * FROM {view_name}"); + // FIXME: this should rewrite the view to reference the new column name + conn.prepare(view_select_sql.as_str()).map_err(|e| { + LimboError::ParseError(format!( + "cannot rename column \"{}\": referenced in VIEW {view_name}: {}", + old_column_name, view.sql, + )) + })?; + } + } + state.pc += 1; Ok(InsnFunctionStepResult::Step) } diff --git a/tests/integration/query_processing/mod.rs b/tests/integration/query_processing/mod.rs index 742cdf52c..fd30cac1e 100644 --- a/tests/integration/query_processing/mod.rs +++ b/tests/integration/query_processing/mod.rs @@ -1,4 +1,5 @@ mod test_btree; +mod test_ddl; mod test_read_path; mod test_write_path; diff --git a/tests/integration/query_processing/test_ddl.rs b/tests/integration/query_processing/test_ddl.rs new file mode 100644 index 000000000..990101973 --- /dev/null +++ b/tests/integration/query_processing/test_ddl.rs @@ -0,0 +1,134 @@ +use crate::common::TempDatabase; + +#[test] +fn test_fail_drop_indexed_column() -> anyhow::Result<()> { + let _ = env_logger::try_init(); + let tmp_db = TempDatabase::new_with_rusqlite("CREATE TABLE t (a, b);", true); + let conn = tmp_db.connect_limbo(); + + conn.execute("CREATE INDEX i ON t (a)")?; + let res = conn.execute("ALTER TABLE t DROP COLUMN a"); + assert!(res.is_err(), "Expected error when dropping indexed column"); + Ok(()) +} + +#[test] +fn test_fail_drop_unique_column() -> anyhow::Result<()> { + let _ = env_logger::try_init(); + let tmp_db = TempDatabase::new_with_rusqlite("CREATE TABLE t (a UNIQUE, b);", true); + let conn = tmp_db.connect_limbo(); + + let res = conn.execute("ALTER TABLE t DROP COLUMN a"); + assert!(res.is_err(), "Expected error when dropping UNIQUE column"); + Ok(()) +} + +#[test] +fn test_fail_drop_compound_unique_column() -> anyhow::Result<()> { + let _ = env_logger::try_init(); + let tmp_db = TempDatabase::new_with_rusqlite("CREATE TABLE t (a, b, UNIQUE(a, b));", true); + let conn = tmp_db.connect_limbo(); + + let res = conn.execute("ALTER TABLE t DROP COLUMN a"); + assert!( + res.is_err(), + "Expected error when dropping column in compound UNIQUE" + ); + Ok(()) +} + +#[test] +fn test_fail_drop_primary_key_column() -> anyhow::Result<()> { + let _ = env_logger::try_init(); + let tmp_db = TempDatabase::new_with_rusqlite("CREATE TABLE t (a PRIMARY KEY, b);", true); + let conn = tmp_db.connect_limbo(); + + let res = conn.execute("ALTER TABLE t DROP COLUMN a"); + assert!( + res.is_err(), + "Expected error when dropping PRIMARY KEY column" + ); + Ok(()) +} + +#[test] +fn test_fail_drop_compound_primary_key_column() -> anyhow::Result<()> { + let _ = env_logger::try_init(); + let tmp_db = TempDatabase::new_with_rusqlite("CREATE TABLE t (a, b, PRIMARY KEY(a, b));", true); + let conn = tmp_db.connect_limbo(); + + let res = conn.execute("ALTER TABLE t DROP COLUMN a"); + assert!( + res.is_err(), + "Expected error when dropping column in compound PRIMARY KEY" + ); + Ok(()) +} + +#[test] +fn test_fail_drop_partial_index_column() -> anyhow::Result<()> { + let _ = env_logger::try_init(); + let tmp_db = TempDatabase::new_with_rusqlite("CREATE TABLE t (a, b);", true); + let conn = tmp_db.connect_limbo(); + + conn.execute("CREATE INDEX i ON t (b) WHERE a > 0")?; + let res = conn.execute("ALTER TABLE t DROP COLUMN a"); + assert!( + res.is_err(), + "Expected error when dropping column referenced by partial index" + ); + Ok(()) +} + +#[test] +fn test_fail_drop_view_column() -> anyhow::Result<()> { + let _ = env_logger::try_init(); + let tmp_db = TempDatabase::new_with_rusqlite("CREATE TABLE t (a, b);", true); + let conn = tmp_db.connect_limbo(); + + conn.execute("CREATE VIEW v AS SELECT a, b FROM t")?; + let res = conn.execute("ALTER TABLE t DROP COLUMN a"); + assert!( + res.is_err(), + "Expected error when dropping column referenced by view" + ); + Ok(()) +} + +// FIXME: this should rewrite the view to reference the new column name +#[test] +fn test_fail_rename_view_column() -> anyhow::Result<()> { + let _ = env_logger::try_init(); + let tmp_db = TempDatabase::new_with_rusqlite("CREATE TABLE t (a, b);", true); + let conn = tmp_db.connect_limbo(); + + conn.execute("CREATE VIEW v AS SELECT a, b FROM t")?; + let res = conn.execute("ALTER TABLE t RENAME a TO c"); + assert!( + res.is_err(), + "Expected error when renaming column referenced by view" + ); + Ok(()) +} + +#[test] +fn test_allow_drop_unreferenced_columns() -> anyhow::Result<()> { + let _ = env_logger::try_init(); + let tmp_db = TempDatabase::new_with_rusqlite( + "CREATE TABLE t (pk INTEGER PRIMARY KEY, indexed INTEGER, viewed INTEGER, partial INTEGER, compound1 INTEGER, compound2 INTEGER, unused1 INTEGER, unused2 INTEGER, unused3 INTEGER);", + true + ); + let conn = tmp_db.connect_limbo(); + + conn.execute("CREATE INDEX idx ON t(indexed)")?; + conn.execute("CREATE VIEW v AS SELECT viewed FROM t")?; + conn.execute("CREATE INDEX partial_idx ON t(compound1) WHERE partial > 0")?; + conn.execute("CREATE INDEX compound_idx ON t(compound1, compound2)")?; + + // Should be able to drop unused columns + conn.execute("ALTER TABLE t DROP COLUMN unused1")?; + conn.execute("ALTER TABLE t DROP COLUMN unused2")?; + conn.execute("ALTER TABLE t DROP COLUMN unused3")?; + + Ok(()) +}