Adjustments and explicitely just emitting NoConflict on unique indexes

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

core/schema.rs

@@ -81,6 +81,14 @@ impl Schema {
             .map_or_else(|| &[] as &[Arc<Index>], |v| v.as_slice())
     }
 
+    pub fn get_index(&self, table_name: &str, index_name: &str) -> Option<&Arc<Index>> {
+        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);

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 {

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

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

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

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