Merge 'btree: fix incorrect comparison implementation in key_exists_in_index()' from Jussi Saurio

1. current implementation did not use the custom PartialOrd
implementation for RefValue
2. current implementation did not take collation into account
I don't have a test for this in this PR but it fixes an issue related to
#1757
EDIT:
Okay, it appears the first commit cannot be merged by itself because
since it fixes the incorrect comparison logic, now our UNION tests fail
due to UNIQUE constraint violation (since we are now correctly detecting
duplicates). I have introduced a workaround for this in https://github.c
om/tursodatabase/turso/pull/2001/commits/cb8a576501702cf91713c7f76520501
77318c49c
So, in effect, this PR:
Closes #1757
But, I will make better fixes in #1988 later.

Reviewed-by: Pere Diaz Bou <pere-altea@homail.com>

Closes #2001
This commit is contained in:
Jussi Saurio
2025-07-08 16:03:34 +03:00
4 changed files with 28 additions and 9 deletions

View File

@@ -4637,8 +4637,13 @@ impl BTreeCursor {
let record_opt = return_if_io!(self.record());
match record_opt.as_ref() {
Some(record) => {
// Existing record found — compare prefix
let existing_key = &record.get_values()[..record.count().saturating_sub(1)];
// Existing record found; if the index has a rowid, exclude it from the comparison since it's a pointer to the table row;
// UNIQUE indexes disallow duplicates like (a=1,b=2,rowid=1) and (a=1,b=2,rowid=2).
let existing_key = if self.has_rowid() {
&record.get_values()[..record.count().saturating_sub(1)]
} else {
record.get_values()
};
let inserted_key_vals = &key.get_values();
// Need this check because .all returns True on an empty iterator,
// So when record_opt is invalidated, it would always indicate show up as a duplicate key
@@ -4647,10 +4652,12 @@ impl BTreeCursor {
}
Ok(CursorResult::Ok(
existing_key
.iter()
.zip(inserted_key_vals.iter())
.all(|(a, b)| a == b),
compare_immutable(
existing_key,
inserted_key_vals,
self.key_sort_order(),
&self.collations,
) == std::cmp::Ordering::Equal,
))
}
None => {

View File

@@ -98,7 +98,7 @@ pub fn emit_result_row_and_limit(
record_reg,
unpacked_start: None,
unpacked_count: None,
flags: IdxInsertFlags::new(),
flags: IdxInsertFlags::new().no_op_duplicate(),
});
}
QueryDestination::EphemeralTable {

View File

@@ -4450,7 +4450,7 @@ pub fn op_idx_insert(
let CursorType::BTreeIndex(index_meta) = cursor_type else {
panic!("IdxInsert: not a BTree index cursor");
};
{
'block: {
let mut cursor = state.get_cursor(cursor_id);
let cursor = cursor.as_btree_mut();
let record = match &state.registers[record_reg] {
@@ -4470,9 +4470,12 @@ pub fn op_idx_insert(
// check for uniqueness violation
match cursor.key_exists_in_index(record)? {
CursorResult::Ok(true) => {
if flags.has(IdxInsertFlags::NO_OP_DUPLICATE) {
break 'block;
}
return Err(LimboError::Constraint(
"UNIQUE constraint failed: duplicate key".into(),
))
));
}
CursorResult::IO => return Ok(InsnFunctionStepResult::IO),
CursorResult::Ok(false) => {}

View File

@@ -63,6 +63,7 @@ impl IdxInsertFlags {
pub const APPEND: u8 = 0x01; // Hint: insert likely at the end
pub const NCHANGE: u8 = 0x02; // Increment the change counter
pub const USE_SEEK: u8 = 0x04; // Skip seek if last one was same key
pub const NO_OP_DUPLICATE: u8 = 0x08; // Do not error on duplicate key
pub fn new() -> Self {
IdxInsertFlags(0)
}
@@ -93,6 +94,14 @@ impl IdxInsertFlags {
}
self
}
/// If this is set, we will not error on duplicate key.
/// This is a bit of a hack we use to make ephemeral indexes for UNION work --
/// instead we should allow overwriting index interior cells, which we currently don't;
/// this should (and will) be fixed in a future PR.
pub fn no_op_duplicate(mut self) -> Self {
self.0 |= IdxInsertFlags::NO_OP_DUPLICATE;
self
}
}
#[derive(Clone, Copy, Debug, Default)]