diff --git a/Makefile b/Makefile index 06afa0e5d..db3c3acdb 100644 --- a/Makefile +++ b/Makefile @@ -66,7 +66,7 @@ uv-sync: uv sync --all-packages .PHONE: uv-sync -test: limbo uv-sync test-compat test-vector test-sqlite3 test-shell test-extensions test-memory test-write test-update +test: limbo uv-sync test-compat test-vector test-sqlite3 test-shell test-extensions test-memory test-write test-update test-constraint .PHONY: test test-extensions: limbo uv-sync @@ -109,6 +109,10 @@ test-update: limbo uv-sync SQLITE_EXEC=$(SQLITE_EXEC) uv run --project limbo_test test-update .PHONY: test-update +test-constraint: limbo uv-sync + SQLITE_EXEC=$(SQLITE_EXEC) uv run --project limbo_test test-constraint +.PHONY: test-constraint + bench-vfs: uv-sync cargo build --release uv run --project limbo_test bench-vfs "$(SQL)" "$(N)" diff --git a/core/schema.rs b/core/schema.rs index dd09671ab..42c619693 100644 --- a/core/schema.rs +++ b/core/schema.rs @@ -81,6 +81,14 @@ impl Schema { .map_or_else(|| &[] as &[Arc], |v| v.as_slice()) } + pub fn get_index(&self, table_name: &str, index_name: &str) -> Option<&Arc> { + let name = normalize_ident(table_name); + self.indexes + .get(&name)? + .iter() + .find(|index| index.name == index_name) + } + pub fn remove_indices_for_table(&mut self, table_name: &str) { let name = normalize_ident(table_name); self.indexes.remove(&name); diff --git a/core/translate/insert.rs b/core/translate/insert.rs index 235cc09ac..b17d19110 100644 --- a/core/translate/insert.rs +++ b/core/translate/insert.rs @@ -320,42 +320,48 @@ pub fn translate_insert( dest_reg: record_reg, }); - let make_record_label = program.allocate_label(); - program.emit_insn(Insn::NoConflict { - cursor_id: idx_cursor_id, - target_pc: make_record_label, - record_reg: idx_start_reg, - num_regs: num_cols, - }); - let mut column_names = Vec::new(); - for (index, ..) in index_col_mapping.columns.iter() { - let name = btree_table - .columns - .get(*index) - .unwrap() - .name - .as_ref() - .expect("column name is None"); - column_names.push(format!("{}.{name}", btree_table.name)); - } - let column_names = - column_names - .into_iter() - .enumerate() - .fold(String::new(), |mut accum, (idx, name)| { - if idx % 2 == 1 { + let index = schema + .get_index(&table_name.0, &index_col_mapping.idx_name) + .expect("index should be present"); + + if index.unique { + let label_idx_insert = program.allocate_label(); + program.emit_insn(Insn::NoConflict { + cursor_id: idx_cursor_id, + target_pc: label_idx_insert, + record_reg: idx_start_reg, + num_regs: num_cols, + }); + let column_names = index_col_mapping.columns.iter().enumerate().fold( + String::with_capacity(50), + |mut accum, (idx, (index, _))| { + if idx > 0 { accum.push_str(", "); } - accum.push_str(&name); + + accum.push_str(&btree_table.name); + accum.push('.'); + + let name = btree_table + .columns + .get(*index) + .unwrap() + .name + .as_ref() + .expect("column name is None"); + accum.push_str(name); + accum - }); + }, + ); - program.emit_insn(Insn::Halt { - err_code: SQLITE_CONSTRAINT_PRIMARYKEY, - description: column_names, - }); + program.emit_insn(Insn::Halt { + err_code: SQLITE_CONSTRAINT_PRIMARYKEY, + description: column_names, + }); - program.resolve_label(make_record_label, program.offset()); + program.resolve_label(label_idx_insert, program.offset()); + } // now do the actual index insertion using the unpacked registers program.emit_insn(Insn::IdxInsert { diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index 31a81e9b9..7b6f23f5e 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -3,7 +3,6 @@ use crate::numeric::{NullableInteger, Numeric}; use crate::storage::database::FileMemoryStorage; use crate::storage::page_cache::DumbLruPageCache; use crate::storage::pager::CreateBTreeFlags; -use crate::types::ImmutableRecord; use crate::{ error::{LimboError, SQLITE_CONSTRAINT, SQLITE_CONSTRAINT_PRIMARYKEY}, ext::ExtValue, @@ -3912,48 +3911,41 @@ pub fn op_no_conflict( else { unreachable!("unexpected Insn {:?}", insn) }; - let found = { - let mut cursor = state.get_cursor(*cursor_id); - let cursor = cursor.as_btree_mut(); + let mut cursor_ref = state.get_cursor(*cursor_id); + let cursor = cursor_ref.as_btree_mut(); - let any_fn = |record: &ImmutableRecord| { - for val in record.values.iter() { - if matches!(val, RefValue::Null) { - return false; - } + let record = if *num_regs == 0 { + let record = match &state.registers[*record_reg] { + Register::Record(r) => r, + _ => { + return Err(LimboError::InternalError( + "NoConflict: exepected a record in the register".into(), + )); } - true }; - - let record = if *num_regs == 0 { - let record = match &state.registers[*record_reg] { - Register::Record(r) => r, - _ => { - return Err(LimboError::InternalError( - "NoConflict: exepected a record in the register".into(), - )); - } - }; - record - } else { - &make_record(&state.registers, record_reg, num_regs) - }; - - // Should early return and jump if any of the values in the record is NULL - let found = any_fn(record); - if found { - return_if_io!(cursor.seek(SeekKey::IndexKey(record), SeekOp::EQ)) - } else { - found - } - }; - - if found { - state.pc += 1; + record } else { + &make_record(&state.registers, record_reg, num_regs) + }; + // If there is at least one NULL in the index record, there cannot be a conflict so we can immediately jump. + let contains_nulls = record + .get_values() + .iter() + .any(|val| matches!(val, RefValue::Null)); + + if contains_nulls { + drop(cursor_ref); state.pc = target_pc.to_offset_int(); + return Ok(InsnFunctionStepResult::Step); } + let conflict = return_if_io!(cursor.seek(SeekKey::IndexKey(record), SeekOp::EQ)); + drop(cursor_ref); + if !conflict { + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } Ok(InsnFunctionStepResult::Step) } diff --git a/core/vdbe/insn.rs b/core/vdbe/insn.rs index 633647c36..6f310f746 100644 --- a/core/vdbe/insn.rs +++ b/core/vdbe/insn.rs @@ -664,6 +664,11 @@ pub enum Insn { reg: usize, }, + /// If P4==0 then register P3 holds a blob constructed by [MakeRecord](https://sqlite.org/opcode.html#MakeRecord). If P4>0 then register P3 is the first of P4 registers that form an unpacked record.\ + /// + /// Cursor P1 is on an index btree. If the record identified by P3 and P4 contains any NULL value, jump immediately to P2. If all terms of the record are not-NULL then a check is done to determine if any row in the P1 index btree has a matching key prefix. If there are no matches, jump immediately to P2. If there is a match, fall through and leave the P1 cursor pointing to the matching row.\ + /// + /// This opcode is similar to [NotFound](https://sqlite.org/opcode.html#NotFound) with the exceptions that the branch is always taken if any part of the search key input is NULL. NoConflict { cursor_id: CursorID, // P1 index cursor target_pc: BranchOffset, // P2 jump target diff --git a/testing/cli_tests/constraint.py b/testing/cli_tests/constraint.py index a37a5b020..65758745b 100644 --- a/testing/cli_tests/constraint.py +++ b/testing/cli_tests/constraint.py @@ -1,4 +1,6 @@ #!/usr/bin/env python3 + +# Eventually extract these tests to be in the fuzzing integration tests import os from faker import Faker from faker.providers.lorem.en_US import Provider as P diff --git a/testing/pyproject.toml b/testing/pyproject.toml index cdd30ec54..0aed7b99b 100644 --- a/testing/pyproject.toml +++ b/testing/pyproject.toml @@ -16,6 +16,7 @@ test-extensions = "cli_tests.extensions:main" test-update = "cli_tests.update:main" test-memory = "cli_tests.memory:main" bench-vfs = "cli_tests.vfs_bench:main" +test-constraint = "cli_tests.constraint:main" [tool.uv] package = true