From de2ac89ad2b766cc423be33d55b5b250090d992a Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Mon, 19 May 2025 23:54:14 -0300 Subject: [PATCH 01/33] feat: complete ALTER TABLE implementation --- core/translate/delete.rs | 2 +- core/translate/emitter.rs | 17 +-- core/translate/mod.rs | 262 +++++++++++++++++++++++++++++++++++--- core/translate/plan.rs | 3 +- core/translate/schema.rs | 6 - core/translate/select.rs | 2 +- core/translate/update.rs | 36 ++++-- core/vdbe/explain.rs | 2 +- fuzz/Cargo.lock | 55 +++++--- testing/alter_table.test | 37 ++++++ 10 files changed, 349 insertions(+), 73 deletions(-) diff --git a/core/translate/delete.rs b/core/translate/delete.rs index 3786a0e00..301b1586d 100644 --- a/core/translate/delete.rs +++ b/core/translate/delete.rs @@ -36,7 +36,7 @@ pub fn translate_delete( approx_num_labels: 0, }; program.extend(&opts); - emit_program(&mut program, delete_plan, schema, syms)?; + emit_program(&mut program, delete_plan, schema, syms, |_| {})?; Ok(program) } diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs index 501c0d7b9..30224147b 100644 --- a/core/translate/emitter.rs +++ b/core/translate/emitter.rs @@ -19,7 +19,6 @@ use super::order_by::{emit_order_by, init_order_by, SortMetadata}; use super::plan::{ JoinOrderMember, Operation, QueryDestination, SelectPlan, TableReferences, UpdatePlan, }; -use super::schema::ParseSchema; use super::select::emit_simple_count; use super::subquery::emit_subqueries; use crate::error::SQLITE_CONSTRAINT_PRIMARYKEY; @@ -182,11 +181,12 @@ pub fn emit_program( plan: Plan, schema: &Schema, syms: &SymbolTable, + after: impl FnOnce(&mut ProgramBuilder), ) -> Result<()> { match plan { Plan::Select(plan) => emit_program_for_select(program, plan, schema, syms), Plan::Delete(plan) => emit_program_for_delete(program, plan, schema, syms), - Plan::Update(plan) => emit_program_for_update(program, plan, schema, syms), + Plan::Update(plan) => emit_program_for_update(program, plan, schema, syms, after), Plan::CompoundSelect { .. } => { emit_program_for_compound_select(program, plan, schema, syms) } @@ -834,6 +834,7 @@ fn emit_program_for_update( mut plan: UpdatePlan, schema: &Schema, syms: &SymbolTable, + after: impl FnOnce(&mut ProgramBuilder), ) -> Result<()> { let mut t_ctx = TranslateCtx::new( program, @@ -902,16 +903,6 @@ fn emit_program_for_update( )?; emit_update_insns(&plan, &t_ctx, program, index_cursors)?; - match plan.parse_schema { - ParseSchema::None => {} - ParseSchema::Reload => { - program.emit_insn(crate::vdbe::insn::Insn::ParseSchema { - db: usize::MAX, // TODO: This value is unused, change when we do something with it - where_clause: None, - }); - } - } - close_loop( program, &mut t_ctx, @@ -921,6 +912,8 @@ fn emit_program_for_update( program.preassign_label_to_next_insn(after_main_loop_label); + after(program); + // Finalize program program.epilogue(TransactionMode::Write); program.result_columns = plan.returning.unwrap_or_default(); diff --git a/core/translate/mod.rs b/core/translate/mod.rs index 716479c6e..1e0b05ea8 100644 --- a/core/translate/mod.rs +++ b/core/translate/mod.rs @@ -31,11 +31,12 @@ pub(crate) mod update; mod values; use crate::fast_lock::SpinLock; -use crate::schema::Schema; +use crate::schema::{Column, Schema}; use crate::storage::pager::Pager; use crate::storage::sqlite3_ondisk::DatabaseHeader; use crate::translate::delete::translate_delete; use crate::vdbe::builder::{ProgramBuilder, ProgramBuilderOpts, QueryMode}; +use crate::vdbe::insn::{Insn, RegisterOrLiteral}; use crate::vdbe::Program; use crate::{bail_parse_error, Connection, LimboError, Result, SymbolTable}; use fallible_iterator::FallibleIterator as _; @@ -44,15 +45,14 @@ use insert::translate_insert; use limbo_sqlite3_parser::ast::{self, Delete, Insert}; use limbo_sqlite3_parser::lexer::sql::Parser; use schema::{ - translate_create_table, translate_create_virtual_table, translate_drop_table, ParseSchema, - SQLITE_TABLEID, + translate_create_table, translate_create_virtual_table, translate_drop_table, SQLITE_TABLEID, }; use select::translate_select; use std::rc::{Rc, Weak}; use std::sync::Arc; use tracing::{instrument, Level}; use transaction::{translate_tx_begin, translate_tx_commit}; -use update::translate_update; +use update::{translate_update, translate_update_with_after}; #[instrument(skip_all, level = Level::TRACE)] pub fn translate( @@ -112,27 +112,245 @@ pub fn translate_inner( ) -> Result { let program = match stmt { ast::Stmt::AlterTable(a) => { - let (table_name, alter_table) = a.as_ref(); + let (table_name, alter_table) = *a; + let ast::Name(table_name) = table_name.name; + + let Some(table) = schema.tables.get(&table_name) else { + return Err(LimboError::ParseError(format!( + "no such table: {table_name}" + ))); + }; + + let Some(btree) = table.btree() else { todo!() }; + let mut btree = (*btree).clone(); match alter_table { - ast::AlterTableBody::RenameTo(name) => { - let rename = &name.0; - let name = &table_name.name.0; + ast::AlterTableBody::DropColumn(column) => { + let ast::Name(column) = column; - let Some(table) = schema.tables.get(name) else { - return Err(LimboError::ParseError(format!("no such table: {name}"))); + // Tables always have at least one column. + assert_ne!(btree.columns.len(), 0); + + if btree.columns.len() == 1 { + return Err(LimboError::ParseError(format!( + "cannot drop column \"{column}\": no other columns exist" + ))); + } + + let (dropped_col, col) = btree + .columns + .iter() + .enumerate() + .find(|(_, Column { name, .. })| name.as_ref() == Some(&column)) + .ok_or_else(|| { + LimboError::ParseError(format!("no such column: \"{column}\"")) + })?; + + if col.primary_key { + return Err(LimboError::ParseError(format!( + "cannot drop column \"{column}\": PRIMARY KEY" + ))); + } + + if col.unique { + return Err(LimboError::ParseError(format!( + "cannot drop column \"{column}\": UNIQUE" + ))); + } + + btree.columns.remove(dropped_col); + + let sql = btree.to_sql(); + + let stmt = format!( + r#" + UPDATE {SQLITE_TABLEID} + SET sql = '{sql}' + WHERE tbl_name = '{table_name}' + "#, + ); + + let mut parser = Parser::new(stmt.as_bytes()); + let Some(ast::Cmd::Stmt(ast::Stmt::Update(mut update))) = parser.next()? else { + unreachable!(); }; - if schema.tables.contains_key(rename) { + translate_update_with_after( + QueryMode::Normal, + schema, + &mut update, + syms, + program, + |program| { + let loop_start = program.allocate_label(); + let loop_end = program.allocate_label(); + + let column_count = btree.columns.len(); + let root_page = btree.root_page; + let table_name = btree.name.clone(); + + let cursor_id = program.alloc_cursor_id( + crate::vdbe::builder::CursorType::BTreeTable(Rc::new(btree)), + ); + + program.emit_insn(Insn::OpenWrite { + cursor_id, + root_page: RegisterOrLiteral::Literal(root_page), + name: table_name.clone(), + }); + + program.emit_insn(Insn::Rewind { + cursor_id, + pc_if_empty: loop_end, + }); + + let rowid = program.alloc_register(); + + program.emit_insn(Insn::RowId { + cursor_id, + dest: rowid, + }); + + program.preassign_label_to_next_insn(loop_start); + + let first_column = program.alloc_registers(column_count); + + let mut iter = first_column; + + for i in 0..(column_count + 1) { + if i == dropped_col { + continue; + } + + program.emit_insn(Insn::Column { + cursor_id, + column: i, + dest: iter, + }); + + iter += 1; + } + + let record = program.alloc_register(); + + program.emit_insn(Insn::MakeRecord { + start_reg: first_column, + count: column_count, + dest_reg: record, + index_name: None, + }); + + program.emit_insn(Insn::Insert { + cursor: cursor_id, + key_reg: rowid, + record_reg: record, + flag: 0, + table_name: table_name.clone(), + }); + + program.emit_insn(Insn::Next { + cursor_id, + pc_if_next: loop_start, + }); + + program.preassign_label_to_next_insn(loop_end); + + program.emit_insn(Insn::ParseSchema { + db: usize::MAX, // TODO: This value is unused, change when we do something with it + where_clause: None, + }) + }, + )? + } + ast::AlterTableBody::AddColumn(col_def) => { + btree.columns.push(Column { + name: Some(col_def.col_name.0), + ty: crate::schema::Type::Null, + ty_str: "".to_string(), + primary_key: false, + is_rowid_alias: false, + notnull: false, + default: None, + unique: false, + }); + + let sql = btree.to_sql(); + + let stmt = format!( + r#" + UPDATE {SQLITE_TABLEID} + SET sql = '{sql}' + WHERE tbl_name = '{table_name}' + "#, + ); + + let mut parser = Parser::new(stmt.as_bytes()); + let Some(ast::Cmd::Stmt(ast::Stmt::Update(mut update))) = parser.next()? else { + unreachable!(); + }; + + translate_update_with_after( + QueryMode::Normal, + schema, + &mut update, + syms, + |program| { + program.emit_insn(Insn::ParseSchema { + db: usize::MAX, // TODO: This value is unused, change when we do something with it + where_clause: None, + }); + }, + )? + } + ast::AlterTableBody::RenameColumn { old, new } => { + let Some(column) = btree + .columns + .iter_mut() + .find(|column| column.name == Some(old.0.clone())) + else { + return Err(LimboError::ParseError(format!("no such column: \"{old}\""))); + }; + + column.name = Some(new.0.clone()); + + let sql = btree.to_sql(); + + let stmt = format!( + r#" + UPDATE {SQLITE_TABLEID} + SET sql = '{sql}' + WHERE tbl_name = '{table_name}' + "#, + ); + + let mut parser = Parser::new(stmt.as_bytes()); + let Some(ast::Cmd::Stmt(ast::Stmt::Update(mut update))) = parser.next()? else { + unreachable!(); + }; + + translate_update_with_after( + QueryMode::Normal, + schema, + &mut update, + syms, + |program| { + program.emit_insn(Insn::ParseSchema { + db: usize::MAX, // TODO: This value is unused, change when we do something with it + where_clause: None, + }); + }, + )? + } + ast::AlterTableBody::RenameTo(name) => { + let ast::Name(rename) = name; + + if schema.tables.contains_key(&rename) { return Err(LimboError::ParseError(format!( "there is already another table or index with this name: {rename}" ))); }; - let Some(btree) = table.btree() else { todo!() }; - - let mut btree = (*btree).clone(); - btree.name = rename.clone(); + btree.name = rename; let sql = btree.to_sql(); @@ -142,8 +360,9 @@ pub fn translate_inner( SET name = '{rename}' , tbl_name = '{rename}' , sql = '{sql}' - WHERE tbl_name = '{name}' + WHERE tbl_name = '{table_name}' "#, + rename = &btree.name, ); let mut parser = Parser::new(stmt.as_bytes()); @@ -151,16 +370,20 @@ pub fn translate_inner( unreachable!(); }; - translate_update( + translate_update_with_after( QueryMode::Normal, schema, &mut update, syms, - ParseSchema::Reload, program, + |program| { + program.emit_insn(Insn::ParseSchema { + db: usize::MAX, // TODO: This value is unused, change when we do something with it + where_clause: None, + }); + }, )? } - _ => todo!(), } } ast::Stmt::Analyze(_) => bail_parse_error!("ANALYZE not supported yet"), @@ -253,7 +476,6 @@ pub fn translate_inner( schema, &mut update, syms, - ParseSchema::None, program, )?, ast::Stmt::Vacuum(_, _) => bail_parse_error!("VACUUM not supported yet"), diff --git a/core/translate/plan.rs b/core/translate/plan.rs index de0a5b00f..fca387cf5 100644 --- a/core/translate/plan.rs +++ b/core/translate/plan.rs @@ -17,7 +17,7 @@ use crate::{schema::Type, types::SeekOp, util::can_pushdown_predicate}; use limbo_sqlite3_parser::ast::TableInternalId; -use super::{emitter::OperationMode, planner::determine_where_to_eval_term, schema::ParseSchema}; +use super::{emitter::OperationMode, planner::determine_where_to_eval_term}; #[derive(Debug, Clone)] pub struct ResultSetColumn { @@ -564,7 +564,6 @@ pub struct UpdatePlan { // whether the WHERE clause is always false pub contains_constant_false_condition: bool, pub indexes_to_update: Vec>, - pub parse_schema: ParseSchema, } #[derive(Clone, Copy, Debug, PartialEq, Eq)] diff --git a/core/translate/schema.rs b/core/translate/schema.rs index 1b1f6c040..5d9435b06 100644 --- a/core/translate/schema.rs +++ b/core/translate/schema.rs @@ -24,12 +24,6 @@ use crate::{bail_parse_error, Result}; use limbo_ext::VTabKind; use limbo_sqlite3_parser::ast::{fmt::ToTokens, CreateVirtualTable}; -#[derive(Debug, Clone, Copy)] -pub enum ParseSchema { - None, - Reload, -} - pub fn translate_create_table( query_mode: QueryMode, tbl_name: ast::QualifiedName, diff --git a/core/translate/select.rs b/core/translate/select.rs index 392bfcf53..4aff03a63 100644 --- a/core/translate/select.rs +++ b/core/translate/select.rs @@ -79,7 +79,7 @@ pub fn translate_select( }; program.extend(&opts); - emit_program(&mut program, select_plan, schema, syms)?; + emit_program(&mut program, select_plan, schema, syms, |_| {})?; Ok(TranslateSelectResult { program, num_result_cols, diff --git a/core/translate/update.rs b/core/translate/update.rs index 9d3d72f9e..d4af63b0e 100644 --- a/core/translate/update.rs +++ b/core/translate/update.rs @@ -17,8 +17,6 @@ use super::plan::{ }; use super::planner::bind_column_references; use super::planner::{parse_limit, parse_where}; -use super::schema::ParseSchema; - /* * Update is simple. By default we scan the table, and for each row, we check the WHERE * clause. If it evaluates to true, we build the new record with the updated value and insert. @@ -53,15 +51,9 @@ pub fn translate_update( schema: &Schema, body: &mut Update, syms: &SymbolTable, - parse_schema: ParseSchema, mut program: ProgramBuilder, ) -> crate::Result { - let mut plan = prepare_update_plan( - schema, - body, - parse_schema, - &mut program.table_reference_counter, - )?; + let mut plan = prepare_update_plan(schema, body, &mut program.table_reference_counter)?; optimize_plan(&mut plan, schema)?; // TODO: freestyling these numbers let opts = ProgramBuilderOpts { @@ -71,14 +63,35 @@ pub fn translate_update( approx_num_labels: 4, }; program.extend(&opts); - emit_program(&mut program, plan, schema, syms)?; + emit_program(&mut program, plan, schema, syms, |_| {})?; + Ok(program) +} + +pub fn translate_update_with_after( + query_mode: QueryMode, + schema: &Schema, + body: &mut Update, + syms: &SymbolTable, + mut program: ProgramBuilder, + after: impl FnOnce(&mut ProgramBuilder), +) -> crate::Result { + let mut plan = prepare_update_plan(schema, body, &mut program.table_reference_counter)?; + optimize_plan(&mut plan, schema)?; + // TODO: freestyling these numbers + let opts = ProgramBuilderOpts { + query_mode, + num_cursors: 1, + approx_num_insns: 20, + approx_num_labels: 4, + }; + program.extend(&opts); + emit_program(&mut program, plan, schema, syms, after)?; Ok(program) } pub fn prepare_update_plan( schema: &Schema, body: &mut Update, - parse_schema: ParseSchema, table_ref_counter: &mut TableRefIdCounter, ) -> crate::Result { if body.with.is_some() { @@ -215,6 +228,5 @@ pub fn prepare_update_plan( offset, contains_constant_false_condition: false, indexes_to_update, - parse_schema, })) } diff --git a/core/vdbe/explain.rs b/core/vdbe/explain.rs index fa6a2a70e..2e687598c 100644 --- a/core/vdbe/explain.rs +++ b/core/vdbe/explain.rs @@ -531,7 +531,7 @@ pub fn insn_to_str( let cursor_type = &program.cursor_ref[*cursor_id].1; let column_name: Option<&String> = match cursor_type { CursorType::BTreeTable(table) => { - let name = table.columns.get(*column).unwrap().name.as_ref(); + let name = table.columns.get(*column).and_then(|v| v.name.as_ref()); name } CursorType::BTreeIndex(index) => { diff --git a/fuzz/Cargo.lock b/fuzz/Cargo.lock index 091feceb7..e56c4f280 100644 --- a/fuzz/Cargo.lock +++ b/fuzz/Cargo.lock @@ -523,9 +523,9 @@ checksum = "8355be11b20d696c8f18f6cc018c4e372165b1fa8126cef092399c9951984ffa" [[package]] name = "libmimalloc-sys" -version = "0.1.39" +version = "0.1.42" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "23aa6811d3bd4deb8a84dde645f943476d13b248d818edcf8ce0b2f37f036b44" +checksum = "ec9d6fac27761dabcd4ee73571cdb06b7022dc99089acbe5435691edffaac0f4" dependencies = [ "cc", "libc", @@ -566,8 +566,9 @@ dependencies = [ [[package]] name = "limbo_core" -version = "0.0.19" +version = "0.0.20" dependencies = [ + "bitflags", "built", "cfg_block", "chrono", @@ -590,7 +591,7 @@ dependencies = [ "rand", "regex", "regex-syntax", - "rustix", + "rustix 1.0.7", "ryu", "strum", "thiserror 1.0.69", @@ -599,7 +600,7 @@ dependencies = [ [[package]] name = "limbo_ext" -version = "0.0.19" +version = "0.0.20" dependencies = [ "chrono", "getrandom 0.3.1", @@ -608,7 +609,7 @@ dependencies = [ [[package]] name = "limbo_macros" -version = "0.0.19" +version = "0.0.20" dependencies = [ "proc-macro2", "quote", @@ -617,7 +618,7 @@ dependencies = [ [[package]] name = "limbo_sqlite3_parser" -version = "0.0.19" +version = "0.0.20" dependencies = [ "bitflags", "cc", @@ -636,7 +637,7 @@ dependencies = [ [[package]] name = "limbo_time" -version = "0.0.19" +version = "0.0.20" dependencies = [ "chrono", "limbo_ext", @@ -648,7 +649,7 @@ dependencies = [ [[package]] name = "limbo_uuid" -version = "0.0.19" +version = "0.0.20" dependencies = [ "limbo_ext", "mimalloc", @@ -661,6 +662,12 @@ version = "0.4.15" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d26c52dbd32dccf2d10cac7725f8eae5296885fb5703b261f7d0a0739ec807ab" +[[package]] +name = "linux-raw-sys" +version = "0.9.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cd945864f07fe9f5371a27ad7b52a172b4b499999f1d97574c9fa68373937e12" + [[package]] name = "litemap" version = "0.7.5" @@ -691,21 +698,20 @@ checksum = "78ca9ab1a0babb1e7d5695e3530886289c18cf2f87ec19a575a0abdce112e3a3" [[package]] name = "miette" -version = "7.5.0" +version = "7.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1a955165f87b37fd1862df2a59547ac542c77ef6d17c666f619d1ad22dd89484" +checksum = "5f98efec8807c63c752b5bd61f862c165c115b0a35685bdcfd9238c7aeb592b7" dependencies = [ "cfg-if", "miette-derive", - "thiserror 1.0.69", "unicode-width", ] [[package]] name = "miette-derive" -version = "7.5.0" +version = "7.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bf45bf44ab49be92fd1227a3be6fc6f617f1a337c06af54981048574d8783147" +checksum = "db5b29714e950dbb20d5e6f74f9dcec4edbcc1067bb7f8ed198c097b8c1a818b" dependencies = [ "proc-macro2", "quote", @@ -714,9 +720,9 @@ dependencies = [ [[package]] name = "mimalloc" -version = "0.1.43" +version = "0.1.46" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "68914350ae34959d83f732418d51e2427a794055d0b9529f48259ac07af65633" +checksum = "995942f432bbb4822a7e9c3faa87a695185b0d09273ba85f097b54f4e458f2af" dependencies = [ "libmimalloc-sys", ] @@ -826,7 +832,7 @@ dependencies = [ "concurrent-queue", "hermit-abi", "pin-project-lite", - "rustix", + "rustix 0.38.44", "tracing", "windows-sys", ] @@ -949,7 +955,20 @@ dependencies = [ "bitflags", "errno", "libc", - "linux-raw-sys", + "linux-raw-sys 0.4.15", + "windows-sys", +] + +[[package]] +name = "rustix" +version = "1.0.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c71e83d6afe7ff64890ec6b71d6a69bb8a610ab78ce364b3352876bb4c801266" +dependencies = [ + "bitflags", + "errno", + "libc", + "linux-raw-sys 0.9.4", "windows-sys", ] diff --git a/testing/alter_table.test b/testing/alter_table.test index 65a397c9b..fe9b6f7cb 100755 --- a/testing/alter_table.test +++ b/testing/alter_table.test @@ -9,3 +9,40 @@ do_execsql_test_on_specific_db {:memory:} alter-table-rename-table { ALTER TABLE t1 RENAME TO t2; SELECT tbl_name FROM sqlite_schema; } { "t2" } + +# ALTER TABLE _ RENAME _ TO _ +do_execsql_test_on_specific_db {:memory:} alter-table-rename-column { + CREATE TABLE t(a); + ALTER TABLE t RENAME a TO b; + SELECT sql FROM sqlite_schema; +} { "CREATE TABLE t(b)" } + +# ALTER TABLE _ ADD _ +do_execsql_test_on_specific_db {:memory:} alter-table-add-column { + CREATE TABLE t(a); + INSERT INTO t VALUES (1); + SELECT * FROM t; + + ALTER TABLE t ADD b; + SELECT sql FROM sqlite_schema; + SELECT * FROM t; +} { + "1" + "CREATE TABLE t(a, b)" + "1|" +} + +# ALTER TABLE _ DROP _ +do_execsql_test_on_specific_db {:memory:} alter-table-add-column { + CREATE TABLE t(a, b); + INSERT INTO t VALUES (1, 2); + SELECT * FROM t; + + ALTER TABLE t DROP b; + SELECT sql FROM sqlite_schema; + SELECT * FROM t; +} { + "1|2" + "CREATE TABLE t(a)" + "1" +} From 5f25ed0738b2d12d835412481a36f9f88e041d4e Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Tue, 20 May 2025 02:38:34 -0300 Subject: [PATCH 02/33] fix UNIQUE constraints --- core/translate/mod.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/core/translate/mod.rs b/core/translate/mod.rs index 1e0b05ea8..ba72e0c53 100644 --- a/core/translate/mod.rs +++ b/core/translate/mod.rs @@ -158,6 +158,15 @@ pub fn translate_inner( ))); } + if btree.unique_sets.as_ref().is_some_and(|set| { + set.iter() + .any(|set| set.iter().any(|(column_name, _)| column_name == &column)) + }) { + return Err(LimboError::ParseError(format!( + "cannot drop column \"{column}\": UNIQUE" + ))); + } + btree.columns.remove(dropped_col); let sql = btree.to_sql(); From 65b6984c2af62757addf99a803a0586eae99c7b1 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Tue, 20 May 2025 02:39:42 -0300 Subject: [PATCH 03/33] fix: make sure to not modify a index --- core/translate/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/translate/mod.rs b/core/translate/mod.rs index ba72e0c53..be5a4178a 100644 --- a/core/translate/mod.rs +++ b/core/translate/mod.rs @@ -175,7 +175,7 @@ pub fn translate_inner( r#" UPDATE {SQLITE_TABLEID} SET sql = '{sql}' - WHERE tbl_name = '{table_name}' + WHERE name = '{table_name}' AND type = 'table' "#, ); @@ -289,7 +289,7 @@ pub fn translate_inner( r#" UPDATE {SQLITE_TABLEID} SET sql = '{sql}' - WHERE tbl_name = '{table_name}' + WHERE name = '{table_name}' AND type = 'table' "#, ); @@ -328,7 +328,7 @@ pub fn translate_inner( r#" UPDATE {SQLITE_TABLEID} SET sql = '{sql}' - WHERE tbl_name = '{table_name}' + WHERE name = '{table_name}' AND type = 'table' "#, ); @@ -369,7 +369,7 @@ pub fn translate_inner( SET name = '{rename}' , tbl_name = '{rename}' , sql = '{sql}' - WHERE tbl_name = '{table_name}' + WHERE name = '{table_name}' AND type = 'table' "#, rename = &btree.name, ); From 91f981a8b13d3ae2e4c7b3615637ef8885043d29 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Tue, 20 May 2025 17:29:57 -0300 Subject: [PATCH 04/33] fix(compat): sqlite firt checks if `old_table` exists something like `ALTER TABLE a RENAME TO a` should fail with `no such table: a` if `a` doesn't exists. --- vendored/sqlite3-parser/src/parser/ast/check.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/vendored/sqlite3-parser/src/parser/ast/check.rs b/vendored/sqlite3-parser/src/parser/ast/check.rs index 3df2c1c97..2ba263347 100644 --- a/vendored/sqlite3-parser/src/parser/ast/check.rs +++ b/vendored/sqlite3-parser/src/parser/ast/check.rs @@ -106,14 +106,6 @@ impl Stmt { Self::AlterTable(alter_table) => { let (old_name, body) = &**alter_table; match body { - AlterTableBody::RenameTo(new_name) => { - if *new_name == old_name.name { - return Err(custom_err!( - "there is already another table or index with this name: {}", - new_name - )); - } - } AlterTableBody::AddColumn(cd) => { for c in cd { if let ColumnConstraint::PrimaryKey { .. } = c { From d8bc8c48c3484abe71bd745c2892aacc32a60d8f Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Tue, 20 May 2025 17:32:55 -0300 Subject: [PATCH 05/33] add constraint dropping tests --- testing/alter_table.test | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/testing/alter_table.test b/testing/alter_table.test index fe9b6f7cb..f58664f32 100755 --- a/testing/alter_table.test +++ b/testing/alter_table.test @@ -3,21 +3,18 @@ set testdir [file dirname $argv0] source $testdir/tester.tcl -# ALTER TABLE _ RENAME TO _ do_execsql_test_on_specific_db {:memory:} alter-table-rename-table { CREATE TABLE t1(x INTEGER PRIMARY KEY); ALTER TABLE t1 RENAME TO t2; SELECT tbl_name FROM sqlite_schema; } { "t2" } -# ALTER TABLE _ RENAME _ TO _ do_execsql_test_on_specific_db {:memory:} alter-table-rename-column { CREATE TABLE t(a); ALTER TABLE t RENAME a TO b; SELECT sql FROM sqlite_schema; } { "CREATE TABLE t(b)" } -# ALTER TABLE _ ADD _ do_execsql_test_on_specific_db {:memory:} alter-table-add-column { CREATE TABLE t(a); INSERT INTO t VALUES (1); @@ -32,8 +29,7 @@ do_execsql_test_on_specific_db {:memory:} alter-table-add-column { "1|" } -# ALTER TABLE _ DROP _ -do_execsql_test_on_specific_db {:memory:} alter-table-add-column { +do_execsql_test_on_specific_db {:memory:} alter-table-drop-column { CREATE TABLE t(a, b); INSERT INTO t VALUES (1, 2); SELECT * FROM t; @@ -46,3 +42,23 @@ do_execsql_test_on_specific_db {:memory:} alter-table-add-column { "CREATE TABLE t(a)" "1" } + +do_execsql_test_in_memory_any_error fail-alter-table-drop-unique-column { + CREATE TABLE t(a, b UNIQUE); + ALTER TABLE t DROP b; +} + +do_execsql_test_in_memory_any_error fail-alter-table-drop-unique-column-constraint { + CREATE TABLE t(a, b, UNIQUE (b)); + ALTER TABLE t DROP b; +} + +do_execsql_test_in_memory_any_error fail-alter-table-drop-primary-key-column { + CREATE TABLE t(a PRIMARY KEY, b); + ALTER TABLE t DROP a; +} + +do_execsql_test_in_memory_any_error fail-alter-table-drop-primary-key-column-constrait { + CREATE TABLE t(a, b, PRIMARY KEY (a)); + ALTER TABLE t DROP a; +} From 587cf345cc9eceb9a03de20379788f7d2d43c5e1 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Tue, 20 May 2025 17:46:36 -0300 Subject: [PATCH 06/33] refactor: merge branches --- core/translate/mod.rs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/core/translate/mod.rs b/core/translate/mod.rs index be5a4178a..c32365a55 100644 --- a/core/translate/mod.rs +++ b/core/translate/mod.rs @@ -152,16 +152,13 @@ pub fn translate_inner( ))); } - if col.unique { - return Err(LimboError::ParseError(format!( - "cannot drop column \"{column}\": UNIQUE" - ))); - } - - if btree.unique_sets.as_ref().is_some_and(|set| { - set.iter() - .any(|set| set.iter().any(|(column_name, _)| column_name == &column)) - }) { + if col.unique + || btree.unique_sets.as_ref().is_some_and(|set| { + set.iter().any(|set| { + set.iter().any(|(column_name, _)| column_name == &column) + }) + }) + { return Err(LimboError::ParseError(format!( "cannot drop column \"{column}\": UNIQUE" ))); From f92e000277fea57bba52f6afcb4e377458b49db3 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Wed, 21 May 2025 00:45:58 -0300 Subject: [PATCH 07/33] fix: remove unused variable --- vendored/sqlite3-parser/src/parser/ast/check.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vendored/sqlite3-parser/src/parser/ast/check.rs b/vendored/sqlite3-parser/src/parser/ast/check.rs index 2ba263347..cbe2cacc7 100644 --- a/vendored/sqlite3-parser/src/parser/ast/check.rs +++ b/vendored/sqlite3-parser/src/parser/ast/check.rs @@ -104,7 +104,7 @@ impl Stmt { pub fn check(&self) -> Result<(), ParserError> { match self { Self::AlterTable(alter_table) => { - let (old_name, body) = &**alter_table; + let (_, body) = &**alter_table; match body { AlterTableBody::AddColumn(cd) => { for c in cd { From 3bc24eb86f41918f286418a33f3029a3f356e18b Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Wed, 21 May 2025 00:46:56 -0300 Subject: [PATCH 08/33] feat: proper column definition parsing --- core/schema.rs | 76 ++++++++++++++++++++++++++++++++++++++++++- core/translate/mod.rs | 11 +------ 2 files changed, 76 insertions(+), 11 deletions(-) diff --git a/core/schema.rs b/core/schema.rs index eceabbcfc..dea630da4 100644 --- a/core/schema.rs +++ b/core/schema.rs @@ -4,7 +4,7 @@ use crate::{util::normalize_ident, Result}; use crate::{LimboError, VirtualTable}; use core::fmt; use fallible_iterator::FallibleIterator; -use limbo_sqlite3_parser::ast::{Expr, Literal, SortOrder, TableOptions}; +use limbo_sqlite3_parser::ast::{self, ColumnDefinition, Expr, Literal, SortOrder, TableOptions}; use limbo_sqlite3_parser::{ ast::{Cmd, CreateTableBody, QualifiedName, ResultColumn, Stmt}, lexer::sql::Parser, @@ -586,6 +586,80 @@ impl Column { } } +// TODO: This might replace some of util::columns_from_create_table_body +impl From for Column { + fn from(value: ColumnDefinition) -> Self { + let ast::Name(name) = value.col_name; + + let mut default = None; + let mut notnull = false; + let mut primary_key = false; + let mut unique = false; + let mut collation = None; + + for ast::NamedColumnConstraint { constraint, .. } in value.constraints { + match constraint { + ast::ColumnConstraint::PrimaryKey { .. } => primary_key = true, + ast::ColumnConstraint::NotNull { .. } => notnull = true, + ast::ColumnConstraint::Unique(..) => unique = true, + ast::ColumnConstraint::Default(expr) => { + default.replace(expr); + } + ast::ColumnConstraint::Collate { collation_name } => { + collation.replace( + CollationSeq::new(&collation_name.0) + .expect("collation should have been set correctly in create table"), + ); + } + _ => {} + }; + } + + let ty = match value.col_type { + Some(ref data_type) => { + // https://www.sqlite.org/datatype3.html + let type_name = data_type.name.clone().to_uppercase(); + + if type_name.contains("INT") { + Type::Integer + } else if type_name.contains("CHAR") + || type_name.contains("CLOB") + || type_name.contains("TEXT") + { + Type::Text + } else if type_name.contains("BLOB") || type_name.is_empty() { + Type::Blob + } else if type_name.contains("REAL") + || type_name.contains("FLOA") + || type_name.contains("DOUB") + { + Type::Real + } else { + Type::Numeric + } + } + None => Type::Null, + }; + + let ty_str = value + .col_type + .map(|t| t.name.to_string()) + .unwrap_or_default(); + + Column { + name: Some(name), + ty, + default, + notnull, + ty_str, + primary_key, + is_rowid_alias: false, + unique, + collation, + } + } +} + /// 3.1. Determination Of Column Affinity /// For tables not declared as STRICT, the affinity of a column is determined by the declared type of the column, according to the following rules in the order shown: /// diff --git a/core/translate/mod.rs b/core/translate/mod.rs index c32365a55..4e6ed992f 100644 --- a/core/translate/mod.rs +++ b/core/translate/mod.rs @@ -269,16 +269,7 @@ pub fn translate_inner( )? } ast::AlterTableBody::AddColumn(col_def) => { - btree.columns.push(Column { - name: Some(col_def.col_name.0), - ty: crate::schema::Type::Null, - ty_str: "".to_string(), - primary_key: false, - is_rowid_alias: false, - notnull: false, - default: None, - unique: false, - }); + btree.columns.push(Column::from(col_def)); let sql = btree.to_sql(); From fa621115b531636b3be47474fe3391aee9229406 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Wed, 21 May 2025 00:56:37 -0300 Subject: [PATCH 09/33] fix: broken test --- vendored/sqlite3-parser/src/lexer/sql/test.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/vendored/sqlite3-parser/src/lexer/sql/test.rs b/vendored/sqlite3-parser/src/lexer/sql/test.rs index c0923403b..7fb315c52 100644 --- a/vendored/sqlite3-parser/src/lexer/sql/test.rs +++ b/vendored/sqlite3-parser/src/lexer/sql/test.rs @@ -280,14 +280,6 @@ fn alter_add_column_unique() { ); } -#[test] -fn alter_rename_same() { - expect_parser_err_msg( - b"ALTER TABLE t RENAME TO t", - "there is already another table or index with this name: t", - ); -} - #[test] fn natural_join_on() { expect_parser_err_msg( From db7bee41c31f53550b0bd0056a9645fe599608a9 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Wed, 21 May 2025 09:44:01 -0300 Subject: [PATCH 10/33] restore fuzz/Cargo.lock --- fuzz/Cargo.lock | 55 ++++++++++++++++--------------------------------- 1 file changed, 18 insertions(+), 37 deletions(-) diff --git a/fuzz/Cargo.lock b/fuzz/Cargo.lock index e56c4f280..091feceb7 100644 --- a/fuzz/Cargo.lock +++ b/fuzz/Cargo.lock @@ -523,9 +523,9 @@ checksum = "8355be11b20d696c8f18f6cc018c4e372165b1fa8126cef092399c9951984ffa" [[package]] name = "libmimalloc-sys" -version = "0.1.42" +version = "0.1.39" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ec9d6fac27761dabcd4ee73571cdb06b7022dc99089acbe5435691edffaac0f4" +checksum = "23aa6811d3bd4deb8a84dde645f943476d13b248d818edcf8ce0b2f37f036b44" dependencies = [ "cc", "libc", @@ -566,9 +566,8 @@ dependencies = [ [[package]] name = "limbo_core" -version = "0.0.20" +version = "0.0.19" dependencies = [ - "bitflags", "built", "cfg_block", "chrono", @@ -591,7 +590,7 @@ dependencies = [ "rand", "regex", "regex-syntax", - "rustix 1.0.7", + "rustix", "ryu", "strum", "thiserror 1.0.69", @@ -600,7 +599,7 @@ dependencies = [ [[package]] name = "limbo_ext" -version = "0.0.20" +version = "0.0.19" dependencies = [ "chrono", "getrandom 0.3.1", @@ -609,7 +608,7 @@ dependencies = [ [[package]] name = "limbo_macros" -version = "0.0.20" +version = "0.0.19" dependencies = [ "proc-macro2", "quote", @@ -618,7 +617,7 @@ dependencies = [ [[package]] name = "limbo_sqlite3_parser" -version = "0.0.20" +version = "0.0.19" dependencies = [ "bitflags", "cc", @@ -637,7 +636,7 @@ dependencies = [ [[package]] name = "limbo_time" -version = "0.0.20" +version = "0.0.19" dependencies = [ "chrono", "limbo_ext", @@ -649,7 +648,7 @@ dependencies = [ [[package]] name = "limbo_uuid" -version = "0.0.20" +version = "0.0.19" dependencies = [ "limbo_ext", "mimalloc", @@ -662,12 +661,6 @@ version = "0.4.15" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d26c52dbd32dccf2d10cac7725f8eae5296885fb5703b261f7d0a0739ec807ab" -[[package]] -name = "linux-raw-sys" -version = "0.9.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cd945864f07fe9f5371a27ad7b52a172b4b499999f1d97574c9fa68373937e12" - [[package]] name = "litemap" version = "0.7.5" @@ -698,20 +691,21 @@ checksum = "78ca9ab1a0babb1e7d5695e3530886289c18cf2f87ec19a575a0abdce112e3a3" [[package]] name = "miette" -version = "7.6.0" +version = "7.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5f98efec8807c63c752b5bd61f862c165c115b0a35685bdcfd9238c7aeb592b7" +checksum = "1a955165f87b37fd1862df2a59547ac542c77ef6d17c666f619d1ad22dd89484" dependencies = [ "cfg-if", "miette-derive", + "thiserror 1.0.69", "unicode-width", ] [[package]] name = "miette-derive" -version = "7.6.0" +version = "7.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "db5b29714e950dbb20d5e6f74f9dcec4edbcc1067bb7f8ed198c097b8c1a818b" +checksum = "bf45bf44ab49be92fd1227a3be6fc6f617f1a337c06af54981048574d8783147" dependencies = [ "proc-macro2", "quote", @@ -720,9 +714,9 @@ dependencies = [ [[package]] name = "mimalloc" -version = "0.1.46" +version = "0.1.43" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "995942f432bbb4822a7e9c3faa87a695185b0d09273ba85f097b54f4e458f2af" +checksum = "68914350ae34959d83f732418d51e2427a794055d0b9529f48259ac07af65633" dependencies = [ "libmimalloc-sys", ] @@ -832,7 +826,7 @@ dependencies = [ "concurrent-queue", "hermit-abi", "pin-project-lite", - "rustix 0.38.44", + "rustix", "tracing", "windows-sys", ] @@ -955,20 +949,7 @@ dependencies = [ "bitflags", "errno", "libc", - "linux-raw-sys 0.4.15", - "windows-sys", -] - -[[package]] -name = "rustix" -version = "1.0.7" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c71e83d6afe7ff64890ec6b71d6a69bb8a610ab78ce364b3352876bb4c801266" -dependencies = [ - "bitflags", - "errno", - "libc", - "linux-raw-sys 0.9.4", + "linux-raw-sys", "windows-sys", ] From 6d0a3c95c6acb4e1c6a4e8d9e7b0392b43e412a0 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Wed, 21 May 2025 10:15:51 -0300 Subject: [PATCH 11/33] more tests! --- testing/alter_table.test | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/testing/alter_table.test b/testing/alter_table.test index f58664f32..0144aa093 100755 --- a/testing/alter_table.test +++ b/testing/alter_table.test @@ -29,6 +29,15 @@ do_execsql_test_on_specific_db {:memory:} alter-table-add-column { "1|" } +do_execsql_test_on_specific_db {:memory:} alter-table-add-column-typed { + CREATE TABLE t(a); + ALTER TABLE t ADD b TEXT; + ALTER TABLE t ADD c INTEGER DEFAULT(0); + SELECT sql FROM sqlite_schema; +} { + "CREATE TABLE t(a, b TEXT, c INTEGER DEFAULT(0))" +} + do_execsql_test_on_specific_db {:memory:} alter-table-drop-column { CREATE TABLE t(a, b); INSERT INTO t VALUES (1, 2); From 6945c0c09e084ca12383a56a5c64a6a38b266d88 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Wed, 21 May 2025 16:15:55 -0300 Subject: [PATCH 12/33] fix+refactor: incorrect label placement also added a `cursor_loop` helper on `ProgramBuilder` to avoid making this mistake in the future. this is zero-cost, and will be optimized to the same thing (hopefully). --- core/translate/mod.rs | 89 +++++++++++++++++----------------------- core/vdbe/builder.rs | 20 +++++++++ testing/alter_table.test | 18 ++++++-- 3 files changed, 73 insertions(+), 54 deletions(-) diff --git a/core/translate/mod.rs b/core/translate/mod.rs index 4e6ed992f..9c1e81641 100644 --- a/core/translate/mod.rs +++ b/core/translate/mod.rs @@ -188,9 +188,6 @@ pub fn translate_inner( syms, program, |program| { - let loop_start = program.allocate_label(); - let loop_end = program.allocate_label(); - let column_count = btree.columns.len(); let root_page = btree.root_page; let table_name = btree.name.clone(); @@ -205,62 +202,50 @@ pub fn translate_inner( name: table_name.clone(), }); - program.emit_insn(Insn::Rewind { - cursor_id, - pc_if_empty: loop_end, - }); + program.cursor_loop(cursor_id, |program| { + let rowid = program.alloc_register(); - let rowid = program.alloc_register(); - - program.emit_insn(Insn::RowId { - cursor_id, - dest: rowid, - }); - - program.preassign_label_to_next_insn(loop_start); - - let first_column = program.alloc_registers(column_count); - - let mut iter = first_column; - - for i in 0..(column_count + 1) { - if i == dropped_col { - continue; - } - - program.emit_insn(Insn::Column { + program.emit_insn(Insn::RowId { cursor_id, - column: i, - dest: iter, + dest: rowid, }); - iter += 1; - } + let first_column = program.alloc_registers(column_count); - let record = program.alloc_register(); + let mut iter = first_column; - program.emit_insn(Insn::MakeRecord { - start_reg: first_column, - count: column_count, - dest_reg: record, - index_name: None, + for i in 0..(column_count + 1) { + if i == dropped_col { + continue; + } + + program.emit_insn(Insn::Column { + cursor_id, + column: i, + dest: iter, + }); + + iter += 1; + } + + let record = program.alloc_register(); + + program.emit_insn(Insn::MakeRecord { + start_reg: first_column, + count: column_count, + dest_reg: record, + index_name: None, + }); + + program.emit_insn(Insn::Insert { + cursor: cursor_id, + key_reg: rowid, + record_reg: record, + flag: 0, + table_name: table_name.clone(), + }); }); - program.emit_insn(Insn::Insert { - cursor: cursor_id, - key_reg: rowid, - record_reg: record, - flag: 0, - table_name: table_name.clone(), - }); - - program.emit_insn(Insn::Next { - cursor_id, - pc_if_next: loop_start, - }); - - program.preassign_label_to_next_insn(loop_end); - program.emit_insn(Insn::ParseSchema { db: usize::MAX, // TODO: This value is unused, change when we do something with it where_clause: None, @@ -291,6 +276,7 @@ pub fn translate_inner( schema, &mut update, syms, + program, |program| { program.emit_insn(Insn::ParseSchema { db: usize::MAX, // TODO: This value is unused, change when we do something with it @@ -330,6 +316,7 @@ pub fn translate_inner( schema, &mut update, syms, + program, |program| { program.emit_insn(Insn::ParseSchema { db: usize::MAX, // TODO: This value is unused, change when we do something with it diff --git a/core/vdbe/builder.rs b/core/vdbe/builder.rs index 0da96dade..4492b520f 100644 --- a/core/vdbe/builder.rs +++ b/core/vdbe/builder.rs @@ -771,6 +771,26 @@ impl ProgramBuilder { self.table_references.contains_table(table) } + #[inline] + pub fn cursor_loop(&mut self, cursor_id: CursorID, f: impl Fn(&mut ProgramBuilder)) { + let loop_start = self.allocate_label(); + let loop_end = self.allocate_label(); + + self.emit_insn(Insn::Rewind { + cursor_id, + pc_if_empty: loop_end, + }); + self.preassign_label_to_next_insn(loop_start); + + f(self); + + self.emit_insn(Insn::Next { + cursor_id, + pc_if_next: loop_start, + }); + self.preassign_label_to_next_insn(loop_end); + } + pub fn build( mut self, database_header: Arc>, diff --git a/testing/alter_table.test b/testing/alter_table.test index 0144aa093..8035a475f 100755 --- a/testing/alter_table.test +++ b/testing/alter_table.test @@ -32,24 +32,36 @@ do_execsql_test_on_specific_db {:memory:} alter-table-add-column { do_execsql_test_on_specific_db {:memory:} alter-table-add-column-typed { CREATE TABLE t(a); ALTER TABLE t ADD b TEXT; - ALTER TABLE t ADD c INTEGER DEFAULT(0); + ALTER TABLE t ADD c INTEGER DEFAULT 0; + SELECT sql FROM sqlite_schema; + + INSERT INTO t VALUES (1, 'a'); + SELECT * FROM t; } { "CREATE TABLE t(a, b TEXT, c INTEGER DEFAULT(0))" + "1|a|0" } do_execsql_test_on_specific_db {:memory:} alter-table-drop-column { CREATE TABLE t(a, b); - INSERT INTO t VALUES (1, 2); + INSERT INTO t VALUES (1, 1), (2, 2), (3, 3); SELECT * FROM t; ALTER TABLE t DROP b; SELECT sql FROM sqlite_schema; + SELECT * FROM t; } { - "1|2" + "1|1" + "2|2" + "3|3" + "CREATE TABLE t(a)" + "1" + "2" + "3" } do_execsql_test_in_memory_any_error fail-alter-table-drop-unique-column { From 326a8b39db2cfdc296b5be0b9f878b0251a62b4b Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Thu, 22 May 2025 12:45:14 -0300 Subject: [PATCH 13/33] fix: default values not being converted to SQL --- core/schema.rs | 4 ++++ testing/alter_table.test | 9 ++++----- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/core/schema.rs b/core/schema.rs index dea630da4..f6d1aa7c1 100644 --- a/core/schema.rs +++ b/core/schema.rs @@ -249,6 +249,10 @@ impl BTreeTable { sql.push_str(column.name.as_ref().expect("column name is None")); sql.push(' '); sql.push_str(&column.ty.to_string()); + if let Some(default) = &column.default { + sql.push_str(" DEFAULT "); + sql.push_str(&default.to_string()); + } } sql.push_str(");\n"); sql diff --git a/testing/alter_table.test b/testing/alter_table.test index 8035a475f..508dfac16 100755 --- a/testing/alter_table.test +++ b/testing/alter_table.test @@ -31,16 +31,15 @@ do_execsql_test_on_specific_db {:memory:} alter-table-add-column { do_execsql_test_on_specific_db {:memory:} alter-table-add-column-typed { CREATE TABLE t(a); - ALTER TABLE t ADD b TEXT; - ALTER TABLE t ADD c INTEGER DEFAULT 0; + ALTER TABLE t ADD b DEFAULT 0; SELECT sql FROM sqlite_schema; - INSERT INTO t VALUES (1, 'a'); + INSERT INTO t (a) VALUES (1); SELECT * FROM t; } { - "CREATE TABLE t(a, b TEXT, c INTEGER DEFAULT(0))" - "1|a|0" + "CREATE TABLE t(a, b DEFAULT 0)" + "1|0" } do_execsql_test_on_specific_db {:memory:} alter-table-drop-column { From 7638b0dab7efb16892a2577aa3117d018c79d9f1 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Fri, 23 May 2025 01:01:12 -0300 Subject: [PATCH 14/33] fix: use default value on empty columns added via ALTER TABLE --- core/schema.rs | 19 ++++++--- core/translate/emitter.rs | 1 + core/translate/index.rs | 1 + core/translate/main_loop.rs | 2 + core/translate/mod.rs | 22 +++++++++- core/translate/optimizer/join.rs | 5 +++ core/translate/optimizer/mod.rs | 1 + core/vdbe/execute.rs | 73 ++++++++++++++++++++++++++++---- testing/alter_table.test | 19 +++++++++ 9 files changed, 129 insertions(+), 14 deletions(-) diff --git a/core/schema.rs b/core/schema.rs index f6d1aa7c1..3a75e0f3b 100644 --- a/core/schema.rs +++ b/core/schema.rs @@ -955,6 +955,7 @@ pub struct IndexColumn { /// b.pos_in_table == 1 pub pos_in_table: usize, pub collation: Option, + pub default: Option, } impl Index { @@ -979,12 +980,13 @@ impl Index { name, index_name, table.name ))); }; - let collation = table.get_column(&name).unwrap().1.collation; + let (_, column) = table.get_column(&name).unwrap(); index_columns.push(IndexColumn { name, order: col.order.unwrap_or(SortOrder::Asc), pos_in_table, - collation, + collation: column.collation, + default: column.default.clone(), }); } Ok(Index { @@ -1041,11 +1043,14 @@ impl Index { ); }; + let (_, column) = table.get_column(col_name).unwrap(); + IndexColumn { name: normalize_ident(col_name), order: order.clone(), pos_in_table, - collation: table.get_column(col_name).unwrap().1.collation, + collation: column.collation, + default: column.default.clone(), } }) .collect::>(); @@ -1077,6 +1082,7 @@ impl Index { return None; } let (index_name, root_page) = auto_indices.next().expect("number of auto_indices in schema should be same number of indices calculated"); + let (_, column) = table.get_column(col_name).unwrap(); Some(Index { name: normalize_ident(index_name.as_str()), table_name: table.name.clone(), @@ -1085,7 +1091,8 @@ impl Index { name: normalize_ident(col_name), order: SortOrder::Asc, // Default Sort Order pos_in_table, - collation: table.get_column(col_name).unwrap().1.collation, + collation: column.collation, + default: column.default.clone(), }], unique: true, ephemeral: false, @@ -1147,11 +1154,13 @@ impl Index { col_name, index_name, table.name ); }; + let (_, column) = table.get_column(col_name).unwrap(); IndexColumn { name: normalize_ident(col_name), order: *order, pos_in_table, - collation: table.get_column(col_name).unwrap().1.collation, + collation: column.collation, + default: column.default.clone(), } }); Index { diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs index 30224147b..9a31e51c3 100644 --- a/core/translate/emitter.rs +++ b/core/translate/emitter.rs @@ -396,6 +396,7 @@ fn get_union_dedupe_index( order: SortOrder::Asc, pos_in_table: 0, collation: None, // FIXME: this should be inferred + default: None, }) .collect(), name: "union_dedupe".to_string(), diff --git a/core/translate/index.rs b/core/translate/index.rs index d8edfa591..5ea8eaffd 100644 --- a/core/translate/index.rs +++ b/core/translate/index.rs @@ -57,6 +57,7 @@ pub fn translate_create_index( order: *order, pos_in_table: *pos_in_table, collation: col.collation, + default: col.default.clone(), }) .collect(), unique: unique_if_not_exists.0, diff --git a/core/translate/main_loop.rs b/core/translate/main_loop.rs index 17868d9a7..885c4edce 100644 --- a/core/translate/main_loop.rs +++ b/core/translate/main_loop.rs @@ -87,6 +87,7 @@ pub fn init_distinct(program: &mut ProgramBuilder, plan: &mut SelectPlan) { order: SortOrder::Asc, pos_in_table: i, collation: None, // FIXME: this should be determined based on the result column expression! + default: None, // FIXME: this should be determined based on the result column expression! }) .collect(), unique: false, @@ -140,6 +141,7 @@ pub fn init_loop( order: SortOrder::Asc, pos_in_table: 0, collation: None, // FIXME: this should be inferred from the expression + default: None, // FIXME: this should be inferred from the expression }], has_rowid: false, unique: false, diff --git a/core/translate/mod.rs b/core/translate/mod.rs index 9c1e81641..4f7de8cf4 100644 --- a/core/translate/mod.rs +++ b/core/translate/mod.rs @@ -254,7 +254,27 @@ pub fn translate_inner( )? } ast::AlterTableBody::AddColumn(col_def) => { - btree.columns.push(Column::from(col_def)); + let column = Column::from(col_def); + + if let Some(default) = &column.default { + if !matches!( + default, + ast::Expr::Literal( + ast::Literal::Null + | ast::Literal::Blob(_) + | ast::Literal::Numeric(_) + | ast::Literal::String(_) + ) + ) { + // TODO: This is slightly inaccurate since sqlite returns a `Runtime + // error`. + return Err(LimboError::ParseError( + "Cannot add a column with non-constant default".to_string(), + )); + } + } + + btree.columns.push(column); let sql = btree.to_sql(); diff --git a/core/translate/optimizer/join.rs b/core/translate/optimizer/join.rs index 2378b101e..98fb92da0 100644 --- a/core/translate/optimizer/join.rs +++ b/core/translate/optimizer/join.rs @@ -657,6 +657,7 @@ mod tests { order: SortOrder::Asc, pos_in_table: 0, collation: None, + default: None, }], unique: true, ephemeral: false, @@ -724,6 +725,7 @@ mod tests { order: SortOrder::Asc, pos_in_table: 0, collation: None, + default: None, }], unique: true, ephemeral: false, @@ -839,6 +841,7 @@ mod tests { order: SortOrder::Asc, pos_in_table: 0, collation: None, + default: None, }], unique: true, ephemeral: false, @@ -855,6 +858,7 @@ mod tests { order: SortOrder::Asc, pos_in_table: 1, collation: None, + default: None, }], unique: false, ephemeral: false, @@ -869,6 +873,7 @@ mod tests { order: SortOrder::Asc, pos_in_table: 1, collation: None, + default: None, }], unique: false, ephemeral: false, diff --git a/core/translate/optimizer/mod.rs b/core/translate/optimizer/mod.rs index 71c156687..e99bc8895 100644 --- a/core/translate/optimizer/mod.rs +++ b/core/translate/optimizer/mod.rs @@ -776,6 +776,7 @@ fn ephemeral_index_build( order: SortOrder::Asc, pos_in_table: i, collation: c.collation, + default: c.default.clone(), }) // only include columns that are used in the query .filter(|c| table_reference.column_is_used(c.pos_in_table)) diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index ae0fe12ea..28d7cebed 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -6,7 +6,7 @@ use crate::storage::page_cache::DumbLruPageCache; use crate::storage::pager::CreateBTreeFlags; use crate::storage::wal::DummyWAL; use crate::translate::collate::CollationSeq; -use crate::types::ImmutableRecord; +use crate::types::{ImmutableRecord, Text}; use crate::{ error::{LimboError, SQLITE_CONSTRAINT, SQLITE_CONSTRAINT_PRIMARYKEY}, ext::ExtValue, @@ -53,6 +53,7 @@ use super::{ insn::{Cookie, RegisterOrLiteral}, CommitState, }; +use limbo_sqlite3_parser::ast; use parking_lot::RwLock; use rand::thread_rng; @@ -1322,7 +1323,7 @@ pub fn op_column( let (_, cursor_type) = program.cursor_ref.get(*cursor_id).unwrap(); match cursor_type { CursorType::BTreeTable(_) | CursorType::BTreeIndex(_) => { - let value = { + let value = 'value: { let mut cursor = must_be_btree_cursor!(*cursor_id, program.cursor_ref, state, "Column"); let cursor = cursor.as_btree_mut(); @@ -1339,21 +1340,77 @@ pub fn op_column( } else { RefValue::Null }; - value + + if cursor.get_null_flag() { + break 'value Value::Null; + } + + if let Some(value) = record.get_value_opt(*column) { + break 'value value.to_owned(); + } + + // WARNING(levy): Work-around for the P4 parameter. This should be reworked to be + // more similar to sqlite: when emiting a OP_Column, also add a pre-computed + // contant default value from the schema.' + + let literal = { + match cursor_type { + CursorType::BTreeIndex(index) => { + let Some(ast::Expr::Literal(literal)) = &index.columns[*column].default + else { + panic!("column added by ALTER TABLE should be constant") + }; + literal + } + CursorType::BTreeTable(btree) => { + let Some(ast::Expr::Literal(literal)) = &btree.columns[*column].default + else { + panic!("column added by ALTER TABLE should be constant") + }; + literal + } + _ => unreachable!(), + } + }; + + use crate::translate::expr::sanitize_string; + + match literal { + ast::Literal::Numeric(s) => match Numeric::from(s) { + Numeric::Null => Value::Null, + Numeric::Integer(v) => Value::Integer(v), + Numeric::Float(v) => Value::Float(v.into()), + }, + ast::Literal::Null => Value::Null, + ast::Literal::String(s) => Value::Text(Text::from_str(sanitize_string(s))), + ast::Literal::Blob(s) => Value::Blob( + // Taken from `translate_expr` + s.as_bytes() + .chunks_exact(2) + .map(|pair| { + // We assume that sqlite3-parser has already validated that + // the input is valid hex string, thus unwrap is safe. + let hex_byte = std::str::from_utf8(pair).unwrap(); + u8::from_str_radix(hex_byte, 16).unwrap() + }) + .collect(), + ), + _ => panic!("column added by ALTER TABLE should be constant"), + } }; // If we are copying a text/blob, let's try to simply update size of text if we need to allocate more and reuse. match (&value, &mut state.registers[*dest]) { - (RefValue::Text(text_ref), Register::Value(Value::Text(text_reg))) => { + (Value::Text(text_ref), Register::Value(Value::Text(text_reg))) => { text_reg.value.clear(); - text_reg.value.extend_from_slice(text_ref.value.to_slice()); + text_reg.value.extend_from_slice(text_ref.value.as_slice()); } - (RefValue::Blob(raw_slice), Register::Value(Value::Blob(blob_reg))) => { + (Value::Blob(raw_slice), Register::Value(Value::Blob(blob_reg))) => { blob_reg.clear(); - blob_reg.extend_from_slice(raw_slice.to_slice()); + blob_reg.extend_from_slice(raw_slice.as_slice()); } _ => { let reg = &mut state.registers[*dest]; - *reg = Register::Value(value.to_owned()); + *reg = Register::Value(value); } } } diff --git a/testing/alter_table.test b/testing/alter_table.test index 508dfac16..ec0056a72 100755 --- a/testing/alter_table.test +++ b/testing/alter_table.test @@ -42,6 +42,25 @@ do_execsql_test_on_specific_db {:memory:} alter-table-add-column-typed { "1|0" } +do_execsql_test_on_specific_db {:memory:} alter-table-add-column-default { + CREATE TABLE test(a); + INSERT INTO test VALUES (1), (2), (3); + + ALTER TABLE test ADD b DEFAULT 0.1; + SELECT * FROM test; + + CREATE INDEX idx ON test (b); + SELECT b FROM test WHERE b = 0.1; +} { + "1|0.1" + "2|0.1" + "3|0.1" + + "0.1" + "0.1" + "0.1" +} + do_execsql_test_on_specific_db {:memory:} alter-table-drop-column { CREATE TABLE t(a, b); INSERT INTO t VALUES (1, 1), (2, 2), (3, 3); From 15e0cab8d824b42727addb43469d54857d8020c8 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Fri, 23 May 2025 20:10:01 -0300 Subject: [PATCH 15/33] refactor+fix: precompute default values from schema --- core/translate/emitter.rs | 49 ++++++++++++++------------------ core/translate/expr.rs | 7 ++--- core/translate/group_by.rs | 18 ++---------- core/translate/index.rs | 18 ++---------- core/translate/insert.rs | 10 +++---- core/translate/main_loop.rs | 6 +--- core/translate/mod.rs | 15 +++++----- core/translate/order_by.rs | 8 +++--- core/translate/schema.rs | 50 ++++++++++----------------------- core/vdbe/builder.rs | 56 ++++++++++++++++++++++++++++++++++++- core/vdbe/execute.rs | 53 ++--------------------------------- core/vdbe/explain.rs | 3 +- core/vdbe/insn.rs | 2 ++ 13 files changed, 123 insertions(+), 172 deletions(-) diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs index 9a31e51c3..d5cddd7bd 100644 --- a/core/translate/emitter.rs +++ b/core/translate/emitter.rs @@ -438,11 +438,7 @@ fn read_deduplicated_union_rows( } else { dedupe_cols_start_reg }; - program.emit_insn(Insn::Column { - cursor_id: dedupe_cursor_id, - column: col_idx, - dest: start_reg + col_idx, - }); + program.emit_column(dedupe_cursor_id, col_idx, start_reg + col_idx); } if let Some(yield_reg) = yield_reg { program.emit_insn(Insn::Yield { @@ -797,11 +793,11 @@ fn emit_delete_insns( .iter() .enumerate() .for_each(|(reg_offset, column_index)| { - program.emit_insn(Insn::Column { - cursor_id: main_table_cursor_id, - column: column_index.pos_in_table, - dest: start_reg + reg_offset, - }); + program.emit_column( + main_table_cursor_id, + column_index.pos_in_table, + start_reg + reg_offset, + ); }); program.emit_insn(Insn::RowId { cursor_id: main_table_cursor_id, @@ -1121,20 +1117,17 @@ fn emit_update_insns( dest: target_reg, }); } else { - program.emit_insn(Insn::Column { - cursor_id: *index - .as_ref() - .and_then(|(_, id)| { - if column_idx_in_index.is_some() { - Some(id) - } else { - None - } - }) - .unwrap_or(&cursor_id), - column: column_idx_in_index.unwrap_or(idx), - dest: target_reg, - }); + let cursor_id = *index + .as_ref() + .and_then(|(_, id)| { + if column_idx_in_index.is_some() { + Some(id) + } else { + None + } + }) + .unwrap_or(&cursor_id); + program.emit_column(cursor_id, column_idx_in_index.unwrap_or(idx), target_reg); } } } @@ -1295,11 +1288,11 @@ fn emit_update_insns( .iter() .enumerate() .for_each(|(reg_offset, column_index)| { - program.emit_insn(Insn::Column { + program.emit_column( cursor_id, - column: column_index.pos_in_table, - dest: start_reg + reg_offset, - }); + column_index.pos_in_table, + start_reg + reg_offset, + ); }); program.emit_insn(Insn::RowId { diff --git a/core/translate/expr.rs b/core/translate/expr.rs index c1beb32d2..4d3669db0 100644 --- a/core/translate/expr.rs +++ b/core/translate/expr.rs @@ -1884,11 +1884,8 @@ pub fn translate_expr( } else { *column }; - program.emit_insn(Insn::Column { - cursor_id: read_cursor, - column, - dest: target_register, - }); + + program.emit_column(read_cursor, column, target_register); } let Some(column) = table.get_column_at(*column) else { crate::bail_parse_error!("column index out of bounds"); diff --git a/core/translate/group_by.rs b/core/translate/group_by.rs index d47f116b1..b3875b264 100644 --- a/core/translate/group_by.rs +++ b/core/translate/group_by.rs @@ -442,11 +442,7 @@ impl<'a> GroupByAggArgumentSource<'a> { dest_reg_start, .. } => { - program.emit_insn(Insn::Column { - cursor_id: *cursor_id, - column: *col_start, - dest: dest_reg_start + arg_idx, - }); + program.emit_column(*cursor_id, *col_start, dest_reg_start + arg_idx); Ok(dest_reg_start + arg_idx) } GroupByAggArgumentSource::Register { @@ -493,11 +489,7 @@ pub fn group_by_process_single_group( for i in 0..group_by.exprs.len() { let sorter_column_index = i; let group_reg = groups_start_reg + i; - program.emit_insn(Insn::Column { - cursor_id: *pseudo_cursor, - column: sorter_column_index, - dest: group_reg, - }); + program.emit_column(*pseudo_cursor, sorter_column_index, group_reg); } groups_start_reg } @@ -617,11 +609,7 @@ pub fn group_by_process_single_group( } => { for (sorter_column_index, dest_reg) in column_register_mapping.iter().enumerate() { if let Some(dest_reg) = dest_reg { - program.emit_insn(Insn::Column { - cursor_id: *pseudo_cursor, - column: sorter_column_index, - dest: *dest_reg, - }); + program.emit_column(*pseudo_cursor, sorter_column_index, *dest_reg); } } } diff --git a/core/translate/index.rs b/core/translate/index.rs index 5ea8eaffd..a645582f2 100644 --- a/core/translate/index.rs +++ b/core/translate/index.rs @@ -143,11 +143,7 @@ pub fn translate_create_index( // Then insert the record into the sorter let start_reg = program.alloc_registers(columns.len() + 1); for (i, (col, _)) in columns.iter().enumerate() { - program.emit_insn(Insn::Column { - cursor_id: table_cursor_id, - column: col.0, - dest: start_reg + i, - }); + program.emit_column(table_cursor_id, col.0, start_reg + i); } let rowid_reg = start_reg + columns.len(); program.emit_insn(Insn::RowId { @@ -372,11 +368,7 @@ pub fn translate_drop_index( // Read sqlite_schema.name into dest_reg let dest_reg = program.alloc_register(); - program.emit_insn(Insn::Column { - cursor_id: sqlite_schema_cursor_id, - column: 1, // sqlite_schema.name - dest: dest_reg, - }); + program.emit_column(sqlite_schema_cursor_id, 1, dest_reg); // if current column is not index_name then jump to Next // skip if sqlite_schema.name != index_name_reg @@ -391,11 +383,7 @@ pub fn translate_drop_index( // read type of table // skip if sqlite_schema.type != 'index' (index_str_reg) - program.emit_insn(Insn::Column { - cursor_id: sqlite_schema_cursor_id, - column: 0, - dest: dest_reg, - }); + program.emit_column(sqlite_schema_cursor_id, 0, dest_reg); // if current column is not index then jump to Next program.emit_insn(Insn::Ne { lhs: index_str_reg, diff --git a/core/translate/insert.rs b/core/translate/insert.rs index d664aca6c..14431ab04 100644 --- a/core/translate/insert.rs +++ b/core/translate/insert.rs @@ -721,11 +721,11 @@ fn populate_columns_multiple_rows( // Decrement as we have now seen a value index instead other_values_seen -= 1; if let Some(temp_table_ctx) = temp_table_ctx { - program.emit_insn(Insn::Column { - cursor_id: temp_table_ctx.cursor_id, - column: value_index_seen, - dest: column_registers_start + i, - }); + program.emit_column( + temp_table_ctx.cursor_id, + value_index_seen, + column_registers_start + i, + ); } else { program.emit_insn(Insn::Copy { src_reg: yield_reg + value_index_seen, diff --git a/core/translate/main_loop.rs b/core/translate/main_loop.rs index 885c4edce..fd8ec644f 100644 --- a/core/translate/main_loop.rs +++ b/core/translate/main_loop.rs @@ -1407,11 +1407,7 @@ fn emit_autoindex( let ephemeral_cols_start_reg = program.alloc_registers(num_regs_to_reserve); for (i, col) in index.columns.iter().enumerate() { let reg = ephemeral_cols_start_reg + i; - program.emit_insn(Insn::Column { - cursor_id: table_cursor_id, - column: col.pos_in_table, - dest: reg, - }); + program.emit_column(table_cursor_id, col.pos_in_table, reg); } if table_has_rowid { program.emit_insn(Insn::RowId { diff --git a/core/translate/mod.rs b/core/translate/mod.rs index 4f7de8cf4..aede373e5 100644 --- a/core/translate/mod.rs +++ b/core/translate/mod.rs @@ -121,8 +121,10 @@ pub fn translate_inner( ))); }; - let Some(btree) = table.btree() else { todo!() }; - let mut btree = (*btree).clone(); + let Some(original_btree) = table.btree() else { + todo!() + }; + let mut btree = (*original_btree).clone(); match alter_table { ast::AlterTableBody::DropColumn(column) => { @@ -193,7 +195,7 @@ pub fn translate_inner( let table_name = btree.name.clone(); let cursor_id = program.alloc_cursor_id( - crate::vdbe::builder::CursorType::BTreeTable(Rc::new(btree)), + crate::vdbe::builder::CursorType::BTreeTable(original_btree), ); program.emit_insn(Insn::OpenWrite { @@ -205,6 +207,7 @@ pub fn translate_inner( program.cursor_loop(cursor_id, |program| { let rowid = program.alloc_register(); + // FIXME: Handle tables without rowid. program.emit_insn(Insn::RowId { cursor_id, dest: rowid, @@ -219,11 +222,7 @@ pub fn translate_inner( continue; } - program.emit_insn(Insn::Column { - cursor_id, - column: i, - dest: iter, - }); + program.emit_column(cursor_id, i, iter); iter += 1; } diff --git a/core/translate/order_by.rs b/core/translate/order_by.rs index eed98cd3e..d710996d8 100644 --- a/core/translate/order_by.rs +++ b/core/translate/order_by.rs @@ -166,11 +166,11 @@ pub fn emit_order_by( let start_reg = t_ctx.reg_result_cols_start.unwrap(); for i in 0..result_columns.len() { let reg = start_reg + i; - program.emit_insn(Insn::Column { + program.emit_column( cursor_id, - column: t_ctx.result_column_indexes_in_orderby_sorter[i], - dest: reg, - }); + t_ctx.result_column_indexes_in_orderby_sorter[i], + reg, + ); } emit_result_row_and_limit( diff --git a/core/translate/schema.rs b/core/translate/schema.rs index 5d9435b06..ab96f24b6 100644 --- a/core/translate/schema.rs +++ b/core/translate/schema.rs @@ -665,11 +665,11 @@ pub fn translate_drop_table( program.preassign_label_to_next_insn(metadata_loop); // start loop on schema table - program.emit_insn(Insn::Column { - cursor_id: sqlite_schema_cursor_id_0, - column: 2, - dest: table_name_and_root_page_register, - }); + program.emit_column( + sqlite_schema_cursor_id_0, + 2, + table_name_and_root_page_register, + ); let next_label = program.allocate_label(); program.emit_insn(Insn::Ne { lhs: table_name_and_root_page_register, @@ -678,11 +678,11 @@ pub fn translate_drop_table( flags: CmpInsFlags::default(), collation: program.curr_collation(), }); - program.emit_insn(Insn::Column { - cursor_id: sqlite_schema_cursor_id_0, - column: 0, - dest: table_name_and_root_page_register, - }); + program.emit_column( + sqlite_schema_cursor_id_0, + 0, + table_name_and_root_page_register, + ); program.emit_insn(Insn::Eq { lhs: table_name_and_root_page_register, rhs: table_type, @@ -815,11 +815,7 @@ pub fn translate_drop_table( }); program.preassign_label_to_next_insn(copy_schema_to_temp_table_loop); // start loop on schema table - program.emit_insn(Insn::Column { - cursor_id: sqlite_schema_cursor_id_1, - column: 3, - dest: prev_root_page_register, - }); + program.emit_column(sqlite_schema_cursor_id_1, 3, prev_root_page_register); // The label and Insn::Ne are used to skip over any rows in the schema table that don't have the root page that was moved let next_label = program.allocate_label(); program.emit_insn(Insn::Ne { @@ -878,21 +874,9 @@ pub fn translate_drop_table( rowid_reg: schema_row_id_register, target_pc: next_label, }); - program.emit_insn(Insn::Column { - cursor_id: sqlite_schema_cursor_id_1, - column: 0, - dest: schema_column_0_register, - }); - program.emit_insn(Insn::Column { - cursor_id: sqlite_schema_cursor_id_1, - column: 1, - dest: schema_column_1_register, - }); - program.emit_insn(Insn::Column { - cursor_id: sqlite_schema_cursor_id_1, - column: 2, - dest: schema_column_2_register, - }); + program.emit_column(sqlite_schema_cursor_id_1, 0, schema_column_0_register); + program.emit_column(sqlite_schema_cursor_id_1, 1, schema_column_1_register); + program.emit_column(sqlite_schema_cursor_id_1, 2, schema_column_2_register); let root_page = table .get_root_page() .try_into() @@ -901,11 +885,7 @@ pub fn translate_drop_table( value: root_page, dest: moved_to_root_page_register, }); - program.emit_insn(Insn::Column { - cursor_id: sqlite_schema_cursor_id_1, - column: 4, - dest: schema_column_4_register, - }); + program.emit_column(sqlite_schema_cursor_id_1, 4, schema_column_4_register); program.emit_insn(Insn::MakeRecord { start_reg: schema_column_0_register, count: 5, diff --git a/core/vdbe/builder.rs b/core/vdbe/builder.rs index 4492b520f..c8082fd34 100644 --- a/core/vdbe/builder.rs +++ b/core/vdbe/builder.rs @@ -10,6 +10,7 @@ use tracing::{instrument, Level}; use crate::{ fast_lock::SpinLock, + numeric::Numeric, parameters::Parameters, schema::{BTreeTable, Index, PseudoTable, Table}, storage::sqlite3_ondisk::DatabaseHeader, @@ -18,7 +19,8 @@ use crate::{ emitter::TransactionMode, plan::{ResultSetColumn, TableReferences}, }, - Connection, VirtualTable, + types::Text, + Connection, Value, VirtualTable, }; pub struct TableRefIdCounter { next_free: TableInternalId, @@ -791,6 +793,58 @@ impl ProgramBuilder { self.preassign_label_to_next_insn(loop_end); } + pub fn emit_column(&mut self, cursor_id: CursorID, column: usize, out: usize) { + let (_, cursor_type) = self.cursor_ref.get(cursor_id).unwrap(); + + use crate::translate::expr::sanitize_string; + + let default = 'value: { + let default = match cursor_type { + CursorType::BTreeTable(btree) => &btree.columns[column].default, + CursorType::BTreeIndex(index) => &index.columns[column].default, + _ => break 'value None, + }; + + let Some(default) = default else { + break 'value None; + }; + + let ast::Expr::Literal(ref literal) = default else { + panic!("column added by ALTER TABLE should be constant") + }; + + Some(match literal { + ast::Literal::Numeric(s) => match Numeric::from(s) { + Numeric::Null => Value::Null, + Numeric::Integer(v) => Value::Integer(v), + Numeric::Float(v) => Value::Float(v.into()), + }, + ast::Literal::Null => Value::Null, + ast::Literal::String(s) => Value::Text(Text::from_str(sanitize_string(s))), + ast::Literal::Blob(s) => Value::Blob( + // Taken from `translate_expr` + s.as_bytes() + .chunks_exact(2) + .map(|pair| { + // We assume that sqlite3-parser has already validated that + // the input is valid hex string, thus unwrap is safe. + let hex_byte = std::str::from_utf8(pair).unwrap(); + u8::from_str_radix(hex_byte, 16).unwrap() + }) + .collect(), + ), + _ => break 'value None, + }) + }; + + self.emit_insn(Insn::Column { + cursor_id, + column, + dest: out, + default, + }); + } + pub fn build( mut self, database_header: Arc>, diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index 28d7cebed..6863fbdb0 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -53,7 +53,6 @@ use super::{ insn::{Cookie, RegisterOrLiteral}, CommitState, }; -use limbo_sqlite3_parser::ast; use parking_lot::RwLock; use rand::thread_rng; @@ -1289,6 +1288,7 @@ pub fn op_column( cursor_id, column, dest, + default, } = insn else { unreachable!("unexpected Insn {:?}", insn) @@ -1349,54 +1349,7 @@ pub fn op_column( break 'value value.to_owned(); } - // WARNING(levy): Work-around for the P4 parameter. This should be reworked to be - // more similar to sqlite: when emiting a OP_Column, also add a pre-computed - // contant default value from the schema.' - - let literal = { - match cursor_type { - CursorType::BTreeIndex(index) => { - let Some(ast::Expr::Literal(literal)) = &index.columns[*column].default - else { - panic!("column added by ALTER TABLE should be constant") - }; - literal - } - CursorType::BTreeTable(btree) => { - let Some(ast::Expr::Literal(literal)) = &btree.columns[*column].default - else { - panic!("column added by ALTER TABLE should be constant") - }; - literal - } - _ => unreachable!(), - } - }; - - use crate::translate::expr::sanitize_string; - - match literal { - ast::Literal::Numeric(s) => match Numeric::from(s) { - Numeric::Null => Value::Null, - Numeric::Integer(v) => Value::Integer(v), - Numeric::Float(v) => Value::Float(v.into()), - }, - ast::Literal::Null => Value::Null, - ast::Literal::String(s) => Value::Text(Text::from_str(sanitize_string(s))), - ast::Literal::Blob(s) => Value::Blob( - // Taken from `translate_expr` - s.as_bytes() - .chunks_exact(2) - .map(|pair| { - // We assume that sqlite3-parser has already validated that - // the input is valid hex string, thus unwrap is safe. - let hex_byte = std::str::from_utf8(pair).unwrap(); - u8::from_str_radix(hex_byte, 16).unwrap() - }) - .collect(), - ), - _ => panic!("column added by ALTER TABLE should be constant"), - } + default.clone().unwrap_or(Value::Null) }; // If we are copying a text/blob, let's try to simply update size of text if we need to allocate more and reuse. match (&value, &mut state.registers[*dest]) { @@ -1423,7 +1376,7 @@ pub fn op_column( if let Some(record) = record { state.registers[*dest] = Register::Value(match record.get_value_opt(*column) { Some(val) => val.to_owned(), - None => Value::Null, + None => default.clone().unwrap_or(Value::Null), }); } else { state.registers[*dest] = Register::Value(Value::Null); diff --git a/core/vdbe/explain.rs b/core/vdbe/explain.rs index 2e687598c..ccf5096d4 100644 --- a/core/vdbe/explain.rs +++ b/core/vdbe/explain.rs @@ -527,6 +527,7 @@ pub fn insn_to_str( cursor_id, column, dest, + default, } => { let cursor_type = &program.cursor_ref[*cursor_id].1; let column_name: Option<&String> = match cursor_type { @@ -550,7 +551,7 @@ pub fn insn_to_str( *cursor_id as i32, *column as i32, *dest as i32, - Value::build_text(""), + default.clone().unwrap_or_else(|| Value::build_text("")), 0, format!( "r[{}]={}.{}", diff --git a/core/vdbe/insn.rs b/core/vdbe/insn.rs index db319f936..8e8fcef18 100644 --- a/core/vdbe/insn.rs +++ b/core/vdbe/insn.rs @@ -9,6 +9,7 @@ use crate::{ schema::{Affinity, BTreeTable, Index}, storage::{pager::CreateBTreeFlags, wal::CheckpointMode}, translate::collate::CollationSeq, + Value, }; use limbo_macros::Description; use limbo_sqlite3_parser::ast::SortOrder; @@ -376,6 +377,7 @@ pub enum Insn { cursor_id: CursorID, column: usize, dest: usize, + default: Option, }, TypeCheck { From 41cb13aa74a292c606dd129addacc3c0c6d76d50 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Fri, 23 May 2025 23:50:02 -0300 Subject: [PATCH 16/33] fix: ignore non-constants --- core/vdbe/builder.rs | 6 +----- testing/alter_table.test | 8 ++++++++ 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/core/vdbe/builder.rs b/core/vdbe/builder.rs index c8082fd34..48bd62550 100644 --- a/core/vdbe/builder.rs +++ b/core/vdbe/builder.rs @@ -805,14 +805,10 @@ impl ProgramBuilder { _ => break 'value None, }; - let Some(default) = default else { + let Some(ast::Expr::Literal(ref literal)) = default else { break 'value None; }; - let ast::Expr::Literal(ref literal) = default else { - panic!("column added by ALTER TABLE should be constant") - }; - Some(match literal { ast::Literal::Numeric(s) => match Numeric::from(s) { Numeric::Null => Value::Null, diff --git a/testing/alter_table.test b/testing/alter_table.test index ec0056a72..2839b08ed 100755 --- a/testing/alter_table.test +++ b/testing/alter_table.test @@ -51,6 +51,10 @@ do_execsql_test_on_specific_db {:memory:} alter-table-add-column-default { CREATE INDEX idx ON test (b); SELECT b FROM test WHERE b = 0.1; + + ALTER TABLE test DROP a; + SELECT * FROM test; + } { "1|0.1" "2|0.1" @@ -59,6 +63,10 @@ do_execsql_test_on_specific_db {:memory:} alter-table-add-column-default { "0.1" "0.1" "0.1" + + "0.1" + "0.1" + "0.1" } do_execsql_test_on_specific_db {:memory:} alter-table-drop-column { From a7761e431b2520dd9d33b453af6491bf1227e379 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Sat, 24 May 2025 23:36:38 -0300 Subject: [PATCH 17/33] fix: escape string literals --- core/translate/mod.rs | 14 ++++++++++++-- testing/alter_table.test | 21 +++++++++++---------- 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/core/translate/mod.rs b/core/translate/mod.rs index aede373e5..714e8d81b 100644 --- a/core/translate/mod.rs +++ b/core/translate/mod.rs @@ -276,17 +276,27 @@ pub fn translate_inner( btree.columns.push(column); let sql = btree.to_sql(); + let mut escaped = String::with_capacity(sql.len()); + + for ch in sql.chars() { + match ch { + '\'' => escaped.push_str("''"), + ch => escaped.push(ch), + } + } let stmt = format!( r#" UPDATE {SQLITE_TABLEID} - SET sql = '{sql}' + SET sql = '{escaped}' WHERE name = '{table_name}' AND type = 'table' "#, ); let mut parser = Parser::new(stmt.as_bytes()); - let Some(ast::Cmd::Stmt(ast::Stmt::Update(mut update))) = parser.next()? else { + let Some(ast::Cmd::Stmt(ast::Stmt::Update(mut update))) = + parser.next().unwrap() + else { unreachable!(); }; diff --git a/testing/alter_table.test b/testing/alter_table.test index 2839b08ed..f2b8b6376 100755 --- a/testing/alter_table.test +++ b/testing/alter_table.test @@ -47,26 +47,27 @@ do_execsql_test_on_specific_db {:memory:} alter-table-add-column-default { INSERT INTO test VALUES (1), (2), (3); ALTER TABLE test ADD b DEFAULT 0.1; + ALTER TABLE test ADD c DEFAULT 'hello'; SELECT * FROM test; CREATE INDEX idx ON test (b); - SELECT b FROM test WHERE b = 0.1; + SELECT b, c FROM test WHERE b = 0.1; ALTER TABLE test DROP a; SELECT * FROM test; } { - "1|0.1" - "2|0.1" - "3|0.1" + "1|0.1|hello" + "2|0.1|hello" + "3|0.1|hello" - "0.1" - "0.1" - "0.1" + "0.1|hello" + "0.1|hello" + "0.1|hello" - "0.1" - "0.1" - "0.1" + "0.1|hello" + "0.1|hello" + "0.1|hello" } do_execsql_test_on_specific_db {:memory:} alter-table-drop-column { From d65fe0f9786864a438034c4cef97e8ea53d8a55b Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Sun, 25 May 2025 22:49:50 -0300 Subject: [PATCH 18/33] refactor: simplification and better names --- core/translate/mod.rs | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/core/translate/mod.rs b/core/translate/mod.rs index 714e8d81b..96cfadc36 100644 --- a/core/translate/mod.rs +++ b/core/translate/mod.rs @@ -115,15 +115,16 @@ pub fn translate_inner( let (table_name, alter_table) = *a; let ast::Name(table_name) = table_name.name; - let Some(table) = schema.tables.get(&table_name) else { + let Some(original_btree) = schema + .tables + .get(&table_name) + .and_then(|table| table.btree()) + else { return Err(LimboError::ParseError(format!( "no such table: {table_name}" ))); }; - let Some(original_btree) = table.btree() else { - todo!() - }; let mut btree = (*original_btree).clone(); match alter_table { @@ -139,22 +140,17 @@ pub fn translate_inner( ))); } - let (dropped_col, col) = btree - .columns - .iter() - .enumerate() - .find(|(_, Column { name, .. })| name.as_ref() == Some(&column)) - .ok_or_else(|| { - LimboError::ParseError(format!("no such column: \"{column}\"")) - })?; + let (dropped_index, column) = btree.get_column(name).ok_or_else(|| { + LimboError::ParseError(format!("no such column: \"{column}\"")) + })?; - if col.primary_key { + if column.primary_key { return Err(LimboError::ParseError(format!( "cannot drop column \"{column}\": PRIMARY KEY" ))); } - if col.unique + if column.unique || btree.unique_sets.as_ref().is_some_and(|set| { set.iter().any(|set| { set.iter().any(|(column_name, _)| column_name == &column) @@ -166,7 +162,7 @@ pub fn translate_inner( ))); } - btree.columns.remove(dropped_col); + btree.columns.remove(dropped_index); let sql = btree.to_sql(); @@ -218,7 +214,7 @@ pub fn translate_inner( let mut iter = first_column; for i in 0..(column_count + 1) { - if i == dropped_col { + if i == dropped_index { continue; } From 0bb725899d222cc01e4a316c6e1410d785b41955 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Sun, 25 May 2025 22:50:53 -0300 Subject: [PATCH 19/33] fix: set `is_rowid_alias` --- core/schema.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/schema.rs b/core/schema.rs index 3a75e0f3b..da39216f7 100644 --- a/core/schema.rs +++ b/core/schema.rs @@ -657,7 +657,7 @@ impl From for Column { notnull, ty_str, primary_key, - is_rowid_alias: false, + is_rowid_alias: primary_key && matches!(ty, Type::Integer), unique, collation, } From c2f25b6a1d722317e28ddfc6b9de610e947c7a82 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Fri, 30 May 2025 12:19:59 -0300 Subject: [PATCH 20/33] fix: proper identifier normalization and column constraints --- core/schema.rs | 43 ++++++++++++++++++++------- core/translate/mod.rs | 67 ++++++++++++++++++++++--------------------- 2 files changed, 66 insertions(+), 44 deletions(-) diff --git a/core/schema.rs b/core/schema.rs index da39216f7..c3b496d07 100644 --- a/core/schema.rs +++ b/core/schema.rs @@ -220,12 +220,24 @@ impl BTreeTable { /// then get_column("b") returns (1, &Column { .. }) pub fn get_column(&self, name: &str) -> Option<(usize, &Column)> { let name = normalize_ident(name); - for (i, column) in self.columns.iter().enumerate() { - if column.name.as_ref().map_or(false, |n| *n == name) { - return Some((i, column)); - } - } - None + + self.columns + .iter() + .enumerate() + .find(|(_, column)| column.name.as_ref() == Some(&name)) + } + + /// Returns the column position and column for a given column name. + /// Returns None if the column name is not found. + /// E.g. if table is CREATE TABLE t(a, b, c) + /// then get_column("b") returns (1, &Column { .. }) + pub fn get_column_mut(&mut self, name: &str) -> Option<(usize, &mut Column)> { + let name = normalize_ident(name); + + self.columns + .iter_mut() + .enumerate() + .find(|(_, column)| column.name.as_ref() == Some(&name)) } pub fn from_sql(sql: &str, root_page: usize) -> Result { @@ -240,21 +252,30 @@ impl BTreeTable { } pub fn to_sql(&self) -> String { - let mut sql = format!("CREATE TABLE {} (\n", self.name); + let mut sql = format!("CREATE TABLE {} (", self.name); for (i, column) in self.columns.iter().enumerate() { if i > 0 { - sql.push_str(",\n"); + sql.push_str(", "); } - sql.push_str(" "); + sql.push_str(" "); sql.push_str(column.name.as_ref().expect("column name is None")); sql.push(' '); sql.push_str(&column.ty.to_string()); + + if column.unique { + sql.push_str(" UNIQUE"); + } + + if column.primary_key { + sql.push_str(" PRIMARY KEY"); + } + if let Some(default) = &column.default { sql.push_str(" DEFAULT "); sql.push_str(&default.to_string()); } } - sql.push_str(");\n"); + sql.push_str(" )"); sql } @@ -1496,7 +1517,7 @@ mod tests { name TEXT, tbl_name TEXT, rootpage INTEGER, - sql TEXT); + sql TEXT) "#; let actual = sqlite_schema_table().to_sql(); assert_eq!(expected, actual); diff --git a/core/translate/mod.rs b/core/translate/mod.rs index 96cfadc36..10bd8d519 100644 --- a/core/translate/mod.rs +++ b/core/translate/mod.rs @@ -35,6 +35,7 @@ use crate::schema::{Column, Schema}; use crate::storage::pager::Pager; use crate::storage::sqlite3_ondisk::DatabaseHeader; use crate::translate::delete::translate_delete; +use crate::util::normalize_ident; use crate::vdbe::builder::{ProgramBuilder, ProgramBuilderOpts, QueryMode}; use crate::vdbe::insn::{Insn, RegisterOrLiteral}; use crate::vdbe::Program; @@ -116,8 +117,7 @@ pub fn translate_inner( let ast::Name(table_name) = table_name.name; let Some(original_btree) = schema - .tables - .get(&table_name) + .get_table(&table_name) .and_then(|table| table.btree()) else { return Err(LimboError::ParseError(format!( @@ -128,37 +128,39 @@ pub fn translate_inner( let mut btree = (*original_btree).clone(); match alter_table { - ast::AlterTableBody::DropColumn(column) => { - let ast::Name(column) = column; + ast::AlterTableBody::DropColumn(column_name) => { + let ast::Name(column_name) = column_name; // Tables always have at least one column. assert_ne!(btree.columns.len(), 0); if btree.columns.len() == 1 { return Err(LimboError::ParseError(format!( - "cannot drop column \"{column}\": no other columns exist" + "cannot drop column \"{column_name}\": no other columns exist" ))); } - let (dropped_index, column) = btree.get_column(name).ok_or_else(|| { - LimboError::ParseError(format!("no such column: \"{column}\"")) - })?; + let (dropped_index, column) = + btree.get_column(&column_name).ok_or_else(|| { + LimboError::ParseError(format!("no such column: \"{column_name}\"")) + })?; if column.primary_key { return Err(LimboError::ParseError(format!( - "cannot drop column \"{column}\": PRIMARY KEY" + "cannot drop column \"{column_name}\": PRIMARY KEY" ))); } if column.unique || btree.unique_sets.as_ref().is_some_and(|set| { set.iter().any(|set| { - set.iter().any(|(column_name, _)| column_name == &column) + set.iter() + .any(|(name, _)| name == &normalize_ident(&column_name)) }) }) { return Err(LimboError::ParseError(format!( - "cannot drop column \"{column}\": UNIQUE" + "cannot drop column \"{column_name}\": UNIQUE" ))); } @@ -170,12 +172,12 @@ pub fn translate_inner( r#" UPDATE {SQLITE_TABLEID} SET sql = '{sql}' - WHERE name = '{table_name}' AND type = 'table' + WHERE name = '{table_name}' COLLATE NOCASE AND type = 'table' "#, ); let mut parser = Parser::new(stmt.as_bytes()); - let Some(ast::Cmd::Stmt(ast::Stmt::Update(mut update))) = parser.next()? else { + let Some(ast::Cmd::Stmt(ast::Stmt::Update(mut update))) = parser.next().unwrap() else { unreachable!(); }; @@ -285,7 +287,7 @@ pub fn translate_inner( r#" UPDATE {SQLITE_TABLEID} SET sql = '{escaped}' - WHERE name = '{table_name}' AND type = 'table' + WHERE name = '{table_name}' COLLATE NOCASE AND type = 'table' "#, ); @@ -311,15 +313,14 @@ pub fn translate_inner( )? } ast::AlterTableBody::RenameColumn { old, new } => { - let Some(column) = btree - .columns - .iter_mut() - .find(|column| column.name == Some(old.0.clone())) - else { + let ast::Name(old) = old; + let ast::Name(new) = new; + + let Some((_, column)) = btree.get_column_mut(&old) else { return Err(LimboError::ParseError(format!("no such column: \"{old}\""))); }; - column.name = Some(new.0.clone()); + column.name = Some(new); let sql = btree.to_sql(); @@ -327,12 +328,12 @@ pub fn translate_inner( r#" UPDATE {SQLITE_TABLEID} SET sql = '{sql}' - WHERE name = '{table_name}' AND type = 'table' + WHERE name = '{table_name}' COLLATE NOCASE AND type = 'table' "#, ); let mut parser = Parser::new(stmt.as_bytes()); - let Some(ast::Cmd::Stmt(ast::Stmt::Update(mut update))) = parser.next()? else { + let Some(ast::Cmd::Stmt(ast::Stmt::Update(mut update))) = parser.next().unwrap() else { unreachable!(); }; @@ -350,32 +351,32 @@ pub fn translate_inner( }, )? } - ast::AlterTableBody::RenameTo(name) => { - let ast::Name(rename) = name; + ast::AlterTableBody::RenameTo(new_name) => { + let ast::Name(new_name) = new_name; - if schema.tables.contains_key(&rename) { + if schema.get_table(&new_name).is_some() { return Err(LimboError::ParseError(format!( - "there is already another table or index with this name: {rename}" + "there is already another table or index with this name: {new_name}" ))); }; - btree.name = rename; + btree.name = new_name; let sql = btree.to_sql(); let stmt = format!( r#" UPDATE {SQLITE_TABLEID} - SET name = '{rename}' - , tbl_name = '{rename}' - , sql = '{sql}' - WHERE name = '{table_name}' AND type = 'table' + SET name = CASE WHEN type = 'table' THEN '{new_name}' ELSE name END + , tbl_name = '{new_name}' + , sql = CASE WHEN type = 'table' THEN '{sql}' ELSE sql END + WHERE tbl_name = '{table_name}' COLLATE NOCASE "#, - rename = &btree.name, + new_name = &btree.name, ); let mut parser = Parser::new(stmt.as_bytes()); - let Some(ast::Cmd::Stmt(ast::Stmt::Update(mut update))) = parser.next()? else { + let Some(ast::Cmd::Stmt(ast::Stmt::Update(mut update))) = parser.next().unwrap() else { unreachable!(); }; From 49a6ddad973641de2eb5c01d663e2ce45c3b6d7c Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Tue, 3 Jun 2025 22:56:54 -0300 Subject: [PATCH 21/33] wip --- core/function.rs | 18 ++ core/schema.rs | 13 - core/translate/expr.rs | 3 +- core/translate/mod.rs | 220 +++++++++----- core/types.rs | 6 + core/vdbe/execute.rs | 275 ++++++++++++++++++ testing/alter_table.test | 10 +- vendored/sqlite3-parser/src/parser/ast/fmt.rs | 64 +++- 8 files changed, 512 insertions(+), 97 deletions(-) diff --git a/core/function.rs b/core/function.rs index 5a436465e..971c7dde2 100644 --- a/core/function.rs +++ b/core/function.rs @@ -554,6 +554,21 @@ impl Display for MathFunc { } } +#[derive(Debug)] +pub enum AlterTableFunc { + RenameTable, + RenameColumn, +} + +impl Display for AlterTableFunc { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + AlterTableFunc::RenameTable => write!(f, "limbo_rename_table"), + AlterTableFunc::RenameColumn => write!(f, "limbo_rename_column"), + } + } +} + #[derive(Debug)] pub enum Func { Agg(AggFunc), @@ -562,6 +577,7 @@ pub enum Func { Vector(VectorFunc), #[cfg(feature = "json")] Json(JsonFunc), + AlterTable(AlterTableFunc), External(Rc), } @@ -575,6 +591,7 @@ impl Display for Func { #[cfg(feature = "json")] Self::Json(json_func) => write!(f, "{}", json_func), Self::External(generic_func) => write!(f, "{}", generic_func), + Self::AlterTable(alter_func) => write!(f, "{}", alter_func), } } } @@ -595,6 +612,7 @@ impl Func { #[cfg(feature = "json")] Self::Json(json_func) => json_func.is_deterministic(), Self::External(external_func) => external_func.is_deterministic(), + Self::AlterTable(alter_func) => true, } } pub fn resolve_function(name: &str, arg_count: usize) -> Result { diff --git a/core/schema.rs b/core/schema.rs index c3b496d07..6f8ce22d6 100644 --- a/core/schema.rs +++ b/core/schema.rs @@ -227,19 +227,6 @@ impl BTreeTable { .find(|(_, column)| column.name.as_ref() == Some(&name)) } - /// Returns the column position and column for a given column name. - /// Returns None if the column name is not found. - /// E.g. if table is CREATE TABLE t(a, b, c) - /// then get_column("b") returns (1, &Column { .. }) - pub fn get_column_mut(&mut self, name: &str) -> Option<(usize, &mut Column)> { - let name = normalize_ident(name); - - self.columns - .iter_mut() - .enumerate() - .find(|(_, column)| column.name.as_ref() == Some(&name)) - } - pub fn from_sql(sql: &str, root_page: usize) -> Result { let mut parser = Parser::new(sql.as_bytes()); let cmd = parser.next()?; diff --git a/core/translate/expr.rs b/core/translate/expr.rs index 4d3669db0..c1494e702 100644 --- a/core/translate/expr.rs +++ b/core/translate/expr.rs @@ -6,7 +6,7 @@ use super::optimizer::Optimizable; use super::plan::TableReferences; #[cfg(feature = "json")] use crate::function::JsonFunc; -use crate::function::{Func, FuncCtx, MathFuncArity, ScalarFunc, VectorFunc}; +use crate::function::{AlterTableFunc, Func, FuncCtx, MathFuncArity, ScalarFunc, VectorFunc}; use crate::functions::datetime; use crate::schema::{Affinity, Table, Type}; use crate::util::{exprs_are_equivalent, normalize_ident, parse_numeric_literal}; @@ -1799,6 +1799,7 @@ pub fn translate_expr( Ok(target_register) } }, + Func::AlterTable(_) => unreachable!(), } } ast::Expr::FunctionCallStar { .. } => todo!(), diff --git a/core/translate/mod.rs b/core/translate/mod.rs index 10bd8d519..8eb5a4902 100644 --- a/core/translate/mod.rs +++ b/core/translate/mod.rs @@ -31,6 +31,7 @@ pub(crate) mod update; mod values; use crate::fast_lock::SpinLock; +use crate::function::{AlterTableFunc, Func}; use crate::schema::{Column, Schema}; use crate::storage::pager::Pager; use crate::storage::sqlite3_ondisk::DatabaseHeader; @@ -40,6 +41,7 @@ use crate::vdbe::builder::{ProgramBuilder, ProgramBuilderOpts, QueryMode}; use crate::vdbe::insn::{Insn, RegisterOrLiteral}; use crate::vdbe::Program; use crate::{bail_parse_error, Connection, LimboError, Result, SymbolTable}; +use emitter::TransactionMode; use fallible_iterator::FallibleIterator as _; use index::{translate_create_index, translate_drop_index}; use insert::translate_insert; @@ -109,7 +111,7 @@ pub fn translate_inner( stmt: ast::Stmt, syms: &SymbolTable, query_mode: QueryMode, - program: ProgramBuilder, + mut program: ProgramBuilder, ) -> Result { let program = match stmt { ast::Stmt::AlterTable(a) => { @@ -177,7 +179,9 @@ pub fn translate_inner( ); let mut parser = Parser::new(stmt.as_bytes()); - let Some(ast::Cmd::Stmt(ast::Stmt::Update(mut update))) = parser.next().unwrap() else { + let Some(ast::Cmd::Stmt(ast::Stmt::Update(mut update))) = + parser.next().unwrap() + else { unreachable!(); }; @@ -313,43 +317,88 @@ pub fn translate_inner( )? } ast::AlterTableBody::RenameColumn { old, new } => { - let ast::Name(old) = old; - let ast::Name(new) = new; + let ast::Name(rename_from) = old; + let ast::Name(rename_to) = new; - let Some((_, column)) = btree.get_column_mut(&old) else { - return Err(LimboError::ParseError(format!("no such column: \"{old}\""))); + if btree.get_column(&rename_from).is_none() { + return Err(LimboError::ParseError(format!("no such column: \"{rename_from}\""))); }; - column.name = Some(new); + let sqlite_schema = schema + .get_btree_table(SQLITE_TABLEID) + .expect("sqlite_schema should be on schema"); - let sql = btree.to_sql(); - - let stmt = format!( - r#" - UPDATE {SQLITE_TABLEID} - SET sql = '{sql}' - WHERE name = '{table_name}' COLLATE NOCASE AND type = 'table' - "#, + let cursor_id = program.alloc_cursor_id( + crate::vdbe::builder::CursorType::BTreeTable(sqlite_schema.clone()), ); - let mut parser = Parser::new(stmt.as_bytes()); - let Some(ast::Cmd::Stmt(ast::Stmt::Update(mut update))) = parser.next().unwrap() else { - unreachable!(); - }; + program.emit_insn(Insn::OpenWrite { + cursor_id, + root_page: RegisterOrLiteral::Literal(sqlite_schema.root_page), + name: sqlite_schema.name.clone(), + }); - translate_update_with_after( - QueryMode::Normal, - schema, - &mut update, - syms, - program, - |program| { - program.emit_insn(Insn::ParseSchema { - db: usize::MAX, // TODO: This value is unused, change when we do something with it - where_clause: None, - }); - }, - )? + program.cursor_loop(cursor_id, |program| { + let rowid = program.alloc_register(); + + program.emit_insn(Insn::RowId { + cursor_id, + dest: rowid, + }); + + let first_column = program.alloc_registers(5); + + for i in 0..5 { + program.emit_column(cursor_id, i, first_column + i); + } + + program.emit_string8_new_reg(table_name.clone()); + program.mark_last_insn_constant(); + + program.emit_string8_new_reg(rename_from.clone()); + program.mark_last_insn_constant(); + + program.emit_string8_new_reg(rename_to.clone()); + program.mark_last_insn_constant(); + + let out = program.alloc_registers(5); + + program.emit_insn(Insn::Function { + constant_mask: 0, + start_reg: first_column, + dest: out, + func: crate::function::FuncCtx { + func: Func::AlterTable(AlterTableFunc::RenameColumn), + arg_count: 8, + }, + }); + + let record = program.alloc_register(); + + program.emit_insn(Insn::MakeRecord { + start_reg: out, + count: 5, + dest_reg: record, + index_name: None, + }); + + program.emit_insn(Insn::Insert { + cursor: cursor_id, + key_reg: rowid, + record_reg: record, + flag: 0, + table_name: table_name.clone(), + }); + }); + + program.emit_insn(Insn::ParseSchema { + db: usize::MAX, // TODO: This value is unused, change when we do something with it + where_clause: None, + }); + + program.epilogue(TransactionMode::Write); + + program } ast::AlterTableBody::RenameTo(new_name) => { let ast::Name(new_name) = new_name; @@ -360,39 +409,78 @@ pub fn translate_inner( ))); }; - btree.name = new_name; + let sqlite_schema = schema + .get_btree_table(SQLITE_TABLEID) + .expect("sqlite_schema should be on schema"); - let sql = btree.to_sql(); - - let stmt = format!( - r#" - UPDATE {SQLITE_TABLEID} - SET name = CASE WHEN type = 'table' THEN '{new_name}' ELSE name END - , tbl_name = '{new_name}' - , sql = CASE WHEN type = 'table' THEN '{sql}' ELSE sql END - WHERE tbl_name = '{table_name}' COLLATE NOCASE - "#, - new_name = &btree.name, + let cursor_id = program.alloc_cursor_id( + crate::vdbe::builder::CursorType::BTreeTable(sqlite_schema.clone()), ); - let mut parser = Parser::new(stmt.as_bytes()); - let Some(ast::Cmd::Stmt(ast::Stmt::Update(mut update))) = parser.next().unwrap() else { - unreachable!(); - }; + program.emit_insn(Insn::OpenWrite { + cursor_id, + root_page: RegisterOrLiteral::Literal(sqlite_schema.root_page), + name: sqlite_schema.name.clone(), + }); - translate_update_with_after( - QueryMode::Normal, - schema, - &mut update, - syms, - program, - |program| { - program.emit_insn(Insn::ParseSchema { - db: usize::MAX, // TODO: This value is unused, change when we do something with it - where_clause: None, - }); - }, - )? + program.cursor_loop(cursor_id, |program| { + let rowid = program.alloc_register(); + + program.emit_insn(Insn::RowId { + cursor_id, + dest: rowid, + }); + + let first_column = program.alloc_registers(5); + + for i in 0..5 { + program.emit_column(cursor_id, i, first_column + i); + } + + program.emit_string8_new_reg(table_name.clone()); + program.mark_last_insn_constant(); + + program.emit_string8_new_reg(new_name.clone()); + program.mark_last_insn_constant(); + + let out = program.alloc_registers(5); + + program.emit_insn(Insn::Function { + constant_mask: 0, + start_reg: first_column, + dest: out, + func: crate::function::FuncCtx { + func: Func::AlterTable(AlterTableFunc::RenameTable), + arg_count: 7, + }, + }); + + let record = program.alloc_register(); + + program.emit_insn(Insn::MakeRecord { + start_reg: out, + count: 5, + dest_reg: record, + index_name: None, + }); + + program.emit_insn(Insn::Insert { + cursor: cursor_id, + key_reg: rowid, + record_reg: record, + flag: 0, + table_name: table_name.clone(), + }); + }); + + program.emit_insn(Insn::ParseSchema { + db: usize::MAX, // TODO: This value is unused, change when we do something with it + where_clause: None, + }); + + program.epilogue(TransactionMode::Write); + + program } } } @@ -481,13 +569,9 @@ pub fn translate_inner( )? .program } - ast::Stmt::Update(mut update) => translate_update( - query_mode, - schema, - &mut update, - syms, - program, - )?, + ast::Stmt::Update(mut update) => { + translate_update(query_mode, schema, &mut update, syms, program)? + } ast::Stmt::Vacuum(_, _) => bail_parse_error!("VACUUM not supported yet"), ast::Stmt::Insert(insert) => { let Insert { diff --git a/core/types.rs b/core/types.rs index 8498c6db1..d03bd8f22 100644 --- a/core/types.rs +++ b/core/types.rs @@ -93,6 +93,12 @@ impl Text { } } +impl AsRef for Text { + fn as_ref(&self) -> &str { + self.as_str() + } +} + impl From for Text { fn from(value: String) -> Self { Text { diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index 6863fbdb0..04813efeb 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -1,4 +1,5 @@ #![allow(unused_variables)] +use crate::function::AlterTableFunc; use crate::numeric::{NullableInteger, Numeric}; use crate::schema::Schema; use crate::storage::database::FileMemoryStorage; @@ -7,6 +8,7 @@ use crate::storage::pager::CreateBTreeFlags; use crate::storage::wal::DummyWAL; use crate::translate::collate::CollationSeq; use crate::types::{ImmutableRecord, Text}; +use crate::util::normalize_ident; use crate::{ error::{LimboError, SQLITE_CONSTRAINT, SQLITE_CONSTRAINT_PRIMARYKEY}, ext::ExtValue, @@ -53,6 +55,10 @@ use super::{ insn::{Cookie, RegisterOrLiteral}, CommitState, }; +use fallible_iterator::FallibleIterator; +use limbo_sqlite3_parser::ast; +use limbo_sqlite3_parser::ast::fmt::ToTokens; +use limbo_sqlite3_parser::lexer::sql::Parser; use parking_lot::RwLock; use rand::thread_rng; @@ -3769,6 +3775,275 @@ pub fn op_function( ), }, }, + crate::function::Func::AlterTable(alter_func) => { + let r#type = &state.registers[*start_reg + 0].get_owned_value().clone(); + + let Value::Text(name) = &state.registers[*start_reg + 1].get_owned_value() else { + panic!("sqlite_schema.name should be TEXT") + }; + let name = name.to_string(); + + let Value::Text(tbl_name) = &state.registers[*start_reg + 2].get_owned_value() else { + panic!("sqlite_schema.tbl_name should be TEXT") + }; + let tbl_name = tbl_name.to_string(); + + let Value::Integer(root_page) = + &state.registers[*start_reg + 3].get_owned_value().clone() + else { + panic!("sqlite_schema.root_page should be INTEGER") + }; + + let sql = &state.registers[*start_reg + 4].get_owned_value().clone(); + + let (new_name, new_tbl_name, new_sql) = match alter_func { + AlterTableFunc::RenameTable => { + let rename_from = { + match &state.registers[*start_reg + 5].get_owned_value() { + Value::Text(rename_from) => normalize_ident(rename_from.as_str()), + _ => panic!("rename_from parameter should be TEXT"), + } + }; + + let rename_to = { + match &state.registers[*start_reg + 6].get_owned_value() { + Value::Text(rename_to) => normalize_ident(rename_to.as_str()), + _ => panic!("rename_to parameter should be TEXT"), + } + }; + + let new_name = if let Some(column) = + &name.strip_prefix(&format!("sqlite_autoindex_{rename_from}_")) + { + format!("sqlite_autoindex_{rename_to}_{column}") + } else if name == rename_from { + rename_to.clone() + } else { + name + }; + + let new_tbl_name = if tbl_name == rename_from { + rename_to.clone() + } else { + tbl_name + }; + + let new_sql = 'sql: { + let Value::Text(sql) = sql else { + break 'sql None; + }; + + let mut parser = Parser::new(sql.as_str().as_bytes()); + let ast::Cmd::Stmt(stmt) = parser.next().unwrap().unwrap() else { + todo!() + }; + + match stmt { + ast::Stmt::CreateIndex { + unique, + if_not_exists, + idx_name, + tbl_name, + columns, + where_clause, + } => { + let table_name = normalize_ident(&tbl_name.0); + + if rename_from != table_name { + break 'sql None; + } + + Some( + ast::Stmt::CreateIndex { + unique, + if_not_exists, + idx_name, + tbl_name: ast::Name(rename_to), + columns, + where_clause, + } + .format() + .unwrap(), + ) + } + ast::Stmt::CreateTable { + temporary, + if_not_exists, + tbl_name, + body, + } => { + let table_name = normalize_ident(&tbl_name.name.0); + + if rename_from != table_name { + break 'sql None; + } + + Some( + ast::Stmt::CreateTable { + temporary, + if_not_exists, + tbl_name: ast::QualifiedName { + db_name: None, + name: ast::Name(rename_to), + alias: None, + }, + body, + } + .format() + .unwrap(), + ) + } + _ => todo!(), + } + }; + + (new_name, new_tbl_name, new_sql) + } + AlterTableFunc::RenameColumn => { + let table = { + match &state.registers[*start_reg + 5].get_owned_value() { + Value::Text(rename_to) => normalize_ident(rename_to.as_str()), + _ => panic!("table parameter should be TEXT"), + } + }; + + let rename_from = { + match &state.registers[*start_reg + 6].get_owned_value() { + Value::Text(rename_from) => normalize_ident(rename_from.as_str()), + _ => panic!("rename_from parameter should be TEXT"), + } + }; + + let rename_to = { + match &state.registers[*start_reg + 7].get_owned_value() { + Value::Text(rename_to) => normalize_ident(rename_to.as_str()), + _ => panic!("rename_to parameter should be TEXT"), + } + }; + + let new_sql = 'sql: { + if table != tbl_name { + break 'sql None; + } + + let Value::Text(sql) = sql else { + break 'sql None; + }; + + let mut parser = Parser::new(sql.as_str().as_bytes()); + let ast::Cmd::Stmt(stmt) = parser.next().unwrap().unwrap() else { + todo!() + }; + + match stmt { + ast::Stmt::CreateIndex { + unique, + if_not_exists, + idx_name, + tbl_name, + mut columns, + where_clause, + } => { + if table != normalize_ident(&tbl_name.0) { + break 'sql None; + } + + for column in &mut columns { + match &mut column.expr { + ast::Expr::Id(ast::Id(id)) + if normalize_ident(&id) == rename_from => + { + *id = rename_to.clone(); + } + _ => {} + } + } + + Some( + ast::Stmt::CreateIndex { + unique, + if_not_exists, + idx_name, + tbl_name, + columns, + where_clause, + } + .format() + .unwrap(), + ) + } + ast::Stmt::CreateTable { + temporary, + if_not_exists, + tbl_name, + body, + } => { + if table != normalize_ident(&tbl_name.name.0) { + break 'sql None; + } + + let ast::CreateTableBody::ColumnsAndConstraints { + mut columns, + constraints, + options, + } = *body + else { + todo!() + }; + + let column_index = columns + .get_index_of(&ast::Name(rename_from)) + .expect("column being renamed should be present"); + + let mut column_definition = + columns.get_index(column_index).unwrap().1.clone(); + + column_definition.col_name = ast::Name(rename_to.clone()); + + assert!(columns + .insert(ast::Name(rename_to), column_definition.clone()) + .is_none()); + + // Swaps indexes with the last one and pops the end, effectively + // replacing the entry. + columns.swap_remove_index(column_index).unwrap(); + + Some( + ast::Stmt::CreateTable { + temporary, + if_not_exists, + tbl_name, + body: Box::new( + ast::CreateTableBody::ColumnsAndConstraints { + columns, + constraints, + options, + }, + ), + } + .format() + .unwrap(), + ) + } + _ => todo!(), + } + }; + + (name, tbl_name, new_sql) + } + }; + + state.registers[*dest + 0] = Register::Value(r#type.clone()); + state.registers[*dest + 1] = Register::Value(Value::Text(Text::from(new_name))); + state.registers[*dest + 2] = Register::Value(Value::Text(Text::from(new_tbl_name))); + state.registers[*dest + 3] = Register::Value(Value::Integer(*root_page)); + + if let Some(new_sql) = new_sql { + state.registers[*dest + 4] = Register::Value(Value::Text(Text::from(new_sql))); + } else { + state.registers[*dest + 4] = Register::Value(sql.clone()); + } + } crate::function::Func::Agg(_) => { unreachable!("Aggregate functions should not be handled here") } diff --git a/testing/alter_table.test b/testing/alter_table.test index f2b8b6376..3d3b56053 100755 --- a/testing/alter_table.test +++ b/testing/alter_table.test @@ -4,16 +4,20 @@ set testdir [file dirname $argv0] source $testdir/tester.tcl do_execsql_test_on_specific_db {:memory:} alter-table-rename-table { - CREATE TABLE t1(x INTEGER PRIMARY KEY); + CREATE TABLE t1(x INTEGER PRIMARY KEY, u UNIQUE); ALTER TABLE t1 RENAME TO t2; - SELECT tbl_name FROM sqlite_schema; + SELECT name FROM sqlite_schema WHERE type = 'table'; } { "t2" } do_execsql_test_on_specific_db {:memory:} alter-table-rename-column { CREATE TABLE t(a); + CREATE INDEX i ON t(a); ALTER TABLE t RENAME a TO b; SELECT sql FROM sqlite_schema; -} { "CREATE TABLE t(b)" } +} { + "CREATE TABLE t(b)" + "CREATE INDEX i ON t(b)" +} do_execsql_test_on_specific_db {:memory:} alter-table-add-column { CREATE TABLE t(a); diff --git a/vendored/sqlite3-parser/src/parser/ast/fmt.rs b/vendored/sqlite3-parser/src/parser/ast/fmt.rs index 3c264d607..72f1b2a8b 100644 --- a/vendored/sqlite3-parser/src/parser/ast/fmt.rs +++ b/vendored/sqlite3-parser/src/parser/ast/fmt.rs @@ -46,6 +46,45 @@ impl TokenStream for FmtTokenStream<'_, '_> { } } +struct WriteTokenStream<'a, T: fmt::Write> { + write: &'a mut T, + spaced: bool, +} + +impl TokenStream for WriteTokenStream<'_, T> { + type Error = fmt::Error; + + fn append(&mut self, ty: TokenType, value: Option<&str>) -> fmt::Result { + if !self.spaced { + match ty { + TK_COMMA | TK_SEMI | TK_RP | TK_DOT => {} + _ => { + self.write.write_char(' ')?; + self.spaced = true; + } + }; + } + if ty == TK_BLOB { + self.write.write_char('X')?; + self.write.write_char('\'')?; + if let Some(str) = value { + self.write.write_str(str)?; + } + return self.write.write_char('\''); + } else if let Some(str) = ty.as_str() { + self.write.write_str(str)?; + self.spaced = ty == TK_LP || ty == TK_DOT; // str should not be whitespace + } + if let Some(str) = value { + // trick for pretty-print + self.spaced = str.bytes().all(|b| b.is_ascii_whitespace()); + self.write.write_str(str) + } else { + Ok(()) + } + } +} + /// Stream of token pub trait TokenStream { /// Potential error raised @@ -63,6 +102,19 @@ pub trait ToTokens { let mut s = FmtTokenStream { f, spaced: true }; self.to_tokens(&mut s) } + // Format AST node to string + fn format(&self) -> Result { + let mut s = String::new(); + + let mut w = WriteTokenStream { + write: &mut s, + spaced: true, + }; + + self.to_tokens(&mut w)?; + + Ok(s) + } } impl ToTokens for &T { @@ -77,18 +129,6 @@ impl ToTokens for String { } } -/* FIXME: does not work, find why -impl Display for dyn ToTokens { - fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - let mut s = FmtTokenStream { f, spaced: true }; - match self.to_tokens(&mut s) { - Err(_) => Err(fmt::Error), - Ok(()) => Ok(()), - } - } -} -*/ - impl ToTokens for Cmd { fn to_tokens(&self, s: &mut S) -> Result<(), S::Error> { match self { From 1881cd04b55a0c5b2059fb84463081e5dd753f06 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Tue, 3 Jun 2025 23:05:10 -0300 Subject: [PATCH 22/33] chore: fmt --- core/schema.rs | 2 +- core/translate/mod.rs | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/core/schema.rs b/core/schema.rs index 6f8ce22d6..d9a1f0ed0 100644 --- a/core/schema.rs +++ b/core/schema.rs @@ -253,7 +253,7 @@ impl BTreeTable { sql.push_str(" UNIQUE"); } - if column.primary_key { + if column.primary_key { sql.push_str(" PRIMARY KEY"); } diff --git a/core/translate/mod.rs b/core/translate/mod.rs index 8eb5a4902..f945a276d 100644 --- a/core/translate/mod.rs +++ b/core/translate/mod.rs @@ -321,7 +321,9 @@ pub fn translate_inner( let ast::Name(rename_to) = new; if btree.get_column(&rename_from).is_none() { - return Err(LimboError::ParseError(format!("no such column: \"{rename_from}\""))); + return Err(LimboError::ParseError(format!( + "no such column: \"{rename_from}\"" + ))); }; let sqlite_schema = schema From b88cb99ff041a271775edb414d5befdf864703a2 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Tue, 3 Jun 2025 23:14:07 -0300 Subject: [PATCH 23/33] fix warnings and some refactoring --- core/function.rs | 2 +- core/schema.rs | 8 +------ core/translate/expr.rs | 2 +- core/translate/schema.rs | 21 +++++++------------ vendored/sqlite3-parser/src/parser/ast/fmt.rs | 2 +- 5 files changed, 12 insertions(+), 23 deletions(-) diff --git a/core/function.rs b/core/function.rs index 971c7dde2..d3207ad0b 100644 --- a/core/function.rs +++ b/core/function.rs @@ -612,7 +612,7 @@ impl Func { #[cfg(feature = "json")] Self::Json(json_func) => json_func.is_deterministic(), Self::External(external_func) => external_func.is_deterministic(), - Self::AlterTable(alter_func) => true, + Self::AlterTable(_) => true, } } pub fn resolve_function(name: &str, arg_count: usize) -> Result { diff --git a/core/schema.rs b/core/schema.rs index d9a1f0ed0..2f2ad4fdf 100644 --- a/core/schema.rs +++ b/core/schema.rs @@ -1499,13 +1499,7 @@ mod tests { #[test] pub fn test_sqlite_schema() { - let expected = r#"CREATE TABLE sqlite_schema ( - type TEXT, - name TEXT, - tbl_name TEXT, - rootpage INTEGER, - sql TEXT) -"#; + let expected = r#"CREATE TABLE sqlite_schema (type TEXT, name TEXT, tbl_name TEXT, rootpage INTEGER, sql TEXT) "#; let actual = sqlite_schema_table().to_sql(); assert_eq!(expected, actual); } diff --git a/core/translate/expr.rs b/core/translate/expr.rs index c1494e702..dbcdb47ff 100644 --- a/core/translate/expr.rs +++ b/core/translate/expr.rs @@ -6,7 +6,7 @@ use super::optimizer::Optimizable; use super::plan::TableReferences; #[cfg(feature = "json")] use crate::function::JsonFunc; -use crate::function::{AlterTableFunc, Func, FuncCtx, MathFuncArity, ScalarFunc, VectorFunc}; +use crate::function::{Func, FuncCtx, MathFuncArity, ScalarFunc, VectorFunc}; use crate::functions::datetime; use crate::schema::{Affinity, Table, Type}; use crate::util::{exprs_are_equivalent, normalize_ident, parse_numeric_literal}; diff --git a/core/translate/schema.rs b/core/translate/schema.rs index ab96f24b6..fd8ee0ef3 100644 --- a/core/translate/schema.rs +++ b/core/translate/schema.rs @@ -1,5 +1,4 @@ use std::collections::HashSet; -use std::fmt::Display; use std::ops::Range; use std::rc::Rc; @@ -441,20 +440,16 @@ enum PrimaryKeyDefinitionType<'a> { }, } -struct TableFormatter<'a> { - body: &'a ast::CreateTableBody, -} - -impl Display for TableFormatter<'_> { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.body.to_fmt(f) - } -} - fn create_table_body_to_str(tbl_name: &ast::QualifiedName, body: &ast::CreateTableBody) -> String { let mut sql = String::new(); - let formatter = TableFormatter { body }; - sql.push_str(format!("CREATE TABLE {} {}", tbl_name.name.0, formatter).as_str()); + sql.push_str( + format!( + "CREATE TABLE {} {}", + tbl_name.name.0, + body.format().unwrap() + ) + .as_str(), + ); match body { ast::CreateTableBody::ColumnsAndConstraints { columns: _, diff --git a/vendored/sqlite3-parser/src/parser/ast/fmt.rs b/vendored/sqlite3-parser/src/parser/ast/fmt.rs index 72f1b2a8b..6598e71d7 100644 --- a/vendored/sqlite3-parser/src/parser/ast/fmt.rs +++ b/vendored/sqlite3-parser/src/parser/ast/fmt.rs @@ -102,7 +102,7 @@ pub trait ToTokens { let mut s = FmtTokenStream { f, spaced: true }; self.to_tokens(&mut s) } - // Format AST node to string + /// Format AST node to string fn format(&self) -> Result { let mut s = String::new(); From 54e8e7f097eb0c30c7f81697f60fe8860509e6f5 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Wed, 4 Jun 2025 09:28:05 -0300 Subject: [PATCH 24/33] fix spacing --- core/schema.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/schema.rs b/core/schema.rs index 2f2ad4fdf..dee8d3eb4 100644 --- a/core/schema.rs +++ b/core/schema.rs @@ -242,9 +242,9 @@ impl BTreeTable { let mut sql = format!("CREATE TABLE {} (", self.name); for (i, column) in self.columns.iter().enumerate() { if i > 0 { - sql.push_str(", "); + sql.push(','); } - sql.push_str(" "); + sql.push(' '); sql.push_str(column.name.as_ref().expect("column name is None")); sql.push(' '); sql.push_str(&column.ty.to_string()); @@ -1499,7 +1499,7 @@ mod tests { #[test] pub fn test_sqlite_schema() { - let expected = r#"CREATE TABLE sqlite_schema (type TEXT, name TEXT, tbl_name TEXT, rootpage INTEGER, sql TEXT) "#; + let expected = r#"CREATE TABLE sqlite_schema ( type TEXT, name TEXT, tbl_name TEXT, rootpage INTEGER, sql TEXT )"#; let actual = sqlite_schema_table().to_sql(); assert_eq!(expected, actual); } From 01a680b69e3ce271c395ba46bd25c46b969c92fc Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Wed, 4 Jun 2025 23:18:37 -0300 Subject: [PATCH 25/33] feat(fuzz)+fix: add schema fuzz testing and fix some bugs --- core/translate/mod.rs | 28 +--- core/vdbe/builder.rs | 16 +- fuzz/Cargo.lock | 57 ++++--- fuzz/Cargo.toml | 4 + fuzz/fuzz_targets/schema.rs | 310 ++++++++++++++++++++++++++++++++++++ 5 files changed, 370 insertions(+), 45 deletions(-) create mode 100644 fuzz/fuzz_targets/schema.rs diff --git a/core/translate/mod.rs b/core/translate/mod.rs index f945a276d..5d0a0875b 100644 --- a/core/translate/mod.rs +++ b/core/translate/mod.rs @@ -206,15 +206,7 @@ pub fn translate_inner( name: table_name.clone(), }); - program.cursor_loop(cursor_id, |program| { - let rowid = program.alloc_register(); - - // FIXME: Handle tables without rowid. - program.emit_insn(Insn::RowId { - cursor_id, - dest: rowid, - }); - + program.cursor_loop(cursor_id, |program, rowid| { let first_column = program.alloc_registers(column_count); let mut iter = first_column; @@ -340,14 +332,7 @@ pub fn translate_inner( name: sqlite_schema.name.clone(), }); - program.cursor_loop(cursor_id, |program| { - let rowid = program.alloc_register(); - - program.emit_insn(Insn::RowId { - cursor_id, - dest: rowid, - }); - + program.cursor_loop(cursor_id, |program, rowid| { let first_column = program.alloc_registers(5); for i in 0..5 { @@ -425,14 +410,7 @@ pub fn translate_inner( name: sqlite_schema.name.clone(), }); - program.cursor_loop(cursor_id, |program| { - let rowid = program.alloc_register(); - - program.emit_insn(Insn::RowId { - cursor_id, - dest: rowid, - }); - + program.cursor_loop(cursor_id, |program, rowid| { let first_column = program.alloc_registers(5); for i in 0..5 { diff --git a/core/vdbe/builder.rs b/core/vdbe/builder.rs index 48bd62550..8254695d8 100644 --- a/core/vdbe/builder.rs +++ b/core/vdbe/builder.rs @@ -774,7 +774,7 @@ impl ProgramBuilder { } #[inline] - pub fn cursor_loop(&mut self, cursor_id: CursorID, f: impl Fn(&mut ProgramBuilder)) { + pub fn cursor_loop(&mut self, cursor_id: CursorID, f: impl Fn(&mut ProgramBuilder, usize)) { let loop_start = self.allocate_label(); let loop_end = self.allocate_label(); @@ -784,7 +784,19 @@ impl ProgramBuilder { }); self.preassign_label_to_next_insn(loop_start); - f(self); + let rowid = self.alloc_register(); + + self.emit_insn(Insn::RowId { + cursor_id, + dest: rowid, + }); + + self.emit_insn(Insn::IsNull { + reg: rowid, + target_pc: loop_end, + }); + + f(self, rowid); self.emit_insn(Insn::Next { cursor_id, diff --git a/fuzz/Cargo.lock b/fuzz/Cargo.lock index 091feceb7..507975013 100644 --- a/fuzz/Cargo.lock +++ b/fuzz/Cargo.lock @@ -523,9 +523,9 @@ checksum = "8355be11b20d696c8f18f6cc018c4e372165b1fa8126cef092399c9951984ffa" [[package]] name = "libmimalloc-sys" -version = "0.1.39" +version = "0.1.42" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "23aa6811d3bd4deb8a84dde645f943476d13b248d818edcf8ce0b2f37f036b44" +checksum = "ec9d6fac27761dabcd4ee73571cdb06b7022dc99089acbe5435691edffaac0f4" dependencies = [ "cc", "libc", @@ -566,8 +566,9 @@ dependencies = [ [[package]] name = "limbo_core" -version = "0.0.19" +version = "0.0.21" dependencies = [ + "bitflags", "built", "cfg_block", "chrono", @@ -590,16 +591,18 @@ dependencies = [ "rand", "regex", "regex-syntax", - "rustix", + "rustix 1.0.7", "ryu", "strum", + "strum_macros", "thiserror 1.0.69", "tracing", + "uncased", ] [[package]] name = "limbo_ext" -version = "0.0.19" +version = "0.0.21" dependencies = [ "chrono", "getrandom 0.3.1", @@ -608,7 +611,7 @@ dependencies = [ [[package]] name = "limbo_macros" -version = "0.0.19" +version = "0.0.21" dependencies = [ "proc-macro2", "quote", @@ -617,7 +620,7 @@ dependencies = [ [[package]] name = "limbo_sqlite3_parser" -version = "0.0.19" +version = "0.0.21" dependencies = [ "bitflags", "cc", @@ -636,7 +639,7 @@ dependencies = [ [[package]] name = "limbo_time" -version = "0.0.19" +version = "0.0.21" dependencies = [ "chrono", "limbo_ext", @@ -648,7 +651,7 @@ dependencies = [ [[package]] name = "limbo_uuid" -version = "0.0.19" +version = "0.0.21" dependencies = [ "limbo_ext", "mimalloc", @@ -661,6 +664,12 @@ version = "0.4.15" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d26c52dbd32dccf2d10cac7725f8eae5296885fb5703b261f7d0a0739ec807ab" +[[package]] +name = "linux-raw-sys" +version = "0.9.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cd945864f07fe9f5371a27ad7b52a172b4b499999f1d97574c9fa68373937e12" + [[package]] name = "litemap" version = "0.7.5" @@ -691,21 +700,20 @@ checksum = "78ca9ab1a0babb1e7d5695e3530886289c18cf2f87ec19a575a0abdce112e3a3" [[package]] name = "miette" -version = "7.5.0" +version = "7.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1a955165f87b37fd1862df2a59547ac542c77ef6d17c666f619d1ad22dd89484" +checksum = "5f98efec8807c63c752b5bd61f862c165c115b0a35685bdcfd9238c7aeb592b7" dependencies = [ "cfg-if", "miette-derive", - "thiserror 1.0.69", "unicode-width", ] [[package]] name = "miette-derive" -version = "7.5.0" +version = "7.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bf45bf44ab49be92fd1227a3be6fc6f617f1a337c06af54981048574d8783147" +checksum = "db5b29714e950dbb20d5e6f74f9dcec4edbcc1067bb7f8ed198c097b8c1a818b" dependencies = [ "proc-macro2", "quote", @@ -714,9 +722,9 @@ dependencies = [ [[package]] name = "mimalloc" -version = "0.1.43" +version = "0.1.46" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "68914350ae34959d83f732418d51e2427a794055d0b9529f48259ac07af65633" +checksum = "995942f432bbb4822a7e9c3faa87a695185b0d09273ba85f097b54f4e458f2af" dependencies = [ "libmimalloc-sys", ] @@ -826,7 +834,7 @@ dependencies = [ "concurrent-queue", "hermit-abi", "pin-project-lite", - "rustix", + "rustix 0.38.44", "tracing", "windows-sys", ] @@ -949,7 +957,20 @@ dependencies = [ "bitflags", "errno", "libc", - "linux-raw-sys", + "linux-raw-sys 0.4.15", + "windows-sys", +] + +[[package]] +name = "rustix" +version = "1.0.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c71e83d6afe7ff64890ec6b71d6a69bb8a610ab78ce364b3352876bb4c801266" +dependencies = [ + "bitflags", + "errno", + "libc", + "linux-raw-sys 0.9.4", "windows-sys", ] diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index 69d6f438f..7c157b8eb 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -18,6 +18,10 @@ rusqlite = { version = "0.34.0", features = ["bundled"] } [workspace] members = ["."] +[[bin]] +name = "schema" +path = "fuzz_targets/schema.rs" + [[bin]] name = "expression" path = "fuzz_targets/expression.rs" diff --git a/fuzz/fuzz_targets/schema.rs b/fuzz/fuzz_targets/schema.rs new file mode 100644 index 000000000..15413b258 --- /dev/null +++ b/fuzz/fuzz_targets/schema.rs @@ -0,0 +1,310 @@ +#![no_main] +use core::fmt; +use std::{error::Error, num::NonZero, sync::Arc}; + +use arbitrary::Arbitrary; +use libfuzzer_sys::{fuzz_target, Corpus}; +use limbo_core::{Value, IO as _}; +use rusqlite::ffi::SQLITE_STATIC; + +#[derive(Debug, Clone, PartialEq, Eq)] +struct Id(String); + +impl<'a> Arbitrary<'a> for Id { + fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result { + let len: usize = u.int_in_range(1..=10)?; + let is_quoted = bool::arbitrary(u)?; + + let mut out = String::with_capacity(len + if is_quoted { 2 } else { 0 }); + + if is_quoted { + out.push('"'); + } + + for _ in 0..len { + out.push(u.choose(b"abcdefghijklnmopqrstuvwxyz")?.clone() as char); + } + + if is_quoted { + out.push('"'); + } + + Ok(Id(out)) + } +} + +impl fmt::Display for Id { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0) + } +} + +#[derive(Debug, Arbitrary, Clone)] +enum Type { + None, + Integer, + Text, + Real, + Blob, + Custom(Id), +} + +impl fmt::Display for Type { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Type::None => Ok(()), + Type::Integer => write!(f, "INTEGER"), + Type::Text => write!(f, "TEXT"), + Type::Real => write!(f, "REAL"), + Type::Blob => write!(f, "BLOB"), + Type::Custom(id) => write!(f, "{}", id), + } + } +} + +#[derive(Debug, Arbitrary, Clone)] +struct ColumnDef { + name: Id, + r#type: Type, + unique: bool, +} + +impl fmt::Display for ColumnDef { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let ColumnDef { + name, + r#type, + unique, + } = self; + write!(f, "{name} {type}",)?; + + if *unique { + write!(f, " UNIQUE")?; + } + + // if *primary_key { + // write!(f, " PRIMARY KEY")?; + // } + + Ok(()) + } +} + +#[derive(Debug, Clone)] +struct Columns(Vec); + +impl<'a> Arbitrary<'a> for Columns { + fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result { + let len: usize = u.int_in_range(1..=4)?; + + let mut out: Vec = Vec::with_capacity(len); + + for i in 0..len { + out.push(ColumnDef { + name: Id(format!("c{i}")), + r#type: u.arbitrary()?, + unique: u.arbitrary()?, + }); + } + + Ok(Self(out)) + } +} + +impl fmt::Display for Columns { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + for (i, column) in self.0.iter().enumerate() { + if i > 0 { + write!(f, ", ")? + } + + write!(f, "{column}")? + } + + Ok(()) + } +} + +#[derive(Debug, Clone)] +struct TableDef { + name: Id, + columns: Columns, +} + +impl fmt::Display for TableDef { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let TableDef { name, columns } = self; + + write!(f, "CREATE TABLE {name} ( {columns} )") + } +} + +#[derive(Debug, Clone)] +struct IndexDef { + name: Id, + table: Id, + columns: Vec, +} + +impl fmt::Display for IndexDef { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let IndexDef { + name, + table, + columns, + } = self; + + todo!() + } +} + +#[derive(Debug)] +enum Op { + CreateTable(TableDef), + CreateIndex(IndexDef), + DropTable { table: Id }, + DropColumn { table: Id, column: Id }, + RenameTable { rename_from: Id, rename_to: Id }, +} + +impl fmt::Display for Op { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Op::CreateTable(table_def) => write!(f, "{table_def}"), + Op::CreateIndex(index_def) => write!(f, "{index_def}"), + Op::DropColumn { table, column } => { + write!(f, "ALTER TABLE {table} DROP COLUMN {column}") + } + Op::DropTable { table } => write!(f, "DROP TABLE {table}"), + Op::RenameTable { + rename_from, + rename_to, + } => write!(f, "ALTER TABLE {rename_from} RENAME TO {rename_to}"), + } + } +} + +#[derive(Debug)] +struct Ops(Vec); + +impl<'a> Arbitrary<'a> for Ops { + fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result { + let mut ops = Vec::new(); + let mut tables = Vec::new(); + + let mut drop_list = Vec::new(); + + let mut table_index: usize = 0; + + let num_ops = u.int_in_range(1..=10)?; + + for _ in 0..num_ops { + let op_type = if tables.is_empty() { + 0 + } else { + u.int_in_range(0..=2)? + }; + + match op_type { + 0 => { + let table_def = TableDef { + name: { + let out = format!("t{table_index}"); + table_index += 1; + + Id(out) + }, + columns: u.arbitrary()?, + }; + + ops.push(Op::CreateTable(table_def.clone())); + + tables.push(table_def); + } + 1 => { + let index = u.choose_index(tables.len())?; + + let table = &tables[index]; + + let rename_to = Id(format!("t{table_index}")); + table_index += 1; + + ops.push(Op::RenameTable { + rename_from: table.name.clone(), + rename_to: rename_to.clone(), + }); + + tables.push(TableDef { + name: rename_to, + columns: table.columns.clone(), + }); + + tables.remove(index); + } + 2 => { + let index = u.choose_index(tables.len())?; + + let table = &tables[index]; + + if table.columns.0.len() == 1 { + let table = tables.remove(index); + + ops.push(Op::DropTable { + table: table.name.clone(), + }); + + drop_list.push(table.name); + } else { + let table = &mut tables[index]; + + let index = u.choose_index(table.columns.0.len())?; + + ops.push(Op::DropColumn { + table: table.name.clone(), + column: table.columns.0.remove(index).name, + }); + } + } + _ => panic!(), + } + } + + Ok(Self(ops)) + } +} + +fn do_fuzz(Ops(ops): Ops) -> Result> { + dbg!(&ops); + + let rusqlite_conn = rusqlite::Connection::open_in_memory()?; + + let io = Arc::new(limbo_core::MemoryIO::new()); + let db = limbo_core::Database::open_file(io.clone(), ":memory:", true)?; + let limbo_conn = db.connect()?; + + for op in ops { + let sql = op.to_string(); + + dbg!(&sql); + + let expected = rusqlite_conn + .execute(&sql, ()) + .inspect_err(|_| { + dbg!(&sql); + }) + .unwrap(); + + let found = 'value: { + limbo_conn + .execute(&sql) + .inspect_err(|_| { + dbg!(&sql); + }) + .unwrap() + }; + } + + Ok(Corpus::Keep) +} + +fuzz_target!(|ops: Ops| -> Corpus { do_fuzz(ops).unwrap_or(Corpus::Keep) }); From dd0551b6f9b7da883124255298895532bdd79228 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Thu, 5 Jun 2025 16:38:31 -0300 Subject: [PATCH 26/33] improve fuzzing --- fuzz/fuzz_targets/expression.rs | 2 +- fuzz/fuzz_targets/schema.rs | 123 +++++++++++++++++++++++--------- 2 files changed, 92 insertions(+), 33 deletions(-) diff --git a/fuzz/fuzz_targets/expression.rs b/fuzz/fuzz_targets/expression.rs index bd838093b..4715e9673 100644 --- a/fuzz/fuzz_targets/expression.rs +++ b/fuzz/fuzz_targets/expression.rs @@ -184,7 +184,7 @@ fn do_fuzz(expr: Expr) -> Result> { let found = 'value: { let io = Arc::new(limbo_core::MemoryIO::new()); - let db = limbo_core::Database::open_file(io.clone(), ":memory:", true)?; + let db = limbo_core::Database::open_file(io.clone(), ":memory:", false)?; let conn = db.connect()?; let mut stmt = conn.prepare(sql)?; diff --git a/fuzz/fuzz_targets/schema.rs b/fuzz/fuzz_targets/schema.rs index 15413b258..b49de53b8 100644 --- a/fuzz/fuzz_targets/schema.rs +++ b/fuzz/fuzz_targets/schema.rs @@ -46,7 +46,6 @@ enum Type { Text, Real, Blob, - Custom(Id), } impl fmt::Display for Type { @@ -57,7 +56,6 @@ impl fmt::Display for Type { Type::Text => write!(f, "TEXT"), Type::Real => write!(f, "REAL"), Type::Blob => write!(f, "BLOB"), - Type::Custom(id) => write!(f, "{}", id), } } } @@ -66,7 +64,6 @@ impl fmt::Display for Type { struct ColumnDef { name: Id, r#type: Type, - unique: bool, } impl fmt::Display for ColumnDef { @@ -78,14 +75,6 @@ impl fmt::Display for ColumnDef { } = self; write!(f, "{name} {type}",)?; - if *unique { - write!(f, " UNIQUE")?; - } - - // if *primary_key { - // write!(f, " PRIMARY KEY")?; - // } - Ok(()) } } @@ -115,10 +104,10 @@ impl fmt::Display for Columns { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { for (i, column) in self.0.iter().enumerate() { if i > 0 { - write!(f, ", ")? + write!(f, ", ")?; } - write!(f, "{column}")? + write!(f, "{column}")?; } Ok(()) @@ -154,7 +143,19 @@ impl fmt::Display for IndexDef { columns, } = self; - todo!() + write!(f, "CREATE INDEX {name} ON {table}(")?; + + for (i, column) in self.columns.iter().enumerate() { + if i > 0 { + write!(f, ", ")?; + } + + write!(f, "{column}")?; + } + + write!(f, ")")?; + + Ok(()) } } @@ -162,9 +163,22 @@ impl fmt::Display for IndexDef { enum Op { CreateTable(TableDef), CreateIndex(IndexDef), - DropTable { table: Id }, - DropColumn { table: Id, column: Id }, - RenameTable { rename_from: Id, rename_to: Id }, + DropTable { + table: Id, + }, + DropColumn { + table: Id, + column: Id, + }, + RenameTable { + rename_from: Id, + rename_to: Id, + }, + RenameColumn { + table: Id, + rename_from: Id, + rename_to: Id, + }, } impl fmt::Display for Op { @@ -180,6 +194,16 @@ impl fmt::Display for Op { rename_from, rename_to, } => write!(f, "ALTER TABLE {rename_from} RENAME TO {rename_to}"), + Op::RenameColumn { + table, + rename_from, + rename_to, + } => { + write!( + f, + "ALTER TABLE {table} RENAME COLUMN {rename_from} TO {rename_to}" + ) + } } } } @@ -222,6 +246,21 @@ impl<'a> Arbitrary<'a> for Ops { tables.push(table_def); } 1 => { + let table = u.choose(&tables)?; + let index_def = IndexDef { + name: { + let out = format!("i{table_index}"); + table_index += 1; + + Id(out) + }, + table: table.name.clone(), + columns: vec![u.choose(&table.columns.0)?.name.clone()], + }; + + ops.push(Op::CreateIndex(index_def.clone())); + } + 2 => { let index = u.choose_index(tables.len())?; let table = &tables[index]; @@ -241,7 +280,7 @@ impl<'a> Arbitrary<'a> for Ops { tables.remove(index); } - 2 => { + 3 => { let index = u.choose_index(tables.len())?; let table = &tables[index]; @@ -265,6 +304,32 @@ impl<'a> Arbitrary<'a> for Ops { }); } } + 4 => { + let index = u.choose_index(tables.len())?; + + let table = &mut tables[index]; + + let index = u.choose_index(table.columns.0.len())?; + + let rename_to = Id(format!("cr{table_index}")); + table_index += 1; + + let column = table.columns.0[index].clone(); + + table.columns.0.insert( + index, + ColumnDef { + name: rename_to.clone(), + ..column + }, + ); + + ops.push(Op::RenameColumn { + table: table.name.clone(), + rename_from: column.name, + rename_to, + }); + } _ => panic!(), } } @@ -274,34 +339,28 @@ impl<'a> Arbitrary<'a> for Ops { } fn do_fuzz(Ops(ops): Ops) -> Result> { - dbg!(&ops); - let rusqlite_conn = rusqlite::Connection::open_in_memory()?; let io = Arc::new(limbo_core::MemoryIO::new()); - let db = limbo_core::Database::open_file(io.clone(), ":memory:", true)?; + let db = limbo_core::Database::open_file(io.clone(), ":memory:", false)?; let limbo_conn = db.connect()?; for op in ops { let sql = op.to_string(); - dbg!(&sql); - - let expected = rusqlite_conn + rusqlite_conn .execute(&sql, ()) .inspect_err(|_| { dbg!(&sql); }) .unwrap(); - let found = 'value: { - limbo_conn - .execute(&sql) - .inspect_err(|_| { - dbg!(&sql); - }) - .unwrap() - }; + limbo_conn + .execute(&sql) + .inspect_err(|_| { + dbg!(&sql); + }) + .unwrap() } Ok(Corpus::Keep) From 3b36c3e771038c5f0167f05020a4f3a16e392ca6 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Thu, 5 Jun 2025 17:01:01 -0300 Subject: [PATCH 27/33] refactor --- core/translate/alter.rs | 368 +++++++++++++++++++++++++++++++++++ core/translate/mod.rs | 371 +----------------------------------- fuzz/fuzz_targets/schema.rs | 8 +- 3 files changed, 378 insertions(+), 369 deletions(-) create mode 100644 core/translate/alter.rs diff --git a/core/translate/alter.rs b/core/translate/alter.rs new file mode 100644 index 000000000..710be6f7b --- /dev/null +++ b/core/translate/alter.rs @@ -0,0 +1,368 @@ +use fallible_iterator::FallibleIterator as _; +use limbo_sqlite3_parser::{ast, lexer::sql::Parser}; + +use crate::{ + function::{AlterTableFunc, Func}, + schema::{Column, Schema}, + util::normalize_ident, + vdbe::{ + builder::{ProgramBuilder, QueryMode}, + insn::{Insn, RegisterOrLiteral}, + }, + LimboError, Result, SymbolTable, +}; + +use super::{ + emitter::TransactionMode, schema::SQLITE_TABLEID, update::translate_update_with_after, +}; + +pub fn translate_alter_table( + alter: Box<(ast::QualifiedName, ast::AlterTableBody)>, + syms: &SymbolTable, + schema: &Schema, + mut program: ProgramBuilder, +) -> Result { + let (table_name, alter_table) = *alter; + let ast::Name(table_name) = table_name.name; + + let Some(original_btree) = schema + .get_table(&table_name) + .and_then(|table| table.btree()) + else { + return Err(LimboError::ParseError(format!( + "no such table: {table_name}" + ))); + }; + + let mut btree = (*original_btree).clone(); + + Ok(match alter_table { + ast::AlterTableBody::DropColumn(column_name) => { + let ast::Name(column_name) = column_name; + + // Tables always have at least one column. + assert_ne!(btree.columns.len(), 0); + + if btree.columns.len() == 1 { + return Err(LimboError::ParseError(format!( + "cannot drop column \"{column_name}\": no other columns exist" + ))); + } + + let (dropped_index, column) = btree.get_column(&column_name).ok_or_else(|| { + LimboError::ParseError(format!("no such column: \"{column_name}\"")) + })?; + + if column.primary_key { + return Err(LimboError::ParseError(format!( + "cannot drop column \"{column_name}\": PRIMARY KEY" + ))); + } + + if column.unique + || btree.unique_sets.as_ref().is_some_and(|set| { + set.iter().any(|set| { + set.iter() + .any(|(name, _)| name == &normalize_ident(&column_name)) + }) + }) + { + return Err(LimboError::ParseError(format!( + "cannot drop column \"{column_name}\": UNIQUE" + ))); + } + + btree.columns.remove(dropped_index); + + let sql = btree.to_sql(); + + let stmt = format!( + r#" + UPDATE {SQLITE_TABLEID} + SET sql = '{sql}' + WHERE name = '{table_name}' COLLATE NOCASE AND type = 'table' + "#, + ); + + let mut parser = Parser::new(stmt.as_bytes()); + let Some(ast::Cmd::Stmt(ast::Stmt::Update(mut update))) = parser.next().unwrap() else { + unreachable!(); + }; + + translate_update_with_after( + QueryMode::Normal, + schema, + &mut update, + syms, + program, + |program| { + let column_count = btree.columns.len(); + let root_page = btree.root_page; + let table_name = btree.name.clone(); + + let cursor_id = program.alloc_cursor_id( + crate::vdbe::builder::CursorType::BTreeTable(original_btree), + ); + + program.emit_insn(Insn::OpenWrite { + cursor_id, + root_page: RegisterOrLiteral::Literal(root_page), + name: table_name.clone(), + }); + + program.cursor_loop(cursor_id, |program, rowid| { + let first_column = program.alloc_registers(column_count); + + let mut iter = first_column; + + for i in 0..(column_count + 1) { + if i == dropped_index { + continue; + } + + program.emit_column(cursor_id, i, iter); + + iter += 1; + } + + let record = program.alloc_register(); + + program.emit_insn(Insn::MakeRecord { + start_reg: first_column, + count: column_count, + dest_reg: record, + index_name: None, + }); + + program.emit_insn(Insn::Insert { + cursor: cursor_id, + key_reg: rowid, + record_reg: record, + flag: 0, + table_name: table_name.clone(), + }); + }); + + program.emit_insn(Insn::ParseSchema { + db: usize::MAX, // TODO: This value is unused, change when we do something with it + where_clause: None, + }) + }, + )? + } + ast::AlterTableBody::AddColumn(col_def) => { + let column = Column::from(col_def); + + if let Some(default) = &column.default { + if !matches!( + default, + ast::Expr::Literal( + ast::Literal::Null + | ast::Literal::Blob(_) + | ast::Literal::Numeric(_) + | ast::Literal::String(_) + ) + ) { + // TODO: This is slightly inaccurate since sqlite returns a `Runtime + // error`. + return Err(LimboError::ParseError( + "Cannot add a column with non-constant default".to_string(), + )); + } + } + + btree.columns.push(column); + + let sql = btree.to_sql(); + let mut escaped = String::with_capacity(sql.len()); + + for ch in sql.chars() { + match ch { + '\'' => escaped.push_str("''"), + ch => escaped.push(ch), + } + } + + let stmt = format!( + r#" + UPDATE {SQLITE_TABLEID} + SET sql = '{escaped}' + WHERE name = '{table_name}' COLLATE NOCASE AND type = 'table' + "#, + ); + + let mut parser = Parser::new(stmt.as_bytes()); + let Some(ast::Cmd::Stmt(ast::Stmt::Update(mut update))) = parser.next().unwrap() else { + unreachable!(); + }; + + translate_update_with_after( + QueryMode::Normal, + schema, + &mut update, + syms, + program, + |program| { + program.emit_insn(Insn::ParseSchema { + db: usize::MAX, // TODO: This value is unused, change when we do something with it + where_clause: None, + }); + }, + )? + } + ast::AlterTableBody::RenameColumn { old, new } => { + let ast::Name(rename_from) = old; + let ast::Name(rename_to) = new; + + if btree.get_column(&rename_from).is_none() { + return Err(LimboError::ParseError(format!( + "no such column: \"{rename_from}\"" + ))); + }; + + let sqlite_schema = schema + .get_btree_table(SQLITE_TABLEID) + .expect("sqlite_schema should be on schema"); + + let cursor_id = program.alloc_cursor_id(crate::vdbe::builder::CursorType::BTreeTable( + sqlite_schema.clone(), + )); + + program.emit_insn(Insn::OpenWrite { + cursor_id, + root_page: RegisterOrLiteral::Literal(sqlite_schema.root_page), + name: sqlite_schema.name.clone(), + }); + + program.cursor_loop(cursor_id, |program, rowid| { + let first_column = program.alloc_registers(5); + + for i in 0..5 { + program.emit_column(cursor_id, i, first_column + i); + } + + program.emit_string8_new_reg(table_name.clone()); + program.mark_last_insn_constant(); + + program.emit_string8_new_reg(rename_from.clone()); + program.mark_last_insn_constant(); + + program.emit_string8_new_reg(rename_to.clone()); + program.mark_last_insn_constant(); + + let out = program.alloc_registers(5); + + program.emit_insn(Insn::Function { + constant_mask: 0, + start_reg: first_column, + dest: out, + func: crate::function::FuncCtx { + func: Func::AlterTable(AlterTableFunc::RenameColumn), + arg_count: 8, + }, + }); + + let record = program.alloc_register(); + + program.emit_insn(Insn::MakeRecord { + start_reg: out, + count: 5, + dest_reg: record, + index_name: None, + }); + + program.emit_insn(Insn::Insert { + cursor: cursor_id, + key_reg: rowid, + record_reg: record, + flag: 0, + table_name: table_name.clone(), + }); + }); + + program.emit_insn(Insn::ParseSchema { + db: usize::MAX, // TODO: This value is unused, change when we do something with it + where_clause: None, + }); + + program.epilogue(TransactionMode::Write); + + program + } + ast::AlterTableBody::RenameTo(new_name) => { + let ast::Name(new_name) = new_name; + + if schema.get_table(&new_name).is_some() { + return Err(LimboError::ParseError(format!( + "there is already another table or index with this name: {new_name}" + ))); + }; + + let sqlite_schema = schema + .get_btree_table(SQLITE_TABLEID) + .expect("sqlite_schema should be on schema"); + + let cursor_id = program.alloc_cursor_id(crate::vdbe::builder::CursorType::BTreeTable( + sqlite_schema.clone(), + )); + + program.emit_insn(Insn::OpenWrite { + cursor_id, + root_page: RegisterOrLiteral::Literal(sqlite_schema.root_page), + name: sqlite_schema.name.clone(), + }); + + program.cursor_loop(cursor_id, |program, rowid| { + let first_column = program.alloc_registers(5); + + for i in 0..5 { + program.emit_column(cursor_id, i, first_column + i); + } + + program.emit_string8_new_reg(table_name.clone()); + program.mark_last_insn_constant(); + + program.emit_string8_new_reg(new_name.clone()); + program.mark_last_insn_constant(); + + let out = program.alloc_registers(5); + + program.emit_insn(Insn::Function { + constant_mask: 0, + start_reg: first_column, + dest: out, + func: crate::function::FuncCtx { + func: Func::AlterTable(AlterTableFunc::RenameTable), + arg_count: 7, + }, + }); + + let record = program.alloc_register(); + + program.emit_insn(Insn::MakeRecord { + start_reg: out, + count: 5, + dest_reg: record, + index_name: None, + }); + + program.emit_insn(Insn::Insert { + cursor: cursor_id, + key_reg: rowid, + record_reg: record, + flag: 0, + table_name: table_name.clone(), + }); + }); + + program.emit_insn(Insn::ParseSchema { + db: usize::MAX, // TODO: This value is unused, change when we do something with it + where_clause: None, + }); + + program.epilogue(TransactionMode::Write); + + program + } + }) +} diff --git a/core/translate/mod.rs b/core/translate/mod.rs index 5d0a0875b..81550983c 100644 --- a/core/translate/mod.rs +++ b/core/translate/mod.rs @@ -8,6 +8,7 @@ //! will read rows from the database and filter them according to a WHERE clause. pub(crate) mod aggregation; +pub(crate) mod alter; pub(crate) mod collate; pub(crate) mod delete; pub(crate) mod display; @@ -31,31 +32,24 @@ pub(crate) mod update; mod values; use crate::fast_lock::SpinLock; -use crate::function::{AlterTableFunc, Func}; -use crate::schema::{Column, Schema}; +use crate::schema::Schema; use crate::storage::pager::Pager; use crate::storage::sqlite3_ondisk::DatabaseHeader; use crate::translate::delete::translate_delete; -use crate::util::normalize_ident; use crate::vdbe::builder::{ProgramBuilder, ProgramBuilderOpts, QueryMode}; -use crate::vdbe::insn::{Insn, RegisterOrLiteral}; use crate::vdbe::Program; -use crate::{bail_parse_error, Connection, LimboError, Result, SymbolTable}; -use emitter::TransactionMode; -use fallible_iterator::FallibleIterator as _; +use crate::{bail_parse_error, Connection, Result, SymbolTable}; +use alter::translate_alter_table; use index::{translate_create_index, translate_drop_index}; use insert::translate_insert; use limbo_sqlite3_parser::ast::{self, Delete, Insert}; -use limbo_sqlite3_parser::lexer::sql::Parser; -use schema::{ - translate_create_table, translate_create_virtual_table, translate_drop_table, SQLITE_TABLEID, -}; +use schema::{translate_create_table, translate_create_virtual_table, translate_drop_table}; use select::translate_select; use std::rc::{Rc, Weak}; use std::sync::Arc; use tracing::{instrument, Level}; use transaction::{translate_tx_begin, translate_tx_commit}; -use update::{translate_update, translate_update_with_after}; +use update::translate_update; #[instrument(skip_all, level = Level::TRACE)] pub fn translate( @@ -111,359 +105,10 @@ pub fn translate_inner( stmt: ast::Stmt, syms: &SymbolTable, query_mode: QueryMode, - mut program: ProgramBuilder, + program: ProgramBuilder, ) -> Result { let program = match stmt { - ast::Stmt::AlterTable(a) => { - let (table_name, alter_table) = *a; - let ast::Name(table_name) = table_name.name; - - let Some(original_btree) = schema - .get_table(&table_name) - .and_then(|table| table.btree()) - else { - return Err(LimboError::ParseError(format!( - "no such table: {table_name}" - ))); - }; - - let mut btree = (*original_btree).clone(); - - match alter_table { - ast::AlterTableBody::DropColumn(column_name) => { - let ast::Name(column_name) = column_name; - - // Tables always have at least one column. - assert_ne!(btree.columns.len(), 0); - - if btree.columns.len() == 1 { - return Err(LimboError::ParseError(format!( - "cannot drop column \"{column_name}\": no other columns exist" - ))); - } - - let (dropped_index, column) = - btree.get_column(&column_name).ok_or_else(|| { - LimboError::ParseError(format!("no such column: \"{column_name}\"")) - })?; - - if column.primary_key { - return Err(LimboError::ParseError(format!( - "cannot drop column \"{column_name}\": PRIMARY KEY" - ))); - } - - if column.unique - || btree.unique_sets.as_ref().is_some_and(|set| { - set.iter().any(|set| { - set.iter() - .any(|(name, _)| name == &normalize_ident(&column_name)) - }) - }) - { - return Err(LimboError::ParseError(format!( - "cannot drop column \"{column_name}\": UNIQUE" - ))); - } - - btree.columns.remove(dropped_index); - - let sql = btree.to_sql(); - - let stmt = format!( - r#" - UPDATE {SQLITE_TABLEID} - SET sql = '{sql}' - WHERE name = '{table_name}' COLLATE NOCASE AND type = 'table' - "#, - ); - - let mut parser = Parser::new(stmt.as_bytes()); - let Some(ast::Cmd::Stmt(ast::Stmt::Update(mut update))) = - parser.next().unwrap() - else { - unreachable!(); - }; - - translate_update_with_after( - QueryMode::Normal, - schema, - &mut update, - syms, - program, - |program| { - let column_count = btree.columns.len(); - let root_page = btree.root_page; - let table_name = btree.name.clone(); - - let cursor_id = program.alloc_cursor_id( - crate::vdbe::builder::CursorType::BTreeTable(original_btree), - ); - - program.emit_insn(Insn::OpenWrite { - cursor_id, - root_page: RegisterOrLiteral::Literal(root_page), - name: table_name.clone(), - }); - - program.cursor_loop(cursor_id, |program, rowid| { - let first_column = program.alloc_registers(column_count); - - let mut iter = first_column; - - for i in 0..(column_count + 1) { - if i == dropped_index { - continue; - } - - program.emit_column(cursor_id, i, iter); - - iter += 1; - } - - let record = program.alloc_register(); - - program.emit_insn(Insn::MakeRecord { - start_reg: first_column, - count: column_count, - dest_reg: record, - index_name: None, - }); - - program.emit_insn(Insn::Insert { - cursor: cursor_id, - key_reg: rowid, - record_reg: record, - flag: 0, - table_name: table_name.clone(), - }); - }); - - program.emit_insn(Insn::ParseSchema { - db: usize::MAX, // TODO: This value is unused, change when we do something with it - where_clause: None, - }) - }, - )? - } - ast::AlterTableBody::AddColumn(col_def) => { - let column = Column::from(col_def); - - if let Some(default) = &column.default { - if !matches!( - default, - ast::Expr::Literal( - ast::Literal::Null - | ast::Literal::Blob(_) - | ast::Literal::Numeric(_) - | ast::Literal::String(_) - ) - ) { - // TODO: This is slightly inaccurate since sqlite returns a `Runtime - // error`. - return Err(LimboError::ParseError( - "Cannot add a column with non-constant default".to_string(), - )); - } - } - - btree.columns.push(column); - - let sql = btree.to_sql(); - let mut escaped = String::with_capacity(sql.len()); - - for ch in sql.chars() { - match ch { - '\'' => escaped.push_str("''"), - ch => escaped.push(ch), - } - } - - let stmt = format!( - r#" - UPDATE {SQLITE_TABLEID} - SET sql = '{escaped}' - WHERE name = '{table_name}' COLLATE NOCASE AND type = 'table' - "#, - ); - - let mut parser = Parser::new(stmt.as_bytes()); - let Some(ast::Cmd::Stmt(ast::Stmt::Update(mut update))) = - parser.next().unwrap() - else { - unreachable!(); - }; - - translate_update_with_after( - QueryMode::Normal, - schema, - &mut update, - syms, - program, - |program| { - program.emit_insn(Insn::ParseSchema { - db: usize::MAX, // TODO: This value is unused, change when we do something with it - where_clause: None, - }); - }, - )? - } - ast::AlterTableBody::RenameColumn { old, new } => { - let ast::Name(rename_from) = old; - let ast::Name(rename_to) = new; - - if btree.get_column(&rename_from).is_none() { - return Err(LimboError::ParseError(format!( - "no such column: \"{rename_from}\"" - ))); - }; - - let sqlite_schema = schema - .get_btree_table(SQLITE_TABLEID) - .expect("sqlite_schema should be on schema"); - - let cursor_id = program.alloc_cursor_id( - crate::vdbe::builder::CursorType::BTreeTable(sqlite_schema.clone()), - ); - - program.emit_insn(Insn::OpenWrite { - cursor_id, - root_page: RegisterOrLiteral::Literal(sqlite_schema.root_page), - name: sqlite_schema.name.clone(), - }); - - program.cursor_loop(cursor_id, |program, rowid| { - let first_column = program.alloc_registers(5); - - for i in 0..5 { - program.emit_column(cursor_id, i, first_column + i); - } - - program.emit_string8_new_reg(table_name.clone()); - program.mark_last_insn_constant(); - - program.emit_string8_new_reg(rename_from.clone()); - program.mark_last_insn_constant(); - - program.emit_string8_new_reg(rename_to.clone()); - program.mark_last_insn_constant(); - - let out = program.alloc_registers(5); - - program.emit_insn(Insn::Function { - constant_mask: 0, - start_reg: first_column, - dest: out, - func: crate::function::FuncCtx { - func: Func::AlterTable(AlterTableFunc::RenameColumn), - arg_count: 8, - }, - }); - - let record = program.alloc_register(); - - program.emit_insn(Insn::MakeRecord { - start_reg: out, - count: 5, - dest_reg: record, - index_name: None, - }); - - program.emit_insn(Insn::Insert { - cursor: cursor_id, - key_reg: rowid, - record_reg: record, - flag: 0, - table_name: table_name.clone(), - }); - }); - - program.emit_insn(Insn::ParseSchema { - db: usize::MAX, // TODO: This value is unused, change when we do something with it - where_clause: None, - }); - - program.epilogue(TransactionMode::Write); - - program - } - ast::AlterTableBody::RenameTo(new_name) => { - let ast::Name(new_name) = new_name; - - if schema.get_table(&new_name).is_some() { - return Err(LimboError::ParseError(format!( - "there is already another table or index with this name: {new_name}" - ))); - }; - - let sqlite_schema = schema - .get_btree_table(SQLITE_TABLEID) - .expect("sqlite_schema should be on schema"); - - let cursor_id = program.alloc_cursor_id( - crate::vdbe::builder::CursorType::BTreeTable(sqlite_schema.clone()), - ); - - program.emit_insn(Insn::OpenWrite { - cursor_id, - root_page: RegisterOrLiteral::Literal(sqlite_schema.root_page), - name: sqlite_schema.name.clone(), - }); - - program.cursor_loop(cursor_id, |program, rowid| { - let first_column = program.alloc_registers(5); - - for i in 0..5 { - program.emit_column(cursor_id, i, first_column + i); - } - - program.emit_string8_new_reg(table_name.clone()); - program.mark_last_insn_constant(); - - program.emit_string8_new_reg(new_name.clone()); - program.mark_last_insn_constant(); - - let out = program.alloc_registers(5); - - program.emit_insn(Insn::Function { - constant_mask: 0, - start_reg: first_column, - dest: out, - func: crate::function::FuncCtx { - func: Func::AlterTable(AlterTableFunc::RenameTable), - arg_count: 7, - }, - }); - - let record = program.alloc_register(); - - program.emit_insn(Insn::MakeRecord { - start_reg: out, - count: 5, - dest_reg: record, - index_name: None, - }); - - program.emit_insn(Insn::Insert { - cursor: cursor_id, - key_reg: rowid, - record_reg: record, - flag: 0, - table_name: table_name.clone(), - }); - }); - - program.emit_insn(Insn::ParseSchema { - db: usize::MAX, // TODO: This value is unused, change when we do something with it - where_clause: None, - }); - - program.epilogue(TransactionMode::Write); - - program - } - } - } + ast::Stmt::AlterTable(alter) => translate_alter_table(alter, syms, schema, program)?, ast::Stmt::Analyze(_) => bail_parse_error!("ANALYZE not supported yet"), ast::Stmt::Attach { .. } => bail_parse_error!("ATTACH not supported yet"), ast::Stmt::Begin(tx_type, tx_name) => translate_tx_begin(tx_type, tx_name, program)?, diff --git a/fuzz/fuzz_targets/schema.rs b/fuzz/fuzz_targets/schema.rs index b49de53b8..c5ff3c731 100644 --- a/fuzz/fuzz_targets/schema.rs +++ b/fuzz/fuzz_targets/schema.rs @@ -1,11 +1,9 @@ #![no_main] use core::fmt; -use std::{error::Error, num::NonZero, sync::Arc}; +use std::{error::Error, sync::Arc}; use arbitrary::Arbitrary; use libfuzzer_sys::{fuzz_target, Corpus}; -use limbo_core::{Value, IO as _}; -use rusqlite::ffi::SQLITE_STATIC; #[derive(Debug, Clone, PartialEq, Eq)] struct Id(String); @@ -71,7 +69,6 @@ impl fmt::Display for ColumnDef { let ColumnDef { name, r#type, - unique, } = self; write!(f, "{name} {type}",)?; @@ -92,7 +89,6 @@ impl<'a> Arbitrary<'a> for Columns { out.push(ColumnDef { name: Id(format!("c{i}")), r#type: u.arbitrary()?, - unique: u.arbitrary()?, }); } @@ -145,7 +141,7 @@ impl fmt::Display for IndexDef { write!(f, "CREATE INDEX {name} ON {table}(")?; - for (i, column) in self.columns.iter().enumerate() { + for (i, column) in columns.iter().enumerate() { if i > 0 { write!(f, ", ")?; } From e3da5a1f09e3210c7219b4e1a06e6f5f37042bc6 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Thu, 5 Jun 2025 17:03:39 -0300 Subject: [PATCH 28/33] fix: clippy --- core/translate/alter.rs | 4 ++-- core/translate/mod.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/core/translate/alter.rs b/core/translate/alter.rs index 710be6f7b..84173d394 100644 --- a/core/translate/alter.rs +++ b/core/translate/alter.rs @@ -17,12 +17,12 @@ use super::{ }; pub fn translate_alter_table( - alter: Box<(ast::QualifiedName, ast::AlterTableBody)>, + alter: (ast::QualifiedName, ast::AlterTableBody), syms: &SymbolTable, schema: &Schema, mut program: ProgramBuilder, ) -> Result { - let (table_name, alter_table) = *alter; + let (table_name, alter_table) = alter; let ast::Name(table_name) = table_name.name; let Some(original_btree) = schema diff --git a/core/translate/mod.rs b/core/translate/mod.rs index 81550983c..a103f9aaf 100644 --- a/core/translate/mod.rs +++ b/core/translate/mod.rs @@ -108,7 +108,7 @@ pub fn translate_inner( program: ProgramBuilder, ) -> Result { let program = match stmt { - ast::Stmt::AlterTable(alter) => translate_alter_table(alter, syms, schema, program)?, + ast::Stmt::AlterTable(alter) => translate_alter_table(*alter, syms, schema, program)?, ast::Stmt::Analyze(_) => bail_parse_error!("ANALYZE not supported yet"), ast::Stmt::Attach { .. } => bail_parse_error!("ATTACH not supported yet"), ast::Stmt::Begin(tx_type, tx_name) => translate_tx_begin(tx_type, tx_name, program)?, From e7ccb0b70707067961cf43cf7219ee586ec95db8 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Thu, 5 Jun 2025 17:39:13 -0300 Subject: [PATCH 29/33] fix: prevent duplicate columns --- core/translate/alter.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/core/translate/alter.rs b/core/translate/alter.rs index 84173d394..e07ca6470 100644 --- a/core/translate/alter.rs +++ b/core/translate/alter.rs @@ -220,6 +220,12 @@ pub fn translate_alter_table( ))); }; + if btree.get_column(&rename_to).is_some() { + return Err(LimboError::ParseError(format!( + "duplicate column name: \"{rename_from}\"" + ))); + }; + let sqlite_schema = schema .get_btree_table(SQLITE_TABLEID) .expect("sqlite_schema should be on schema"); From 8ecc561cd3ee7d36322e5757deb8b9e6c2ce2a6e Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Fri, 6 Jun 2025 09:31:30 -0300 Subject: [PATCH 30/33] refactor: dereference impl Copy --- core/schema.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/schema.rs b/core/schema.rs index dee8d3eb4..3535c9237 100644 --- a/core/schema.rs +++ b/core/schema.rs @@ -1055,7 +1055,7 @@ impl Index { IndexColumn { name: normalize_ident(col_name), - order: order.clone(), + order: *order, pos_in_table, collation: column.collation, default: column.default.clone(), From 96e3f7bc5c8408bf942def8feca2df335c06348d Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Mon, 9 Jun 2025 19:07:36 -0300 Subject: [PATCH 31/33] refactor: remove magic number --- core/translate/alter.rs | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/core/translate/alter.rs b/core/translate/alter.rs index e07ca6470..5940b3d95 100644 --- a/core/translate/alter.rs +++ b/core/translate/alter.rs @@ -241,9 +241,12 @@ pub fn translate_alter_table( }); program.cursor_loop(cursor_id, |program, rowid| { - let first_column = program.alloc_registers(5); + let sqlite_schema_column_len = sqlite_schema.columns.len(); + assert_eq!(sqlite_schema_column_len, 5); - for i in 0..5 { + let first_column = program.alloc_registers(sqlite_schema_column_len); + + for i in 0..sqlite_schema_column_len { program.emit_column(cursor_id, i, first_column + i); } @@ -256,7 +259,7 @@ pub fn translate_alter_table( program.emit_string8_new_reg(rename_to.clone()); program.mark_last_insn_constant(); - let out = program.alloc_registers(5); + let out = program.alloc_registers(sqlite_schema_column_len); program.emit_insn(Insn::Function { constant_mask: 0, @@ -272,7 +275,7 @@ pub fn translate_alter_table( program.emit_insn(Insn::MakeRecord { start_reg: out, - count: 5, + count: sqlite_schema_column_len, dest_reg: record, index_name: None, }); @@ -319,9 +322,12 @@ pub fn translate_alter_table( }); program.cursor_loop(cursor_id, |program, rowid| { - let first_column = program.alloc_registers(5); + let sqlite_schema_column_len = sqlite_schema.columns.len(); + assert_eq!(sqlite_schema_column_len, 5); - for i in 0..5 { + let first_column = program.alloc_registers(sqlite_schema_column_len); + + for i in 0..sqlite_schema_column_len { program.emit_column(cursor_id, i, first_column + i); } @@ -347,7 +353,7 @@ pub fn translate_alter_table( program.emit_insn(Insn::MakeRecord { start_reg: out, - count: 5, + count: sqlite_schema_column_len, dest_reg: record, index_name: None, }); From 43db84e6ea1bc80cb2a269e52357fa9804a952d3 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Tue, 10 Jun 2025 16:33:29 -0300 Subject: [PATCH 32/33] fix: broken rebase --- core/vdbe/execute.rs | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index 04813efeb..f936c94ec 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -1334,17 +1334,18 @@ pub fn op_column( must_be_btree_cursor!(*cursor_id, program.cursor_ref, state, "Column"); let cursor = cursor.as_btree_mut(); let record = return_if_io!(cursor.record()); - let value = if let Some(record) = record.as_ref() { - if cursor.get_null_flag() { - RefValue::Null - } else { - match record.get_value_opt(*column) { - Some(val) => val.clone(), - None => RefValue::Null, - } - } + + let Some(record) = record.as_ref() else { + break 'value Value::Null; + }; + + let value = if cursor.get_null_flag() { + Value::Null } else { - RefValue::Null + match record.get_value_opt(*column) { + Some(val) => val.to_owned(), + None => Value::Null, + } }; if cursor.get_null_flag() { From 5d60d82499da29e3c7641f8b66b1bdf126f80829 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Tue, 10 Jun 2025 16:37:12 -0300 Subject: [PATCH 33/33] fix: add `default` --- core/translate/optimizer/join.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/core/translate/optimizer/join.rs b/core/translate/optimizer/join.rs index 98fb92da0..11ebbd109 100644 --- a/core/translate/optimizer/join.rs +++ b/core/translate/optimizer/join.rs @@ -1283,12 +1283,14 @@ mod tests { order: SortOrder::Asc, pos_in_table: 0, collation: None, + default: None, }, IndexColumn { name: "y".to_string(), order: SortOrder::Asc, pos_in_table: 1, collation: None, + default: None, }, ], unique: false, @@ -1367,18 +1369,21 @@ mod tests { order: SortOrder::Asc, pos_in_table: 0, collation: None, + default: None, }, IndexColumn { name: "c2".to_string(), order: SortOrder::Asc, pos_in_table: 1, collation: None, + default: None, }, IndexColumn { name: "c3".to_string(), order: SortOrder::Asc, pos_in_table: 2, collation: None, + default: None, }, ], unique: false, @@ -1480,18 +1485,21 @@ mod tests { order: SortOrder::Asc, pos_in_table: 0, collation: None, + default: None, }, IndexColumn { name: "c2".to_string(), order: SortOrder::Asc, pos_in_table: 1, collation: None, + default: None, }, IndexColumn { name: "c3".to_string(), order: SortOrder::Asc, pos_in_table: 2, collation: None, + default: None, }, ], root_page: 2,