From 51541dd8dc93ba2085988e572403c62220a13d78 Mon Sep 17 00:00:00 2001 From: jussisaurio Date: Sat, 21 Dec 2024 14:46:38 +0200 Subject: [PATCH 1/5] fix issues with insert --- core/translate/insert.rs | 315 ++++++++++++++++++++++++++++----------- 1 file changed, 232 insertions(+), 83 deletions(-) diff --git a/core/translate/insert.rs b/core/translate/insert.rs index 614cde8b2..89665a726 100644 --- a/core/translate/insert.rs +++ b/core/translate/insert.rs @@ -2,10 +2,11 @@ use std::rc::Weak; use std::{cell::RefCell, ops::Deref, rc::Rc}; use sqlite3_parser::ast::{ - DistinctNames, InsertBody, QualifiedName, ResolveType, ResultColumn, With, + DistinctNames, Expr, InsertBody, QualifiedName, ResolveType, ResultColumn, With, }; use crate::error::SQLITE_CONSTRAINT_PRIMARYKEY; +use crate::util::normalize_ident; use crate::{ schema::{Schema, Table}, storage::sqlite3_ondisk::DatabaseHeader, @@ -14,13 +15,117 @@ use crate::{ }; use crate::{Connection, Result}; +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +/// Helper enum to indicate how a column is being inserted. +/// For example: +/// CREATE TABLE t (a, b, c, d); +/// INSERT INTO t (c, b) VALUES (1, 2); +/// +/// resolve_columns_for_insert() returns [ +/// ColumnToInsert::AutomaticNull, +/// ColumnToInsert::UserProvided { index_in_value_tuple: 1 }, +/// ColumnToInsert::UserProvided { index_in_value_tuple: 0 }, +/// ColumnToInsert::AutomaticNull, +/// ] +enum ColumnToInsert { + /// The column is provided by the user. + UserProvided { index_in_value_tuple: usize }, + /// The column is automatically set to NULL since it was not provided by the user. + AutomaticNull, +} + +/// Resolves how each column in a table should be populated during an INSERT. +/// For each column, determines whether it will: +/// 1. Use a user-provided value from the VALUES clause, or +/// 2. Be automatically set to NULL +/// +/// Two cases are handled: +/// 1. No column list specified in INSERT statement: +/// - Values are assigned to columns in table definition order +/// - If fewer values than columns, remaining columns are NULL +/// 2. Column list specified in INSERT statement: +/// - For specified columns, a ColumnToInsert::UserProvided entry is created. +/// - Any columns not listed are set to NULL, i.e. a ColumnToInsert::AutomaticNull entry is created. +/// +/// Returns a Vec with an entry for each column in the table, +/// indicating how that column should be populated. +fn resolve_columns_for_insert( + table: Rc, + columns: &Option, + values: &[Vec], +) -> Result> { + assert!(table.has_rowid()); + if values.is_empty() { + crate::bail_parse_error!("no values to insert"); + } + + let num_cols_in_table = table.columns().len(); + + if columns.is_none() { + let num_cols = values[0].len(); + // ensure value tuples dont have more columns than the table + if num_cols > num_cols_in_table { + crate::bail_parse_error!( + "table {} has {} columns but {} values were supplied", + table.get_name(), + num_cols_in_table, + num_cols + ); + } + // ensure each value tuple has the same number of columns + for value in values.iter().skip(1) { + if value.len() != num_cols { + crate::bail_parse_error!("all VALUES must have the same number of terms"); + } + } + let columns: Vec = (0..num_cols_in_table) + .map(|i| { + if i < num_cols { + ColumnToInsert::UserProvided { + index_in_value_tuple: i, + } + } else { + ColumnToInsert::AutomaticNull + } + }) + .collect(); + return Ok(columns); + } + + // resolve the given columns to actual table column names and ensure they exist + let columns = columns.as_ref().unwrap(); + let mut resolved_columns: Vec = (0..num_cols_in_table) + .map(|i| ColumnToInsert::AutomaticNull) + .collect(); + for (index_in_value_tuple, column) in columns.iter().enumerate() { + let column_name = normalize_ident(column.0.as_str()); + let column_idx = table + .columns() + .iter() + .position(|c| c.name.eq_ignore_ascii_case(&column_name)); + if let Some(i) = column_idx { + resolved_columns[i] = ColumnToInsert::UserProvided { + index_in_value_tuple, + }; + } else { + crate::bail_parse_error!( + "table {} has no column named {}", + table.get_name(), + column_name + ); + } + } + + Ok(resolved_columns) +} + #[allow(clippy::too_many_arguments)] pub fn translate_insert( schema: &Schema, with: &Option, or_conflict: &Option, tbl_name: &QualifiedName, - _columns: &Option, + columns: &Option, body: &InsertBody, _returning: &Option>, database_header: Rc>, @@ -46,6 +151,10 @@ pub fn translate_insert( None => crate::bail_corrupt_error!("Parse error: no such table: {}", table_name), }; let table = Rc::new(Table::BTree(table)); + if !table.has_rowid() { + crate::bail_parse_error!("INSERT into WITHOUT ROWID table is not supported"); + } + let cursor_id = program.alloc_cursor_id( Some(table_name.0.clone()), Some(table.clone().deref().clone()), @@ -55,18 +164,44 @@ pub fn translate_insert( Table::Index(index) => index.root_page, Table::Pseudo(_) => todo!(), }; + let values = match body { + InsertBody::Select(select, None) => match &select.body.select { + sqlite3_parser::ast::OneSelect::Values(values) => values, + _ => todo!(), + }, + _ => todo!(), + }; - let mut num_cols = table.columns().len(); - if table.has_rowid() { - num_cols += 1; - } - // column_registers_start[0] == rowid if has rowid - let column_registers_start = program.alloc_registers(num_cols); + let columns = resolve_columns_for_insert(table.clone(), columns, values)?; + // Check if rowid was provided (through INTEGER PRIMARY KEY as a rowid alias) + let rowid_alias_index = table.columns().iter().position(|c| c.is_rowid_alias); + let has_user_provided_rowid = { + assert!(columns.len() == table.columns().len()); + if let Some(index) = rowid_alias_index { + matches!(columns[index], ColumnToInsert::UserProvided { .. }) + } else { + false + } + }; + + // allocate a register for each column in the table. if not provided by user, they will simply be set as null. + // allocate an extra register for rowid regardless of whether user provided a rowid alias column. + let num_cols = table.columns().len(); + let rowid_reg = program.alloc_registers(num_cols + 1); + let column_registers_start = rowid_reg + 1; + let rowid_alias_reg = { + if has_user_provided_rowid { + Some(column_registers_start + rowid_alias_index.unwrap()) + } else { + None + } + }; - // Coroutine for values let yield_reg = program.alloc_register(); let jump_on_definition_label = program.allocate_label(); { + // Coroutine for values + // TODO/efficiency: only use coroutine when there are multiple values to insert program.emit_insn_with_label_dependency( Insn::InitCoroutine { yield_reg, @@ -75,40 +210,41 @@ pub fn translate_insert( }, jump_on_definition_label, ); - match body { - InsertBody::Select(select, None) => match &select.body.select { - sqlite3_parser::ast::OneSelect::Select { - distinctness: _, - columns: _, - from: _, - where_clause: _, - group_by: _, - window_clause: _, - } => todo!(), - sqlite3_parser::ast::OneSelect::Values(values) => { - for value in values { - for (col, expr) in value.iter().enumerate() { - let mut col = col; - if table.has_rowid() { - col += 1; - } - translate_expr( - &mut program, - None, - expr, - column_registers_start + col, - None, - )?; - } - program.emit_insn(Insn::Yield { - yield_reg, - end_offset: 0, + + for value in values { + // Process each value according to resolved columns + for (i, column) in columns.iter().enumerate() { + match column { + ColumnToInsert::UserProvided { + index_in_value_tuple, + } => { + translate_expr( + &mut program, + None, + value.get(*index_in_value_tuple).expect( + format!( + "values tuple has no value for column {}", + table.column_index_to_name(i).unwrap() + ) + .as_str(), + ), + column_registers_start + i, + None, + )?; + } + ColumnToInsert::AutomaticNull => { + program.emit_insn(Insn::Null { + dest: column_registers_start + i, + dest_end: None, }); + program.mark_last_insn_constant(); } } - }, - InsertBody::DefaultValues => todo!("default values not yet supported"), - _ => todo!(), + } + program.emit_insn(Insn::Yield { + yield_reg, + end_offset: 0, + }); } program.emit_insn(Insn::EndCoroutine { yield_reg }); } @@ -121,6 +257,8 @@ pub fn translate_insert( program.emit_insn(Insn::OpenWriteAwait {}); // Main loop + // FIXME: rollback is not implemented. E.g. if you insert 2 rows and one fails to unique constraint violation, + // the other row will still be inserted. let record_register = program.alloc_register(); let halt_label = program.allocate_label(); let loop_start_offset = program.offset(); @@ -132,68 +270,79 @@ pub fn translate_insert( halt_label, ); - if table.has_rowid() { - let row_id_reg = column_registers_start; - if let Some(rowid_alias_column) = table.get_rowid_alias_column() { - let key_reg = column_registers_start + 1 + rowid_alias_column.0; - // copy key to rowid - program.emit_insn(Insn::Copy { - src_reg: key_reg, - dst_reg: row_id_reg, - amount: 0, - }); - program.emit_insn(Insn::SoftNull { reg: key_reg }); - } - - let notnull_label = program.allocate_label(); + let check_rowid_is_integer_label = rowid_alias_reg.and(Some(program.allocate_label())); + if let Some(reg) = rowid_alias_reg { + program.emit_insn(Insn::Copy { + src_reg: reg, + dst_reg: rowid_reg, + amount: 0, // TODO: rename 'amount' to something else; amount==0 means 1 + }); + // for the row record, the rowid alias column is always set to NULL + program.emit_insn(Insn::SoftNull { reg }); + // the user provided rowid value might itself be NULL. If it is, we create a new rowid on the next instruction. program.emit_insn_with_label_dependency( Insn::NotNull { - reg: row_id_reg, - target_pc: notnull_label, + reg: rowid_reg, + target_pc: check_rowid_is_integer_label.unwrap(), }, - notnull_label, + check_rowid_is_integer_label.unwrap(), ); - program.emit_insn(Insn::NewRowid { - cursor: cursor_id, - rowid_reg: row_id_reg, - prev_largest_reg: 0, - }); + } - program.resolve_label(notnull_label, program.offset()); - program.emit_insn(Insn::MustBeInt { reg: row_id_reg }); + // Create new rowid if a) not provided by user or b) provided by user but is NULL + program.emit_insn(Insn::NewRowid { + cursor: cursor_id, + rowid_reg: rowid_reg, + prev_largest_reg: 0, + }); + + if let Some(must_be_int_label) = check_rowid_is_integer_label { + program.resolve_label(must_be_int_label, program.offset()); + // If the user provided a rowid, it must be an integer. + program.emit_insn(Insn::MustBeInt { reg: rowid_reg }); + } + + // Check uniqueness constraint for rowid if it was provided by user. + // When the DB allocates it there are no need for separate uniqueness checks. + if has_user_provided_rowid { let make_record_label = program.allocate_label(); program.emit_insn_with_label_dependency( Insn::NotExists { cursor: cursor_id, - rowid_reg: row_id_reg, + rowid_reg: rowid_reg, target_pc: make_record_label, }, make_record_label, ); - // TODO: rollback + let rowid_column_name = if let Some(index) = rowid_alias_index { + table.column_index_to_name(index).unwrap() + } else { + "rowid" + }; + program.emit_insn(Insn::Halt { err_code: SQLITE_CONSTRAINT_PRIMARYKEY, - description: format!( - "{}.{}", - table.get_name(), - table.column_index_to_name(0).unwrap() - ), + description: format!("{}.{}", table.get_name(), rowid_column_name), }); + program.resolve_label(make_record_label, program.offset()); - program.emit_insn(Insn::MakeRecord { - start_reg: column_registers_start + 1, - count: num_cols - 1, - dest_reg: record_register, - }); - program.emit_insn(Insn::InsertAsync { - cursor: cursor_id, - key_reg: column_registers_start, - record_reg: record_register, - flag: 0, - }); - program.emit_insn(Insn::InsertAwait { cursor_id }); } + // Create and insert the record + program.emit_insn(Insn::MakeRecord { + start_reg: column_registers_start, + count: num_cols, + dest_reg: record_register, + }); + + program.emit_insn(Insn::InsertAsync { + cursor: cursor_id, + key_reg: rowid_reg, + record_reg: record_register, + flag: 0, + }); + program.emit_insn(Insn::InsertAwait { cursor_id }); + program.emit_insn(Insn::Goto { target_pc: loop_start_offset, }); From fa5ca68eec2f327434520c215833c3aad68f68b6 Mon Sep 17 00:00:00 2001 From: jussisaurio Date: Sat, 21 Dec 2024 15:12:53 +0200 Subject: [PATCH 2/5] Add multi-row insert to simulator --- simulator/generation/plan.rs | 4 ++-- simulator/generation/query.rs | 13 +++++++++---- simulator/model/query.rs | 17 ++++++++++++----- 3 files changed, 23 insertions(+), 11 deletions(-) diff --git a/simulator/generation/plan.rs b/simulator/generation/plan.rs index 61b115f01..82c75c4e3 100644 --- a/simulator/generation/plan.rs +++ b/simulator/generation/plan.rs @@ -106,7 +106,7 @@ impl Interactions { .iter_mut() .find(|t| t.name == insert.table) .unwrap(); - table.rows.push(insert.values.clone()); + table.rows.extend(insert.values.clone()); } Query::Delete(_) => todo!(), Query::Select(_) => {} @@ -320,7 +320,7 @@ fn property_insert_select(rng: &mut R, env: &SimulatorEnv) -> Inte // Insert the row let insert_query = Interaction::Query(Query::Insert(Insert { table: table.name.clone(), - values: row.clone(), + values: vec![row.clone()], })); // Select the row diff --git a/simulator/generation/query.rs b/simulator/generation/query.rs index ca6926650..0ff9d44e1 100644 --- a/simulator/generation/query.rs +++ b/simulator/generation/query.rs @@ -37,10 +37,15 @@ impl ArbitraryFrom> for Select { impl ArbitraryFrom
for Insert { fn arbitrary_from(rng: &mut R, table: &Table) -> Self { - let values = table - .columns - .iter() - .map(|c| Value::arbitrary_from(rng, &c.column_type)) + let num_rows = rng.gen_range(1..10); + let values: Vec> = (0..num_rows) + .map(|_| { + table + .columns + .iter() + .map(|c| Value::arbitrary_from(rng, &c.column_type)) + .collect() + }) .collect(); Insert { table: table.name.clone(), diff --git a/simulator/model/query.rs b/simulator/model/query.rs index eeec68d08..7a12def8d 100644 --- a/simulator/model/query.rs +++ b/simulator/model/query.rs @@ -75,7 +75,7 @@ pub(crate) struct Select { #[derive(Clone, Debug, PartialEq)] pub(crate) struct Insert { pub(crate) table: String, - pub(crate) values: Vec, + pub(crate) values: Vec>, } #[derive(Clone, Debug, PartialEq)] @@ -104,14 +104,21 @@ impl Display for Query { predicate: guard, }) => write!(f, "SELECT * FROM {} WHERE {}", table, guard), Query::Insert(Insert { table, values }) => { - write!(f, "INSERT INTO {} VALUES (", table)?; - for (i, v) in values.iter().enumerate() { + write!(f, "INSERT INTO {} VALUES ", table)?; + for (i, row) in values.iter().enumerate() { if i != 0 { write!(f, ", ")?; } - write!(f, "{}", v)?; + write!(f, "(")?; + for (j, value) in row.iter().enumerate() { + if j != 0 { + write!(f, ", ")?; + } + write!(f, "{}", value)?; + } + write!(f, ")")?; } - write!(f, ")") + Ok(()) } Query::Delete(Delete { table, From c78a3e952a73854002a0b72b3a550ccc6064d73c Mon Sep 17 00:00:00 2001 From: jussisaurio Date: Sat, 21 Dec 2024 19:33:16 +0200 Subject: [PATCH 3/5] clean up implementation --- core/translate/insert.rs | 173 +++++++++++++++++++-------------------- 1 file changed, 83 insertions(+), 90 deletions(-) diff --git a/core/translate/insert.rs b/core/translate/insert.rs index 89665a726..aee6e0ec4 100644 --- a/core/translate/insert.rs +++ b/core/translate/insert.rs @@ -8,115 +8,109 @@ use sqlite3_parser::ast::{ use crate::error::SQLITE_CONSTRAINT_PRIMARYKEY; use crate::util::normalize_ident; use crate::{ - schema::{Schema, Table}, + schema::{Column, Schema, Table}, storage::sqlite3_ondisk::DatabaseHeader, translate::expr::translate_expr, vdbe::{builder::ProgramBuilder, Insn, Program}, }; use crate::{Connection, Result}; -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -/// Helper enum to indicate how a column is being inserted. -/// For example: -/// CREATE TABLE t (a, b, c, d); -/// INSERT INTO t (c, b) VALUES (1, 2); -/// -/// resolve_columns_for_insert() returns [ -/// ColumnToInsert::AutomaticNull, -/// ColumnToInsert::UserProvided { index_in_value_tuple: 1 }, -/// ColumnToInsert::UserProvided { index_in_value_tuple: 0 }, -/// ColumnToInsert::AutomaticNull, -/// ] -enum ColumnToInsert { - /// The column is provided by the user. - UserProvided { index_in_value_tuple: usize }, - /// The column is automatically set to NULL since it was not provided by the user. - AutomaticNull, +#[derive(Debug)] +/// Represents how a column should be populated during an INSERT. +/// Contains both the column definition and optionally the index into the VALUES tuple. +struct ColumnMapping<'a> { + /// Reference to the column definition from the table schema + column: &'a Column, + /// If Some(i), use the i-th value from the VALUES tuple + /// If None, use NULL (column was not specified in INSERT statement) + value_index: Option, } /// Resolves how each column in a table should be populated during an INSERT. -/// For each column, determines whether it will: -/// 1. Use a user-provided value from the VALUES clause, or -/// 2. Be automatically set to NULL +/// Returns a Vec of ColumnMapping, one for each column in the table's schema. +/// +/// For each column, specifies: +/// 1. The column definition (type, constraints, etc) +/// 2. Where to get the value from: +/// - Some(i) -> use i-th value from the VALUES tuple +/// - None -> use NULL (column wasn't specified in INSERT) /// /// Two cases are handled: -/// 1. No column list specified in INSERT statement: +/// 1. No column list specified (INSERT INTO t VALUES ...): /// - Values are assigned to columns in table definition order -/// - If fewer values than columns, remaining columns are NULL -/// 2. Column list specified in INSERT statement: -/// - For specified columns, a ColumnToInsert::UserProvided entry is created. -/// - Any columns not listed are set to NULL, i.e. a ColumnToInsert::AutomaticNull entry is created. -/// -/// Returns a Vec with an entry for each column in the table, -/// indicating how that column should be populated. -fn resolve_columns_for_insert( - table: Rc
, +/// - If fewer values than columns, remaining columns map to None +/// 2. Column list specified (INSERT INTO t (col1, col3) VALUES ...): +/// - Named columns map to their corresponding value index +/// - Unspecified columns map to None +fn resolve_columns_for_insert<'a>( + table: &'a Table, columns: &Option, values: &[Vec], -) -> Result> { - assert!(table.has_rowid()); +) -> Result>> { if values.is_empty() { crate::bail_parse_error!("no values to insert"); } - let num_cols_in_table = table.columns().len(); + let table_columns = table.columns(); + // Case 1: No columns specified - map values to columns in order if columns.is_none() { - let num_cols = values[0].len(); - // ensure value tuples dont have more columns than the table - if num_cols > num_cols_in_table { + let num_values = values[0].len(); + if num_values > table_columns.len() { crate::bail_parse_error!( "table {} has {} columns but {} values were supplied", table.get_name(), - num_cols_in_table, - num_cols + table_columns.len(), + num_values ); } - // ensure each value tuple has the same number of columns + + // Verify all value tuples have same length for value in values.iter().skip(1) { - if value.len() != num_cols { + if value.len() != num_values { crate::bail_parse_error!("all VALUES must have the same number of terms"); } } - let columns: Vec = (0..num_cols_in_table) - .map(|i| { - if i < num_cols { - ColumnToInsert::UserProvided { - index_in_value_tuple: i, - } - } else { - ColumnToInsert::AutomaticNull - } + + // Map each column to either its corresponding value index or None + return Ok(table_columns + .iter() + .enumerate() + .map(|(i, col)| ColumnMapping { + column: col, + value_index: if i < num_values { Some(i) } else { None }, }) - .collect(); - return Ok(columns); + .collect()); } - // resolve the given columns to actual table column names and ensure they exist - let columns = columns.as_ref().unwrap(); - let mut resolved_columns: Vec = (0..num_cols_in_table) - .map(|i| ColumnToInsert::AutomaticNull) + // Case 2: Columns specified - map named columns to their values + let mut mappings: Vec<_> = table_columns + .iter() + .map(|col| ColumnMapping { + column: col, + value_index: None, + }) .collect(); - for (index_in_value_tuple, column) in columns.iter().enumerate() { - let column_name = normalize_ident(column.0.as_str()); - let column_idx = table - .columns() + + // Map each named column to its value index + for (value_index, column_name) in columns.as_ref().unwrap().iter().enumerate() { + let column_name = normalize_ident(column_name.0.as_str()); + let table_index = table_columns .iter() .position(|c| c.name.eq_ignore_ascii_case(&column_name)); - if let Some(i) = column_idx { - resolved_columns[i] = ColumnToInsert::UserProvided { - index_in_value_tuple, - }; - } else { + + if table_index.is_none() { crate::bail_parse_error!( "table {} has no column named {}", table.get_name(), column_name ); } + + mappings[table_index.unwrap()].value_index = Some(value_index); } - Ok(resolved_columns) + Ok(mappings) } #[allow(clippy::too_many_arguments)] @@ -172,13 +166,13 @@ pub fn translate_insert( _ => todo!(), }; - let columns = resolve_columns_for_insert(table.clone(), columns, values)?; + let column_mappings = resolve_columns_for_insert(&table, columns, values)?; // Check if rowid was provided (through INTEGER PRIMARY KEY as a rowid alias) let rowid_alias_index = table.columns().iter().position(|c| c.is_rowid_alias); let has_user_provided_rowid = { - assert!(columns.len() == table.columns().len()); + assert!(column_mappings.len() == table.columns().len()); if let Some(index) = rowid_alias_index { - matches!(columns[index], ColumnToInsert::UserProvided { .. }) + column_mappings[index].value_index.is_some() } else { false } @@ -213,31 +207,30 @@ pub fn translate_insert( for value in values { // Process each value according to resolved columns - for (i, column) in columns.iter().enumerate() { - match column { - ColumnToInsert::UserProvided { - index_in_value_tuple, - } => { - translate_expr( - &mut program, - None, - value.get(*index_in_value_tuple).expect( - format!( - "values tuple has no value for column {}", - table.column_index_to_name(i).unwrap() - ) - .as_str(), - ), - column_registers_start + i, - None, - )?; - } - ColumnToInsert::AutomaticNull => { + for (i, mapping) in column_mappings.iter().enumerate() { + let target_reg = column_registers_start + i; + + if let Some(value_index) = mapping.value_index { + // Column has a value in the VALUES tuple + translate_expr( + &mut program, + None, + value.get(value_index).expect("value index out of bounds"), + target_reg, + None, + )?; + } else { + // Column was not specified - use NULL if it is nullable, otherwise error. + // Rowid alias columns can be NULL because we will autogenerate a rowid in that case. + let is_nullable = !mapping.column.primary_key || mapping.column.is_rowid_alias; + if is_nullable { program.emit_insn(Insn::Null { - dest: column_registers_start + i, + dest: target_reg, dest_end: None, }); program.mark_last_insn_constant(); + } else { + crate::bail_parse_error!("column {} is not nullable", mapping.column.name); } } } From 050b8744eaabe0e7914624869ff7c2c508e8a5a0 Mon Sep 17 00:00:00 2001 From: jussisaurio Date: Sat, 21 Dec 2024 23:21:48 +0200 Subject: [PATCH 4/5] Dont use coroutine when inserting a single row --- core/translate/insert.rs | 192 ++++++++++++++++++++++++++------------- 1 file changed, 128 insertions(+), 64 deletions(-) diff --git a/core/translate/insert.rs b/core/translate/insert.rs index aee6e0ec4..bd9caae4c 100644 --- a/core/translate/insert.rs +++ b/core/translate/insert.rs @@ -113,6 +113,58 @@ fn resolve_columns_for_insert<'a>( Ok(mappings) } +/// Populates the column registers with values for a single row +fn populate_column_registers( + program: &mut ProgramBuilder, + value: &[Expr], + column_mappings: &[ColumnMapping], + column_registers_start: usize, + inserting_multiple_rows: bool, + rowid_reg: usize, +) -> Result<()> { + for (i, mapping) in column_mappings.iter().enumerate() { + let target_reg = column_registers_start + i; + + // Column has a value in the VALUES tuple + if let Some(value_index) = mapping.value_index { + // When inserting a single row, SQLite writes the value provided for the rowid alias column (INTEGER PRIMARY KEY) + // directly into the rowid register and writes a NULL into the rowid alias column. Not sure why this only happens + // in the single row case, but let's copy it. + let write_directly_to_rowid_reg = + mapping.column.is_rowid_alias && !inserting_multiple_rows; + let reg = if write_directly_to_rowid_reg { + rowid_reg + } else { + target_reg + }; + translate_expr( + program, + None, + value.get(value_index).expect("value index out of bounds"), + reg, + None, + )?; + if write_directly_to_rowid_reg { + program.emit_insn(Insn::SoftNull { reg: target_reg }); + } + } else { + // Column was not specified - use NULL if it is nullable, otherwise error + // Rowid alias columns can be NULL because we will autogenerate a rowid in that case. + let is_nullable = !mapping.column.primary_key || mapping.column.is_rowid_alias; + if is_nullable { + program.emit_insn(Insn::Null { + dest: target_reg, + dest_end: None, + }); + program.mark_last_insn_constant(); + } else { + crate::bail_parse_error!("column {} is not nullable", mapping.column.name); + } + } + } + Ok(()) +} + #[allow(clippy::too_many_arguments)] pub fn translate_insert( schema: &Schema, @@ -191,11 +243,16 @@ pub fn translate_insert( } }; - let yield_reg = program.alloc_register(); - let jump_on_definition_label = program.allocate_label(); - { - // Coroutine for values - // TODO/efficiency: only use coroutine when there are multiple values to insert + let record_register = program.alloc_register(); + let halt_label = program.allocate_label(); + let mut loop_start_offset = 0; + + let inserting_multiple_rows = values.len() > 1; + + // Multiple rows - use coroutine for value population + if inserting_multiple_rows { + let yield_reg = program.alloc_register(); + let jump_on_definition_label = program.allocate_label(); program.emit_insn_with_label_dependency( Insn::InitCoroutine { yield_reg, @@ -206,72 +263,75 @@ pub fn translate_insert( ); for value in values { - // Process each value according to resolved columns - for (i, mapping) in column_mappings.iter().enumerate() { - let target_reg = column_registers_start + i; - - if let Some(value_index) = mapping.value_index { - // Column has a value in the VALUES tuple - translate_expr( - &mut program, - None, - value.get(value_index).expect("value index out of bounds"), - target_reg, - None, - )?; - } else { - // Column was not specified - use NULL if it is nullable, otherwise error. - // Rowid alias columns can be NULL because we will autogenerate a rowid in that case. - let is_nullable = !mapping.column.primary_key || mapping.column.is_rowid_alias; - if is_nullable { - program.emit_insn(Insn::Null { - dest: target_reg, - dest_end: None, - }); - program.mark_last_insn_constant(); - } else { - crate::bail_parse_error!("column {} is not nullable", mapping.column.name); - } - } - } + populate_column_registers( + &mut program, + value, + &column_mappings, + column_registers_start, + true, + rowid_reg, + )?; program.emit_insn(Insn::Yield { yield_reg, end_offset: 0, }); } program.emit_insn(Insn::EndCoroutine { yield_reg }); + program.resolve_label(jump_on_definition_label, program.offset()); + + program.emit_insn(Insn::OpenWriteAsync { + cursor_id, + root_page, + }); + program.emit_insn(Insn::OpenWriteAwait {}); + + // Main loop + // FIXME: rollback is not implemented. E.g. if you insert 2 rows and one fails to unique constraint violation, + // the other row will still be inserted. + loop_start_offset = program.offset(); + program.emit_insn_with_label_dependency( + Insn::Yield { + yield_reg, + end_offset: halt_label, + }, + halt_label, + ); + } else { + // Single row - populate registers directly + program.emit_insn(Insn::OpenWriteAsync { + cursor_id, + root_page, + }); + program.emit_insn(Insn::OpenWriteAwait {}); + + populate_column_registers( + &mut program, + &values[0], + &column_mappings, + column_registers_start, + false, + rowid_reg, + )?; } - program.resolve_label(jump_on_definition_label, program.offset()); - program.emit_insn(Insn::OpenWriteAsync { - cursor_id, - root_page, - }); - program.emit_insn(Insn::OpenWriteAwait {}); - - // Main loop - // FIXME: rollback is not implemented. E.g. if you insert 2 rows and one fails to unique constraint violation, - // the other row will still be inserted. - let record_register = program.alloc_register(); - let halt_label = program.allocate_label(); - let loop_start_offset = program.offset(); - program.emit_insn_with_label_dependency( - Insn::Yield { - yield_reg, - end_offset: halt_label, - }, - halt_label, - ); - + // Common record insertion logic for both single and multiple rows let check_rowid_is_integer_label = rowid_alias_reg.and(Some(program.allocate_label())); if let Some(reg) = rowid_alias_reg { - program.emit_insn(Insn::Copy { - src_reg: reg, - dst_reg: rowid_reg, - amount: 0, // TODO: rename 'amount' to something else; amount==0 means 1 - }); - // for the row record, the rowid alias column is always set to NULL - program.emit_insn(Insn::SoftNull { reg }); + // for the row record, the rowid alias column (INTEGER PRIMARY KEY) is always set to NULL + // and its value is copied to the rowid register. in the case where a single row is inserted, + // the value is written directly to the rowid register (see populate_column_registers()). + // again, not sure why this only happens in the single row case, but let's mimic sqlite. + // in the single row case we save a Copy instruction, but in the multiple rows case we do + // it here in the loop. + if inserting_multiple_rows { + program.emit_insn(Insn::Copy { + src_reg: reg, + dst_reg: rowid_reg, + amount: 0, // TODO: rename 'amount' to something else; amount==0 means 1 + }); + // for the row record, the rowid alias column is always set to NULL + program.emit_insn(Insn::SoftNull { reg }); + } // the user provided rowid value might itself be NULL. If it is, we create a new rowid on the next instruction. program.emit_insn_with_label_dependency( Insn::NotNull { @@ -336,15 +396,19 @@ pub fn translate_insert( }); program.emit_insn(Insn::InsertAwait { cursor_id }); - program.emit_insn(Insn::Goto { - target_pc: loop_start_offset, - }); + if inserting_multiple_rows { + // For multiple rows, loop back + program.emit_insn(Insn::Goto { + target_pc: loop_start_offset, + }); + } program.resolve_label(halt_label, program.offset()); program.emit_insn(Insn::Halt { err_code: 0, description: String::new(), }); + program.resolve_label(init_label, program.offset()); program.emit_insn(Insn::Transaction { write: true }); program.emit_constant_insns(); From c4e2a344ae2462008b20900ab002d2b719b0a871 Mon Sep 17 00:00:00 2001 From: jussisaurio Date: Sat, 21 Dec 2024 23:44:41 +0200 Subject: [PATCH 5/5] parse error instead of assert! for unsupported features --- core/translate/insert.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/core/translate/insert.rs b/core/translate/insert.rs index bd9caae4c..12c9ed016 100644 --- a/core/translate/insert.rs +++ b/core/translate/insert.rs @@ -169,7 +169,7 @@ fn populate_column_registers( pub fn translate_insert( schema: &Schema, with: &Option, - or_conflict: &Option, + on_conflict: &Option, tbl_name: &QualifiedName, columns: &Option, body: &InsertBody, @@ -177,8 +177,12 @@ pub fn translate_insert( database_header: Rc>, connection: Weak, ) -> Result { - assert!(with.is_none()); - assert!(or_conflict.is_none()); + if with.is_some() { + crate::bail_parse_error!("WITH clause is not supported"); + } + if on_conflict.is_some() { + crate::bail_parse_error!("ON CONFLICT clause is not supported"); + } let mut program = ProgramBuilder::new(); let init_label = program.allocate_label(); program.emit_insn_with_label_dependency(