Merge 'Fix various ALTER TABLE bugs' from Jussi Saurio

Closes #3392 , Closes #3395
1. Prevent `ALTER TABLE t DROP COLUMN a` when exists e.g. `CREATE INDEX
ta ON t(a)`
2. Prevent `ALTER TABLE t DROP COLUMN a` when exists e.g. `CREATE INDEX
tba ON t(b) WHERE t.a > 100`
3. Prevent `ALTER TABLE t DROP COLUMN a` when exists e.g. `CREATE VIEW
ta AS SELECT a FROM t`;
and
4. Prevent `ALTER TABLE t RENAME a TO b` when exists e.g. `CREATE VIEW
ta AS SELECT a FROM t`;
Number 4 is incompatible with SQLite because we should be rewriting the
view to include the renamed column. I left that as a FIXME

Closes #3394

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::<Vec<_>>();
+
+    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<WalkControl> {
+                            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('\'', "''");

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)
 }

tests/integration/query_processing/mod.rs

@@ -1,4 +1,5 @@
 mod test_btree;
+mod test_ddl;
 mod test_read_path;
 mod test_write_path;
 

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(())
+}