From 2171c5f4e1a602b5a8f75317e16beee87f40e1c0 Mon Sep 17 00:00:00 2001 From: krishvishal Date: Mon, 2 Jun 2025 10:38:11 +0530 Subject: [PATCH 01/20] Added apply_affinity_char to fix bugs in `SeekRowId` --- core/vdbe/execute.rs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index 1e56778b7..72077b551 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -2004,13 +2004,22 @@ pub fn op_seek_rowid( let rowid = match state.registers[*src_reg].get_owned_value() { Value::Integer(rowid) => Some(*rowid), Value::Null => None, + // For non-integer values try to apply affinity and convert them to integer. other => { - return Err(LimboError::InternalError(format!( - "SeekRowid: the value in the register is not an integer or NULL: {}", - other - ))); + let mut temp_reg = Register::Value(other.clone()); + let converted = apply_affinity_char(&mut temp_reg, Affinity::Numeric); + if converted { + match temp_reg.get_owned_value() { + Value::Integer(i) => Some(*i), + Value::Float(f) => Some(*f as i64), + _ => unreachable!("apply_affinity_char with Numeric should produce an integer if it returns true"), + } + } else { + None + } } }; + match rowid { Some(rowid) => { let found = return_if_io!( From 20e6e73057cac1954dae7610b10b3817e1bca121 Mon Sep 17 00:00:00 2001 From: krishvishal Date: Mon, 2 Jun 2025 10:45:29 +0530 Subject: [PATCH 02/20] Fix affinity in `op_seek` when there's string integer comparison --- core/vdbe/execute.rs | 207 ++++++++++++++++++++++++++++++++----------- 1 file changed, 157 insertions(+), 50 deletions(-) diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index 72077b551..7c235c316 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -2134,34 +2134,70 @@ pub fn op_seek( } } else { let pc = { + let original_value = state.registers[*start_reg].get_owned_value().clone(); + let mut temp_value = original_value.clone(); + + if matches!(temp_value, Value::Text(_)) { + let mut temp_reg = Register::Value(temp_value); + apply_affinity_char(&mut temp_reg, Affinity::Numeric); + temp_value = temp_reg.get_owned_value().clone(); + } + + let int_key = extract_int_value(&temp_value); + let lost_precision = !matches!(temp_value, Value::Integer(_)); + + let actual_op = if lost_precision { + match (&temp_value, op) { + (Value::Float(f), op) => { + let int_key_as_float = int_key as f64; + if int_key_as_float > *f { + match op { + SeekOp::GT => SeekOp::GE, + SeekOp::LE => SeekOp::LE, + other => other, + } + } else if int_key_as_float < *f { + match op { + SeekOp::GE => SeekOp::GT, + SeekOp::LT => SeekOp::LE, + other => other, + } + } else { + op + } + } + _ => op, + } + } else { + op + }; + + let rowid = if matches!(original_value, Value::Null) { + match actual_op { + SeekOp::GE | SeekOp::GT => { + state.pc = target_pc.to_offset_int(); + return Ok(InsnFunctionStepResult::Step); + } + SeekOp::LE | SeekOp::LT => { + state.pc = target_pc.to_offset_int(); + + return Ok(InsnFunctionStepResult::Step); + } + _ => unreachable!(), + } + } else { + int_key + }; let mut cursor = state.get_cursor(*cursor_id); let cursor = cursor.as_btree_mut(); - let rowid = match state.registers[*start_reg].get_owned_value() { - Value::Null => { - // All integer values are greater than null so we just rewind the cursor - return_if_io!(cursor.rewind()); - None - } - Value::Integer(rowid) => Some(*rowid), - _ => { - return Err(LimboError::InternalError(format!( - "{}: the value in the register is not an integer", - op_name - ))); - } - }; - let found = match rowid { - Some(rowid) => { - let found = return_if_io!(cursor.seek(SeekKey::TableRowId(rowid), op)); - if !found { - target_pc.to_offset_int() - } else { - state.pc + 1 - } - } - None => state.pc + 1, - }; - found + + let found = return_if_io!(cursor.seek(SeekKey::TableRowId(rowid), actual_op)); + + if !found { + target_pc.to_offset_int() + } else { + state.pc + 1 + } }; state.pc = pc; } @@ -5960,8 +5996,10 @@ fn apply_affinity_char(target: &mut Register, affinity: Affinity) -> bool { if matches!(value, Value::Blob(_)) { return true; } + match affinity { Affinity::Blob => return true, + Affinity::Text => { if matches!(value, Value::Text(_) | Value::Null) { return true; @@ -5970,6 +6008,7 @@ fn apply_affinity_char(target: &mut Register, affinity: Affinity) -> bool { *value = Value::Text(text.into()); return true; } + Affinity::Integer | Affinity::Numeric => { if matches!(value, Value::Integer(_)) { return true; @@ -5979,49 +6018,88 @@ fn apply_affinity_char(target: &mut Register, affinity: Affinity) -> bool { } if let Value::Float(fl) = *value { - if let Ok(int) = cast_real_to_integer(fl).map(Value::Integer) { - *value = int; - return true; - } - return false; + // For floats, try to convert to integer if it's exact + // This is similar to sqlite3VdbeIntegerAffinity + return try_float_to_integer_affinity(value, fl); } - let text = value.to_text().unwrap(); - let Ok(num) = checked_cast_text_to_numeric(&text) else { - return false; - }; + if let Value::Text(t) = value { + let text = t.as_str(); - *value = match &num { - Value::Float(fl) => { - cast_real_to_integer(*fl).map(Value::Integer).unwrap_or(num); - return true; - } - Value::Integer(_) if text.starts_with("0x") => { + // Handle hex numbers - they shouldn't be converted + if text.starts_with("0x") { return false; } - _ => num, - }; + + // Try to parse as number (similar to applyNumericAffinity) + let Ok(num) = checked_cast_text_to_numeric(text) else { + return false; + }; + + match num { + Value::Integer(i) => { + *value = Value::Integer(i); + return true; + } + Value::Float(fl) => { + // For Numeric affinity, try to convert float to int if exact + if affinity == Affinity::Numeric { + return try_float_to_integer_affinity(value, fl); + } else { + *value = Value::Float(fl); + return true; + } + } + other => { + *value = other; + return true; + } + } + } + + return false; } Affinity::Real => { - if let Value::Integer(i) = value { - *value = Value::Float(*i as f64); + if let Value::Integer(i) = *value { + *value = Value::Float(i as f64); return true; - } else if let Value::Text(t) = value { - if t.as_str().starts_with("0x") { + } + if let Value::Text(t) = value { + let s = t.as_str(); + if s.starts_with("0x") { return false; } - if let Ok(num) = checked_cast_text_to_numeric(t.as_str()) { + if let Ok(num) = checked_cast_text_to_numeric(s) { *value = num; return true; } else { return false; } } + return true; } - }; + } } - return true; + + true +} + +fn try_float_to_integer_affinity(value: &mut Value, fl: f64) -> bool { + // Check if the float can be exactly represented as an integer + if let Ok(int_val) = cast_real_to_integer(fl) { + // Additional check: ensure round-trip conversion is exact + // and value is within safe bounds (similar to SQLite's checks) + if (int_val as f64) == fl && int_val > i64::MIN + 1 && int_val < i64::MAX - 1 { + *value = Value::Integer(int_val); + return true; + } + } + + // If we can't convert to exact integer, keep as float for Numeric affinity + // but return false to indicate the conversion wasn't "complete" + *value = Value::Float(fl); + false } fn execute_sqlite_version(version_integer: i64) -> String { @@ -6032,6 +6110,35 @@ fn execute_sqlite_version(version_integer: i64) -> String { format!("{}.{}.{}", major, minor, release) } +pub fn extract_int_value(value: &Value) -> i64 { + match value { + Value::Integer(i) => *i, + Value::Float(f) => { + // Use sqlite3RealToI64 equivalent + if *f < -9223372036854774784.0 { + i64::MIN + } else if *f > 9223372036854774784.0 { + i64::MAX + } else { + *f as i64 + } + } + Value::Text(t) => { + // Try to parse as integer, return 0 if failed + t.as_str().parse::().unwrap_or(0) + } + Value::Blob(b) => { + // Try to parse blob as string then as integer + if let Ok(s) = std::str::from_utf8(b) { + s.parse::().unwrap_or(0) + } else { + 0 + } + } + Value::Null => 0, + } +} + #[cfg(test)] mod tests { use crate::types::{Text, Value}; From 9eb2235135799fd6dce9d8712ac88a18914c4b3e Mon Sep 17 00:00:00 2001 From: krishvishal Date: Mon, 2 Jun 2025 10:52:12 +0530 Subject: [PATCH 03/20] A simple fuzz test for fuzzing seeking ops --- tests/integration/fuzz/mod.rs | 106 ++++++++++++++++++++++++++++------ 1 file changed, 89 insertions(+), 17 deletions(-) diff --git a/tests/integration/fuzz/mod.rs b/tests/integration/fuzz/mod.rs index 9f5437dad..709e48990 100644 --- a/tests/integration/fuzz/mod.rs +++ b/tests/integration/fuzz/mod.rs @@ -69,28 +69,100 @@ mod tests { Some("ORDER BY x ASC"), ]; - for comp in COMPARISONS.iter() { - for order_by in ORDER_BY.iter() { - for max in 0..=2000 { - let query = format!( - "SELECT * FROM t WHERE x {} {} {}", - comp, - max, - order_by.unwrap_or("") - ); - log::trace!("query: {}", query); - let limbo = limbo_exec_rows(&db, &limbo_conn, &query); - let sqlite = sqlite_exec_rows(&sqlite_conn, &query); - assert_eq!( - limbo, sqlite, - "query: {}, limbo: {:?}, sqlite: {:?}", - query, limbo, sqlite - ); + let (mut rng, seed) = rng_from_time_or_env(); + tracing::info!("rowid_seek_fuzz seed: {}", seed); + + for iteration in 0..100 { + tracing::trace!("rowid_seek_fuzz iteration: {}", iteration); + + for comp in COMPARISONS.iter() { + for order_by in ORDER_BY.iter() { + let test_values = generate_random_comparison_values(&mut rng); + + for test_value in test_values.iter() { + let query = format!( + "SELECT * FROM t WHERE x {} {} {}", + comp, + test_value, + order_by.unwrap_or("") + ); + + log::trace!("query: {}", query); + + let limbo_result = limbo_exec_rows(&db, &limbo_conn, &query); + let sqlite_result = sqlite_exec_rows(&sqlite_conn, &query); + assert_eq!( + limbo_result, sqlite_result, + "query: {}, limbo: {:?}, sqlite: {:?}, seed: {}", + query, limbo_result, sqlite_result, seed + ); + } } } } } + fn generate_random_comparison_values(rng: &mut ChaCha8Rng) -> Vec { + let mut values = Vec::new(); + + for _ in 0..5 { + let val = rng.random_range(-10000..10000); + values.push(val.to_string()); + } + + values.push(i64::MAX.to_string()); + values.push(i64::MIN.to_string()); + values.push("0".to_string()); + + for _ in 0..5 { + let val: f64 = rng.random_range(-10000.0..10000.0); + values.push(val.to_string()); + } + + values.push("0.0".to_string()); + values.push("-0.0".to_string()); + values.push("1.5".to_string()); + values.push("-1.5".to_string()); + values.push("999.999".to_string()); + + values.push("'text'".to_string()); + values.push("'123'".to_string()); + values.push("''".to_string()); + values.push("'0'".to_string()); + values.push("'hello'".to_string()); + + values.push("'0x10'".to_string()); + values.push("'+123'".to_string()); + values.push("' 123 '".to_string()); + values.push("'1.5e2'".to_string()); + values.push("'inf'".to_string()); + values.push("'-inf'".to_string()); + values.push("'nan'".to_string()); + + values.push("X'41'".to_string()); + values.push("X''".to_string()); + + values.push("(1 + 1)".to_string()); + values.push("(SELECT 1)".to_string()); + + values + } + + fn rng_from_time_or_env() -> (ChaCha8Rng, u64) { + let seed = std::env::var("SEED").map_or( + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_secs(), + |v| { + v.parse() + .expect("Failed to parse SEED environment variable as u64") + }, + ); + let rng = ChaCha8Rng::seed_from_u64(seed); + (rng, seed) + } + #[test] pub fn index_scan_fuzz() { let db = TempDatabase::new_with_rusqlite("CREATE TABLE t(x PRIMARY KEY)"); From 30ccbe46c7b709704dbc2e63cf40de2cedc6b485 Mon Sep 17 00:00:00 2001 From: krishvishal Date: Mon, 2 Jun 2025 11:19:41 +0530 Subject: [PATCH 04/20] Added `apply_numeric_affinity` function to handle string conversion to integer. Exising functions' behavior is tailored to `CAST` ops. SQLite has different behavior when it comes to handling string to `integer` conversion in CAST vs predicate ops. --- core/vdbe/execute.rs | 358 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 358 insertions(+) diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index 7c235c316..067dd3ace 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -6139,10 +6139,368 @@ pub fn extract_int_value(value: &Value) -> i64 { } } +#[derive(Debug, PartialEq)] +enum NumericParseResult { + NotNumeric, // not a valid number + PureInteger, // pure integer (entire string) + HasDecimalOrExp, // has decimal point or exponent (entire string) + ValidPrefixOnly, // valid prefix but not entire string +} + +#[derive(Debug)] +enum ParsedNumber { + None, + Integer(i64), + Float(f64), +} + +impl ParsedNumber { + fn as_integer(&self) -> Option { + match self { + ParsedNumber::Integer(i) => Some(*i), + _ => None, + } + } + + fn as_float(&self) -> Option { + match self { + ParsedNumber::Float(f) => Some(*f), + _ => None, + } + } +} + +fn try_for_float(text: &str) -> (NumericParseResult, ParsedNumber) { + let bytes = text.as_bytes(); + if bytes.is_empty() { + return (NumericParseResult::NotNumeric, ParsedNumber::None); + } + + let mut pos = 0; + let len = bytes.len(); + + while pos < len && is_space(bytes[pos]) { + pos += 1; + } + + if pos >= len { + return (NumericParseResult::NotNumeric, ParsedNumber::None); + } + + let start_pos = pos; + + let mut sign = 1i64; + + if bytes[pos] == b'-' { + sign = -1; + pos += 1; + } else if bytes[pos] == b'+' { + pos += 1; + } + + if pos >= len { + return (NumericParseResult::NotNumeric, ParsedNumber::None); + } + + let mut significand = 0u64; + let mut digit_count = 0; + let mut decimal_adjust = 0i32; + let mut has_digits = false; + + // Parse digits before decimal point + while pos < len && bytes[pos].is_ascii_digit() { + has_digits = true; + let digit = (bytes[pos] - b'0') as u64; + + if significand <= (u64::MAX - 9) / 10 { + significand = significand * 10 + digit; + digit_count += 1; + } else { + // Skip overflow digits but adjust exponent + decimal_adjust += 1; + } + pos += 1; + } + + let mut has_decimal = false; + let mut has_exponent = false; + + // Check for decimal point + if pos < len && bytes[pos] == b'.' { + has_decimal = true; + pos += 1; + + // Parse fractional digits + while pos < len && bytes[pos].is_ascii_digit() { + has_digits = true; + let digit = (bytes[pos] - b'0') as u64; + + if significand <= (u64::MAX - 9) / 10 { + significand = significand * 10 + digit; + digit_count += 1; + decimal_adjust -= 1; + } + pos += 1; + } + } + + if !has_digits { + return (NumericParseResult::NotNumeric, ParsedNumber::None); + } + + // Check for exponent + let mut exponent = 0i32; + if pos < len && (bytes[pos] == b'e' || bytes[pos] == b'E') { + has_exponent = true; + pos += 1; + + if pos >= len { + // Incomplete exponent, but we have valid digits before + return create_result_from_significand( + significand, + sign, + decimal_adjust, + has_decimal, + has_exponent, + NumericParseResult::ValidPrefixOnly, + ); + } + + let mut exp_sign = 1i32; + if bytes[pos] == b'-' { + exp_sign = -1; + pos += 1; + } else if bytes[pos] == b'+' { + pos += 1; + } + + if pos >= len || !bytes[pos].is_ascii_digit() { + // Incomplete exponent + return create_result_from_significand( + significand, + sign, + decimal_adjust, + has_decimal, + false, + NumericParseResult::ValidPrefixOnly, + ); + } + + // Parse exponent digits + while pos < len && bytes[pos].is_ascii_digit() { + let digit = (bytes[pos] - b'0') as i32; + if exponent < 10000 { + exponent = exponent * 10 + digit; + } else { + exponent = 10000; // Cap at large value + } + pos += 1; + } + exponent *= exp_sign; + } + + // Skip trailing whitespace + while pos < len && is_space(bytes[pos]) { + pos += 1; + } + + // Determine if we consumed the entire string + let consumed_all = pos >= len; + let final_exponent = decimal_adjust + exponent; + + let parse_result = if !consumed_all { + NumericParseResult::ValidPrefixOnly + } else if has_decimal || has_exponent { + NumericParseResult::HasDecimalOrExp + } else { + NumericParseResult::PureInteger + }; + + create_result_from_significand( + significand, + sign, + final_exponent, + has_decimal, + has_exponent, + parse_result, + ) +} + +fn create_result_from_significand( + significand: u64, + sign: i64, + exponent: i32, + has_decimal: bool, + has_exponent: bool, + parse_result: NumericParseResult, +) -> (NumericParseResult, ParsedNumber) { + if significand == 0 { + match parse_result { + NumericParseResult::PureInteger => { + return (parse_result, ParsedNumber::Integer(0)); + } + _ => { + return (parse_result, ParsedNumber::Float(0.0)); + } + } + } + + // For pure integers without exponent, try to return as integer + if !has_decimal && !has_exponent && exponent == 0 { + let signed_val = (significand as i64).wrapping_mul(sign); + if (significand as i64) * sign == signed_val { + return (parse_result, ParsedNumber::Integer(signed_val)); + } + } + + // Convert to float + let mut result = significand as f64; + + let mut exp = exponent; + if exp > 0 { + while exp >= 100 { + result *= 1e100; + exp -= 100; + } + while exp >= 10 { + result *= 1e10; + exp -= 10; + } + while exp >= 1 { + result *= 10.0; + exp -= 1; + } + } else if exp < 0 { + while exp <= -100 { + result *= 1e-100; + exp += 100; + } + while exp <= -10 { + result *= 1e-10; + exp += 10; + } + while exp <= -1 { + result *= 0.1; + exp += 1; + } + } + + if sign < 0 { + result = -result; + } + + (parse_result, ParsedNumber::Float(result)) +} + +pub fn is_space(byte: u8) -> bool { + matches!(byte, b' ' | b'\t' | b'\n' | b'\r' | b'\x0c') +} + +fn real_to_i64(r: f64) -> i64 { + if r < -9223372036854774784.0 { + i64::MIN + } else if r > 9223372036854774784.0 { + i64::MAX + } else { + r as i64 + } +} + +fn apply_integer_affinity(register: &mut Register) -> bool { + let Register::Value(Value::Float(f)) = register else { + return false; + }; + + let ix = real_to_i64(*f); + + // Only convert if round-trip is exact and not at extreme values + if *f == (ix as f64) && ix > i64::MIN && ix < i64::MAX { + *register = Register::Value(Value::Integer(ix)); + true + } else { + false + } +} + +/// Try to convert a value into a numeric representation if we can +/// do so without loss of information. In other words, if the string +/// looks like a number, convert it into a number. If it does not +/// look like a number, leave it alone. +pub fn apply_numeric_affinity(register: &mut Register, try_for_int: bool) -> bool { + let Register::Value(Value::Text(text)) = register else { + return false; // Only apply to text values + }; + + let text_str = text.as_str(); + let (parse_result, parsed_value) = try_for_float(text_str); + + // Only convert if we have a complete valid number (not just a prefix) + match parse_result { + NumericParseResult::NotNumeric | NumericParseResult::ValidPrefixOnly => { + false // Leave as text + } + NumericParseResult::PureInteger => { + if let Some(int_val) = parsed_value.as_integer() { + *register = Register::Value(Value::Integer(int_val)); + true + } else { + false + } + } + NumericParseResult::HasDecimalOrExp => { + if let Some(float_val) = parsed_value.as_float() { + *register = Register::Value(Value::Float(float_val)); + // If try_for_int is true, try to convert float to int if exact + if try_for_int { + apply_integer_affinity(register); + } + true + } else { + false + } + } + } +} + #[cfg(test)] mod tests { + use super::*; use crate::types::{Text, Value}; + #[test] + fn test_apply_numeric_affinity_partial_numbers() { + let mut reg = Register::Value(Value::Text(Text::from_str("123abc"))); + assert!(!apply_numeric_affinity(&mut reg, false)); + assert!(matches!(reg, Register::Value(Value::Text(_)))); + + let mut reg = Register::Value(Value::Text(Text::from_str("-53093015420544-15062897"))); + assert!(!apply_numeric_affinity(&mut reg, false)); + assert!(matches!(reg, Register::Value(Value::Text(_)))); + + let mut reg = Register::Value(Value::Text(Text::from_str("123.45xyz"))); + assert!(!apply_numeric_affinity(&mut reg, false)); + assert!(matches!(reg, Register::Value(Value::Text(_)))); + } + + #[test] + fn test_apply_numeric_affinity_complete_numbers() { + let mut reg = Register::Value(Value::Text(Text::from_str("123"))); + assert!(apply_numeric_affinity(&mut reg, false)); + assert_eq!(*reg.get_owned_value(), Value::Integer(123)); + + let mut reg = Register::Value(Value::Text(Text::from_str("123.45"))); + assert!(apply_numeric_affinity(&mut reg, false)); + assert_eq!(*reg.get_owned_value(), Value::Float(123.45)); + + let mut reg = Register::Value(Value::Text(Text::from_str(" -456 "))); + assert!(apply_numeric_affinity(&mut reg, false)); + assert_eq!(*reg.get_owned_value(), Value::Integer(-456)); + + let mut reg = Register::Value(Value::Text(Text::from_str("0"))); + assert!(apply_numeric_affinity(&mut reg, false)); + assert_eq!(*reg.get_owned_value(), Value::Integer(0)); + } + #[test] fn test_exec_add() { let inputs = vec![ From e68293a1d104a86da3a13462147e6c7db8766e67 Mon Sep 17 00:00:00 2001 From: krishvishal Date: Fri, 6 Jun 2025 13:14:07 +0530 Subject: [PATCH 05/20] Add affinity conversion to op_gt, op_le, op_lt, op_eq, op_ne --- core/vdbe/execute.rs | 443 ++++++++++++++++++++++++++++++------------- 1 file changed, 311 insertions(+), 132 deletions(-) diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index 067dd3ace..4c4e59c88 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -552,37 +552,64 @@ pub fn op_eq( let lhs = *lhs; let rhs = *rhs; let target_pc = *target_pc; - let cond = *state.registers[lhs].get_owned_value() == *state.registers[rhs].get_owned_value(); let nulleq = flags.has_nulleq(); let jump_if_null = flags.has_jump_if_null(); let collation = collation.unwrap_or_default(); - match ( - state.registers[lhs].get_owned_value(), - state.registers[rhs].get_owned_value(), - ) { - (_, Value::Null) | (Value::Null, _) => { - if (nulleq && cond) || (!nulleq && jump_if_null) { + + let lhs_value = state.registers[lhs].get_owned_value(); + let rhs_value = state.registers[rhs].get_owned_value(); + + // Fast path for integers + if matches!(lhs_value, Value::Integer(_)) && matches!(rhs_value, Value::Integer(_)) { + if lhs_value == rhs_value { state.pc = target_pc.to_offset_int(); } else { state.pc += 1; } - } - (Value::Text(lhs), Value::Text(rhs)) => { - let order = collation.compare_strings(lhs.as_str(), rhs.as_str()); - if order.is_eq() { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - } - (lhs, rhs) => { - if *lhs == *rhs { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - } + return Ok(InsnFunctionStepResult::Step); } + + // Handle NULL values + if matches!(lhs_value, Value::Null) || matches!(rhs_value, Value::Null) { + let cond = lhs_value == rhs_value; // Both NULL + if (nulleq && cond) || (!nulleq && jump_if_null) { + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } + return Ok(InsnFunctionStepResult::Step); + } + + let mut lhs_temp_reg = state.registers[lhs].clone(); + let mut rhs_temp_reg = state.registers[rhs].clone(); + + let lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); + let rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + + let should_jump = match ( + lhs_temp_reg.get_owned_value(), + rhs_temp_reg.get_owned_value(), + ) { + (Value::Text(lhs_text), Value::Text(rhs_text)) => { + let order = collation.compare_strings(lhs_text.as_str(), rhs_text.as_str()); + order.is_eq() + } + (lhs, rhs) => *lhs == *rhs, + }; + + if lhs_converted { + state.registers[lhs] = lhs_temp_reg; + } + if rhs_converted { + state.registers[rhs] = rhs_temp_reg; + } + + if should_jump { + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } + Ok(InsnFunctionStepResult::Step) } @@ -607,37 +634,64 @@ pub fn op_ne( let lhs = *lhs; let rhs = *rhs; let target_pc = *target_pc; - let cond = *state.registers[lhs].get_owned_value() != *state.registers[rhs].get_owned_value(); let nulleq = flags.has_nulleq(); let jump_if_null = flags.has_jump_if_null(); let collation = collation.unwrap_or_default(); - match ( - state.registers[lhs].get_owned_value(), - state.registers[rhs].get_owned_value(), - ) { - (_, Value::Null) | (Value::Null, _) => { - if (nulleq && cond) || (!nulleq && jump_if_null) { + + let lhs_value = state.registers[lhs].get_owned_value(); + let rhs_value = state.registers[rhs].get_owned_value(); + + // Fast path for integers + if matches!(lhs_value, Value::Integer(_)) && matches!(rhs_value, Value::Integer(_)) { + if lhs_value != rhs_value { state.pc = target_pc.to_offset_int(); } else { state.pc += 1; } - } - (Value::Text(lhs), Value::Text(rhs)) => { - let order = collation.compare_strings(lhs.as_str(), rhs.as_str()); - if order.is_ne() { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - } - (lhs, rhs) => { - if *lhs != *rhs { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - } + return Ok(InsnFunctionStepResult::Step); } + + // Handle NULL values + if matches!(lhs_value, Value::Null) || matches!(rhs_value, Value::Null) { + let cond = lhs_value != rhs_value; // At least one NULL + if (nulleq && cond) || (!nulleq && jump_if_null) { + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } + return Ok(InsnFunctionStepResult::Step); + } + + let mut lhs_temp_reg = state.registers[lhs].clone(); + let mut rhs_temp_reg = state.registers[rhs].clone(); + + let lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); + let rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + + let should_jump = match ( + lhs_temp_reg.get_owned_value(), + rhs_temp_reg.get_owned_value(), + ) { + (Value::Text(lhs_text), Value::Text(rhs_text)) => { + let order = collation.compare_strings(lhs_text.as_str(), rhs_text.as_str()); + order.is_ne() + } + (lhs, rhs) => *lhs != *rhs, + }; + + if lhs_converted { + state.registers[lhs] = lhs_temp_reg; + } + if rhs_converted { + state.registers[rhs] = rhs_temp_reg; + } + + if should_jump { + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } + Ok(InsnFunctionStepResult::Step) } @@ -664,33 +718,60 @@ pub fn op_lt( let target_pc = *target_pc; let jump_if_null = flags.has_jump_if_null(); let collation = collation.unwrap_or_default(); - match ( - state.registers[lhs].get_owned_value(), - state.registers[rhs].get_owned_value(), - ) { - (_, Value::Null) | (Value::Null, _) => { - if jump_if_null { + + let lhs_value = state.registers[lhs].get_owned_value(); + let rhs_value = state.registers[rhs].get_owned_value(); + + // Fast path for integers + if matches!(lhs_value, Value::Integer(_)) && matches!(rhs_value, Value::Integer(_)) { + if lhs_value < rhs_value { state.pc = target_pc.to_offset_int(); } else { state.pc += 1; } - } - (Value::Text(lhs), Value::Text(rhs)) => { - let order = collation.compare_strings(lhs.as_str(), rhs.as_str()); - if order.is_lt() { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - } - (lhs, rhs) => { - if *lhs < *rhs { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - } + return Ok(InsnFunctionStepResult::Step); } + + // Handle NULL values + if matches!(lhs_value, Value::Null) || matches!(rhs_value, Value::Null) { + if jump_if_null { + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } + return Ok(InsnFunctionStepResult::Step); + } + + let mut lhs_temp_reg = state.registers[lhs].clone(); + let mut rhs_temp_reg = state.registers[rhs].clone(); + + let lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); + let rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + + let should_jump = match ( + lhs_temp_reg.get_owned_value(), + rhs_temp_reg.get_owned_value(), + ) { + (Value::Text(lhs_text), Value::Text(rhs_text)) => { + let order = collation.compare_strings(lhs_text.as_str(), rhs_text.as_str()); + order.is_lt() + } + (lhs, rhs) => *lhs < *rhs, + }; + + if lhs_converted { + state.registers[lhs] = lhs_temp_reg; + } + if rhs_converted { + state.registers[rhs] = rhs_temp_reg; + } + + if should_jump { + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } + Ok(InsnFunctionStepResult::Step) } @@ -717,33 +798,60 @@ pub fn op_le( let target_pc = *target_pc; let jump_if_null = flags.has_jump_if_null(); let collation = collation.unwrap_or_default(); - match ( - state.registers[lhs].get_owned_value(), - state.registers[rhs].get_owned_value(), - ) { - (_, Value::Null) | (Value::Null, _) => { - if jump_if_null { + + let lhs_value = state.registers[lhs].get_owned_value(); + let rhs_value = state.registers[rhs].get_owned_value(); + + // Fast path for integers + if matches!(lhs_value, Value::Integer(_)) && matches!(rhs_value, Value::Integer(_)) { + if lhs_value <= rhs_value { state.pc = target_pc.to_offset_int(); } else { state.pc += 1; } - } - (Value::Text(lhs), Value::Text(rhs)) => { - let order = collation.compare_strings(lhs.as_str(), rhs.as_str()); - if order.is_le() { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - } - (lhs, rhs) => { - if *lhs <= *rhs { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - } + return Ok(InsnFunctionStepResult::Step); } + + // Handle NULL values + if matches!(lhs_value, Value::Null) || matches!(rhs_value, Value::Null) { + if jump_if_null { + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } + return Ok(InsnFunctionStepResult::Step); + } + + let mut lhs_temp_reg = state.registers[lhs].clone(); + let mut rhs_temp_reg = state.registers[rhs].clone(); + + let lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); + let rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + + let should_jump = match ( + lhs_temp_reg.get_owned_value(), + rhs_temp_reg.get_owned_value(), + ) { + (Value::Text(lhs_text), Value::Text(rhs_text)) => { + let order = collation.compare_strings(lhs_text.as_str(), rhs_text.as_str()); + order.is_le() + } + (lhs, rhs) => *lhs <= *rhs, + }; + + if lhs_converted { + state.registers[lhs] = lhs_temp_reg; + } + if rhs_converted { + state.registers[rhs] = rhs_temp_reg; + } + + if should_jump { + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } + Ok(InsnFunctionStepResult::Step) } @@ -770,33 +878,59 @@ pub fn op_gt( let target_pc = *target_pc; let jump_if_null = flags.has_jump_if_null(); let collation = collation.unwrap_or_default(); - match ( - state.registers[lhs].get_owned_value(), - state.registers[rhs].get_owned_value(), - ) { - (_, Value::Null) | (Value::Null, _) => { - if jump_if_null { + + let lhs_value = state.registers[lhs].get_owned_value(); + let rhs_value = state.registers[rhs].get_owned_value(); + + // Fast path for integers + if matches!(lhs_value, Value::Integer(_)) && matches!(rhs_value, Value::Integer(_)) { + if lhs_value > rhs_value { state.pc = target_pc.to_offset_int(); } else { state.pc += 1; } - } - (Value::Text(lhs), Value::Text(rhs)) => { - let order = collation.compare_strings(lhs.as_str(), rhs.as_str()); - if order.is_gt() { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - } - (lhs, rhs) => { - if *lhs > *rhs { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - } + return Ok(InsnFunctionStepResult::Step); } + + // Handle NULL values + if matches!(lhs_value, Value::Null) || matches!(rhs_value, Value::Null) { + if jump_if_null { + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } + return Ok(InsnFunctionStepResult::Step); + } + + let mut lhs_temp_reg = state.registers[lhs].clone(); + let mut rhs_temp_reg = state.registers[rhs].clone(); + + let lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); + let rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + + let should_jump = match ( + lhs_temp_reg.get_owned_value(), + rhs_temp_reg.get_owned_value(), + ) { + (Value::Text(lhs_text), Value::Text(rhs_text)) => { + let order = collation.compare_strings(lhs_text.as_str(), rhs_text.as_str()); + order.is_gt() + } + (lhs, rhs) => *lhs > *rhs, + }; + + if lhs_converted { + state.registers[lhs] = lhs_temp_reg; + } + if rhs_converted { + state.registers[rhs] = rhs_temp_reg; + } + + if should_jump { + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } Ok(InsnFunctionStepResult::Step) } @@ -815,41 +949,86 @@ pub fn op_ge( collation, } = insn else { - unreachable!("unexpected Insn {:?}", insn) + unreachable!("unexpected Insn: {:?}", insn) }; + assert!(target_pc.is_offset()); + let lhs = *lhs; let rhs = *rhs; let target_pc = *target_pc; let jump_if_null = flags.has_jump_if_null(); let collation = collation.unwrap_or_default(); - match ( - state.registers[lhs].get_owned_value(), - state.registers[rhs].get_owned_value(), + + let lhs_value = state.registers[lhs].get_owned_value(); + let rhs_value = state.registers[rhs].get_owned_value(); + + println!("lhs_value: {:?}, rhs_value: {:?}", lhs_value, rhs_value); + println!("jump_if_null: {}", jump_if_null); + + if matches!(lhs_value, Value::Integer(_)) && matches!(rhs_value, Value::Integer(_)) { + if lhs_value >= rhs_value { + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } + return Ok(InsnFunctionStepResult::Step); + } + + if matches!(lhs_value, Value::Null) || matches!(rhs_value, Value::Null) { + if jump_if_null { + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } + return Ok(InsnFunctionStepResult::Step); + } + + let mut lhs_temp_reg = state.registers[lhs].clone(); + let mut rhs_temp_reg = state.registers[rhs].clone(); + + let lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); + let rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + println!( + "lhs_converted: {}, rhs_converted: {}", + lhs_converted, rhs_converted + ); + println!( + "converted_lhs: {:?}, converted_rhs: {:?}", + lhs_temp_reg.get_owned_value(), + rhs_temp_reg.get_owned_value() + ); + let should_jump = match ( + lhs_temp_reg.get_owned_value(), + rhs_temp_reg.get_owned_value(), ) { - (_, Value::Null) | (Value::Null, _) => { - if jump_if_null { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - } - (Value::Text(lhs), Value::Text(rhs)) => { - let order = collation.compare_strings(lhs.as_str(), rhs.as_str()); - if order.is_ge() { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } + (Value::Text(lhs_text), Value::Text(rhs_text)) => { + let order = collation.compare_strings(lhs_text.as_str(), rhs_text.as_str()); + order.is_ge() } + (lhs, rhs) => { if *lhs >= *rhs { - state.pc = target_pc.to_offset_int(); + true } else { - state.pc += 1; + false } } + }; + + if lhs_converted { + state.registers[lhs] = lhs_temp_reg; } + if rhs_converted { + state.registers[rhs] = rhs_temp_reg; + } + + if should_jump { + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } + Ok(InsnFunctionStepResult::Step) } From 3b2980c7c0057fa5a33c29225e7a2b0ed038f05b Mon Sep 17 00:00:00 2001 From: krishvishal Date: Fri, 6 Jun 2025 13:18:43 +0530 Subject: [PATCH 06/20] Fix `op_seek` to handle affinity coercion --- core/vdbe/execute.rs | 228 +++++++++++++++++++++++++++---------------- 1 file changed, 143 insertions(+), 85 deletions(-) diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index 4c4e59c88..3589707d8 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -562,10 +562,10 @@ pub fn op_eq( // Fast path for integers if matches!(lhs_value, Value::Integer(_)) && matches!(rhs_value, Value::Integer(_)) { if lhs_value == rhs_value { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } return Ok(InsnFunctionStepResult::Step); } @@ -573,10 +573,10 @@ pub fn op_eq( if matches!(lhs_value, Value::Null) || matches!(rhs_value, Value::Null) { let cond = lhs_value == rhs_value; // Both NULL if (nulleq && cond) || (!nulleq && jump_if_null) { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } return Ok(InsnFunctionStepResult::Step); } @@ -605,10 +605,10 @@ pub fn op_eq( } if should_jump { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } Ok(InsnFunctionStepResult::Step) } @@ -644,10 +644,10 @@ pub fn op_ne( // Fast path for integers if matches!(lhs_value, Value::Integer(_)) && matches!(rhs_value, Value::Integer(_)) { if lhs_value != rhs_value { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } return Ok(InsnFunctionStepResult::Step); } @@ -655,10 +655,10 @@ pub fn op_ne( if matches!(lhs_value, Value::Null) || matches!(rhs_value, Value::Null) { let cond = lhs_value != rhs_value; // At least one NULL if (nulleq && cond) || (!nulleq && jump_if_null) { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } return Ok(InsnFunctionStepResult::Step); } @@ -687,10 +687,10 @@ pub fn op_ne( } if should_jump { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } Ok(InsnFunctionStepResult::Step) } @@ -725,20 +725,20 @@ pub fn op_lt( // Fast path for integers if matches!(lhs_value, Value::Integer(_)) && matches!(rhs_value, Value::Integer(_)) { if lhs_value < rhs_value { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } return Ok(InsnFunctionStepResult::Step); } // Handle NULL values if matches!(lhs_value, Value::Null) || matches!(rhs_value, Value::Null) { if jump_if_null { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } return Ok(InsnFunctionStepResult::Step); } @@ -767,10 +767,10 @@ pub fn op_lt( } if should_jump { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } Ok(InsnFunctionStepResult::Step) } @@ -805,20 +805,20 @@ pub fn op_le( // Fast path for integers if matches!(lhs_value, Value::Integer(_)) && matches!(rhs_value, Value::Integer(_)) { if lhs_value <= rhs_value { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } return Ok(InsnFunctionStepResult::Step); } // Handle NULL values if matches!(lhs_value, Value::Null) || matches!(rhs_value, Value::Null) { if jump_if_null { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } return Ok(InsnFunctionStepResult::Step); } @@ -847,10 +847,10 @@ pub fn op_le( } if should_jump { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } Ok(InsnFunctionStepResult::Step) } @@ -885,20 +885,20 @@ pub fn op_gt( // Fast path for integers if matches!(lhs_value, Value::Integer(_)) && matches!(rhs_value, Value::Integer(_)) { if lhs_value > rhs_value { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } return Ok(InsnFunctionStepResult::Step); } // Handle NULL values if matches!(lhs_value, Value::Null) || matches!(rhs_value, Value::Null) { if jump_if_null { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } return Ok(InsnFunctionStepResult::Step); } @@ -927,10 +927,10 @@ pub fn op_gt( } if should_jump { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } Ok(InsnFunctionStepResult::Step) } @@ -968,19 +968,19 @@ pub fn op_ge( if matches!(lhs_value, Value::Integer(_)) && matches!(rhs_value, Value::Integer(_)) { if lhs_value >= rhs_value { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } return Ok(InsnFunctionStepResult::Step); } if matches!(lhs_value, Value::Null) || matches!(rhs_value, Value::Null) { if jump_if_null { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } + state.pc = target_pc.to_offset_int(); + } else { + state.pc += 1; + } return Ok(InsnFunctionStepResult::Step); } @@ -1026,8 +1026,8 @@ pub fn op_ge( if should_jump { state.pc = target_pc.to_offset_int(); } else { - state.pc += 1; - } + state.pc += 1; + } Ok(InsnFunctionStepResult::Step) } @@ -2243,6 +2243,7 @@ pub fn op_seek( pager: &Rc, mv_store: Option<&Rc>, ) -> Result { + println!("op_seek"); let (Insn::SeekGE { cursor_id, start_reg, @@ -2312,69 +2313,126 @@ pub fn op_seek( state.pc += 1; } } else { + println!("Table seek branch "); let pc = { let original_value = state.registers[*start_reg].get_owned_value().clone(); + println!("original_value: {:?}", original_value); let mut temp_value = original_value.clone(); - if matches!(temp_value, Value::Text(_)) { + let conversion_successful = if matches!(temp_value, Value::Text(_)) { let mut temp_reg = Register::Value(temp_value); - apply_affinity_char(&mut temp_reg, Affinity::Numeric); + let converted = apply_numeric_affinity(&mut temp_reg, false); temp_value = temp_reg.get_owned_value().clone(); - } + converted + } else { + true // Non-text values don't need conversion + }; let int_key = extract_int_value(&temp_value); - let lost_precision = !matches!(temp_value, Value::Integer(_)); - + println!("int_key: {}", int_key); + let lost_precision = !conversion_successful || !matches!(temp_value, Value::Integer(_)); + println!("lost_precision: {}", lost_precision); + println!("temp_value: {:?}", temp_value); let actual_op = if lost_precision { - match (&temp_value, op) { - (Value::Float(f), op) => { + println!("Lost precision, actual_op might change"); + match &temp_value { + Value::Float(f) => { let int_key_as_float = int_key as f64; - if int_key_as_float > *f { + let c = if int_key_as_float > *f { + 1 + } else if int_key_as_float < *f { + -1 + } else { + 0 + }; + + if c > 0 { + // If approximation is larger than actual search term match op { - SeekOp::GT => SeekOp::GE, - SeekOp::LE => SeekOp::LE, + SeekOp::GT => SeekOp::GE, // (x > 4.9) -> (x >= 5) + SeekOp::LE => SeekOp::LT, // (x <= 4.9) -> (x < 5) other => other, } - } else if int_key_as_float < *f { + } else if c < 0 { + // If approximation is smaller than actual search term match op { - SeekOp::GE => SeekOp::GT, - SeekOp::LT => SeekOp::LE, + SeekOp::LT => SeekOp::LE, // (x < 5.1) -> (x <= 5) + SeekOp::GE => SeekOp::GT, // (x >= 5.1) -> (x > 5) other => other, } } else { op } } + Value::Text(_) => { + match op { + SeekOp::GT | SeekOp::GE => { + // No integers are > or >= non-numeric text, jump to target (empty result) + state.pc = target_pc.to_offset_int(); + return Ok(InsnFunctionStepResult::Step); + } + SeekOp::LT | SeekOp::LE => { + // All integers are < or <= non-numeric text + // Move to last position and then use the normal seek logic + { + let mut cursor = state.get_cursor(*cursor_id); + let cursor = cursor.as_btree_mut(); + return_if_io!(cursor.last()); + } + state.pc += 1; + return Ok(InsnFunctionStepResult::Step); + } + _ => unreachable!(), + } + } _ => op, } } else { + println!("No precision lost, using original op: {:?}", op); op }; + println!("actual_op: {:?}", actual_op); let rowid = if matches!(original_value, Value::Null) { + println!("NULL value handling"); match actual_op { SeekOp::GE | SeekOp::GT => { + println!("NULL + GE/GT -> jumping to target"); state.pc = target_pc.to_offset_int(); return Ok(InsnFunctionStepResult::Step); } SeekOp::LE | SeekOp::LT => { + println!("NULL + LE/LT -> jumping to target"); + // No integers are < NULL, so jump to target state.pc = target_pc.to_offset_int(); - return Ok(InsnFunctionStepResult::Step); } _ => unreachable!(), } } else { + println!("Using rowid: {}", int_key); int_key }; let mut cursor = state.get_cursor(*cursor_id); let cursor = cursor.as_btree_mut(); - + println!( + "About to call cursor.seek with rowid={}, actual_op={:?}", + rowid, actual_op + ); let found = return_if_io!(cursor.seek(SeekKey::TableRowId(rowid), actual_op)); + println!("cursor.seek returned found={}", found); if !found { + println!( + "Not found, jumping to target_pc={}", + target_pc.to_offset_int() + ); target_pc.to_offset_int() } else { + println!( + "Not found, jumping to target_pc={}", + target_pc.to_offset_int() + ); state.pc + 1 } }; From 9130b2511109b005ce7451f18c4610d2373dc550 Mon Sep 17 00:00:00 2001 From: krishvishal Date: Fri, 6 Jun 2025 13:20:12 +0530 Subject: [PATCH 07/20] Add `jump_if_null` flag for rowid alias based seeks --- core/translate/main_loop.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/translate/main_loop.rs b/core/translate/main_loop.rs index 8a35bb50b..b82ad3c75 100644 --- a/core/translate/main_loop.rs +++ b/core/translate/main_loop.rs @@ -1322,28 +1322,28 @@ fn emit_seek_termination( lhs: rowid_reg.unwrap(), rhs: start_reg, target_pc: loop_end, - flags: CmpInsFlags::default(), + flags: CmpInsFlags::default().jump_if_null(), collation: program.curr_collation(), }), (false, SeekOp::GT) => program.emit_insn(Insn::Gt { lhs: rowid_reg.unwrap(), rhs: start_reg, target_pc: loop_end, - flags: CmpInsFlags::default(), + flags: CmpInsFlags::default().jump_if_null(), collation: program.curr_collation(), }), (false, SeekOp::LE { .. }) => program.emit_insn(Insn::Le { lhs: rowid_reg.unwrap(), rhs: start_reg, target_pc: loop_end, - flags: CmpInsFlags::default(), + flags: CmpInsFlags::default().jump_if_null(), collation: program.curr_collation(), }), (false, SeekOp::LT) => program.emit_insn(Insn::Lt { lhs: rowid_reg.unwrap(), rhs: start_reg, target_pc: loop_end, - flags: CmpInsFlags::default(), + flags: CmpInsFlags::default().jump_if_null(), collation: program.curr_collation(), }), }; From 7bd1589615e1036873dbbc268cc9ac8aaca6d061 Mon Sep 17 00:00:00 2001 From: krishvishal Date: Fri, 6 Jun 2025 13:57:30 +0530 Subject: [PATCH 08/20] Added affinity inference and conversion for comparison ops. Added affinity helper function for `CmpInsFlags` --- core/schema.rs | 16 +++++ core/translate/expr.rs | 138 ++++++++++++++++++++++++++++++++++++++--- core/vdbe/insn.rs | 14 ++++- 3 files changed, 157 insertions(+), 11 deletions(-) diff --git a/core/schema.rs b/core/schema.rs index a30d59dcf..eceabbcfc 100644 --- a/core/schema.rs +++ b/core/schema.rs @@ -748,6 +748,22 @@ impl Affinity { ))), } } + + pub fn to_char_code(&self) -> u8 { + self.aff_mask() as u8 + } + + pub fn from_char_code(code: u8) -> Result { + Self::from_char(code as char) + } + + pub fn is_numeric(&self) -> bool { + matches!(self, Affinity::Integer | Affinity::Real | Affinity::Numeric) + } + + pub fn has_affinity(&self) -> bool { + !matches!(self, Affinity::Blob) + } } impl fmt::Display for Type { diff --git a/core/translate/expr.rs b/core/translate/expr.rs index 3371dbaf9..6b983e49a 100644 --- a/core/translate/expr.rs +++ b/core/translate/expr.rs @@ -1,4 +1,4 @@ -use limbo_sqlite3_parser::ast::{self, UnaryOperator}; +use limbo_sqlite3_parser::ast::{self, Expr, UnaryOperator}; use tracing::{instrument, Level}; use super::emitter::Resolver; @@ -8,7 +8,7 @@ use super::plan::TableReferences; use crate::function::JsonFunc; use crate::function::{Func, FuncCtx, MathFuncArity, ScalarFunc, VectorFunc}; use crate::functions::datetime; -use crate::schema::{Table, Type}; +use crate::schema::{Affinity, Table, Type}; use crate::util::{exprs_are_equivalent, normalize_ident, parse_numeric_literal}; use crate::vdbe::builder::CursorKey; use crate::vdbe::{ @@ -458,11 +458,30 @@ pub fn translate_expr( } ast::Expr::Binary(e1, op, e2) => { // Check if both sides of the expression are equivalent and reuse the same register if so + println!("expr: {:?}", &expr); + println!("e1: {:?}, e2: {:?}, op: {:?}", e1, e2, op); + println!( + "expr affinity: {:?}", + get_expr_affinity(e1, referenced_tables) + ); + println!( + "expr affinity: {:?}", + get_expr_affinity(e2, referenced_tables) + ); if exprs_are_equivalent(e1, e2) { let shared_reg = program.alloc_register(); translate_expr(program, referenced_tables, e1, shared_reg, resolver)?; - emit_binary_insn(program, op, shared_reg, shared_reg, target_register)?; + emit_binary_insn( + program, + op, + shared_reg, + shared_reg, + target_register, + e1, + e2, + referenced_tables, + )?; program.reset_collation(); Ok(target_register) } else { @@ -509,7 +528,16 @@ pub fn translate_expr( }; program.set_collation(collation_ctx); - emit_binary_insn(program, op, e1_reg, e2_reg, target_register)?; + emit_binary_insn( + program, + op, + e1_reg, + e2_reg, + target_register, + e1, + e2, + referenced_tables, + )?; program.reset_collation(); Ok(target_register) } @@ -2201,7 +2229,25 @@ fn emit_binary_insn( lhs: usize, rhs: usize, target_register: usize, + lhs_expr: &Expr, + rhs_expr: &Expr, + referenced_tables: Option<&TableReferences>, ) -> Result<()> { + let mut affinity = Affinity::Blob; + if matches!( + op, + ast::Operator::Equals + | ast::Operator::NotEquals + | ast::Operator::Less + | ast::Operator::LessEquals + | ast::Operator::Greater + | ast::Operator::GreaterEquals + | ast::Operator::Is + | ast::Operator::IsNot + ) { + affinity = comparison_affinity(lhs_expr, rhs_expr, referenced_tables); + } + println!("emit_binary affinity: {:?}", affinity); match op { ast::Operator::NotEquals => { let if_true_label = program.allocate_label(); @@ -2211,7 +2257,7 @@ fn emit_binary_insn( lhs, rhs, target_pc: if_true_label, - flags: CmpInsFlags::default(), + flags: CmpInsFlags::default().with_affinity(affinity), collation: program.curr_collation(), }, target_register, @@ -2228,7 +2274,7 @@ fn emit_binary_insn( lhs, rhs, target_pc: if_true_label, - flags: CmpInsFlags::default(), + flags: CmpInsFlags::default().with_affinity(affinity), collation: program.curr_collation(), }, target_register, @@ -2245,7 +2291,7 @@ fn emit_binary_insn( lhs, rhs, target_pc: if_true_label, - flags: CmpInsFlags::default(), + flags: CmpInsFlags::default().with_affinity(affinity), collation: program.curr_collation(), }, target_register, @@ -2262,7 +2308,7 @@ fn emit_binary_insn( lhs, rhs, target_pc: if_true_label, - flags: CmpInsFlags::default(), + flags: CmpInsFlags::default().with_affinity(affinity), collation: program.curr_collation(), }, target_register, @@ -2279,7 +2325,7 @@ fn emit_binary_insn( lhs, rhs, target_pc: if_true_label, - flags: CmpInsFlags::default(), + flags: CmpInsFlags::default().with_affinity(affinity), collation: program.curr_collation(), }, target_register, @@ -2296,7 +2342,7 @@ fn emit_binary_insn( lhs, rhs, target_pc: if_true_label, - flags: CmpInsFlags::default(), + flags: CmpInsFlags::default().with_affinity(affinity), collation: program.curr_collation(), }, target_register, @@ -3023,3 +3069,75 @@ where Ok(()) } + +pub fn get_expr_affinity( + expr: &ast::Expr, + referenced_tables: Option<&TableReferences>, +) -> Affinity { + match expr { + ast::Expr::Column { table, column, .. } => { + if let Some(tables) = referenced_tables { + if let Some(table_ref) = tables.find_table_by_internal_id(*table) { + if let Some(col) = table_ref.get_column_at(*column) { + return col.affinity(); + } + } + } + Affinity::Blob + } + ast::Expr::Cast { type_name, .. } => { + if let Some(type_name) = type_name { + crate::schema::affinity(&type_name.name) + } else { + Affinity::Blob + } + } + ast::Expr::Collate(expr, _) => get_expr_affinity(expr, referenced_tables), + // Literals have NO affinity in SQLite! + ast::Expr::Literal(_) => Affinity::Blob, // No affinity! + _ => Affinity::Blob, // This may need to change. For now this works. + } +} + +pub fn comparison_affinity( + lhs_expr: &ast::Expr, + rhs_expr: &ast::Expr, + referenced_tables: Option<&TableReferences>, +) -> Affinity { + let mut aff = get_expr_affinity(lhs_expr, referenced_tables); + + aff = compare_affinity(rhs_expr, aff, referenced_tables); + + // If no affinity determined (both operands are literals), default to BLOB + if !aff.has_affinity() { + Affinity::Blob + } else { + aff + } +} + +pub fn compare_affinity( + expr: &ast::Expr, + other_affinity: Affinity, + referenced_tables: Option<&TableReferences>, +) -> Affinity { + let expr_affinity = get_expr_affinity(expr, referenced_tables); + + if expr_affinity.has_affinity() && other_affinity.has_affinity() { + // Both sides have affinity - use numeric if either is numeric + if expr_affinity.is_numeric() || other_affinity.is_numeric() { + Affinity::Numeric + } else { + Affinity::Blob + } + } else { + // One or both sides have no affinity - use the one that does, or Blob if neither + if expr_affinity.has_affinity() { + expr_affinity + } else if other_affinity.has_affinity() { + other_affinity + } else { + Affinity::Blob + } + } +} diff --git a/core/vdbe/insn.rs b/core/vdbe/insn.rs index 23b9da480..c6a13840c 100644 --- a/core/vdbe/insn.rs +++ b/core/vdbe/insn.rs @@ -6,7 +6,7 @@ use std::{ use super::{execute, AggFunc, BranchOffset, CursorID, FuncCtx, InsnFunction, PageIdx}; use crate::{ - schema::{BTreeTable, Index}, + schema::{Affinity, BTreeTable, Index}, storage::{pager::CreateBTreeFlags, wal::CheckpointMode}, translate::collate::CollationSeq, }; @@ -20,6 +20,7 @@ pub struct CmpInsFlags(usize); impl CmpInsFlags { const NULL_EQ: usize = 0x80; const JUMP_IF_NULL: usize = 0x10; + const AFFINITY_MASK: usize = 0x0F; fn has(&self, flag: usize) -> bool { (self.0 & flag) != 0 @@ -42,6 +43,17 @@ impl CmpInsFlags { pub fn has_nulleq(&self) -> bool { self.has(CmpInsFlags::NULL_EQ) } + + pub fn with_affinity(mut self, affinity: Affinity) -> Self { + let aff_code = affinity.to_char_code() as usize; + self.0 = (self.0 & !Self::AFFINITY_MASK) | aff_code; + self + } + + pub fn get_affinity(&self) -> Affinity { + let aff_code = (self.0 & Self::AFFINITY_MASK) as u8; + Affinity::from_char_code(aff_code).unwrap_or(Affinity::Blob) + } } #[derive(Clone, Copy, Debug, Default)] From faa9aedbaeb0b7ed0e13af418442be248778fcb4 Mon Sep 17 00:00:00 2001 From: krishvishal Date: Fri, 6 Jun 2025 16:12:32 +0530 Subject: [PATCH 09/20] Add affinity based type coercion to comparison ops --- core/vdbe/execute.rs | 370 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 358 insertions(+), 12 deletions(-) diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index 3589707d8..219b87b23 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -554,6 +554,8 @@ pub fn op_eq( let target_pc = *target_pc; let nulleq = flags.has_nulleq(); let jump_if_null = flags.has_jump_if_null(); + let affinity = flags.get_affinity(); + println!("execute affinity: {:?}", affinity); let collation = collation.unwrap_or_default(); let lhs_value = state.registers[lhs].get_owned_value(); @@ -583,8 +585,61 @@ pub fn op_eq( let mut lhs_temp_reg = state.registers[lhs].clone(); let mut rhs_temp_reg = state.registers[rhs].clone(); - let lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); - let rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + let mut lhs_converted = false; + let mut rhs_converted = false; + + match affinity { + Affinity::Numeric | Affinity::Integer => { + let lhs_is_text = matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)); + let rhs_is_text = matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)); + + if lhs_is_text || rhs_is_text { + if lhs_is_text { + lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); + } + if rhs_is_text { + rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + } + } + } + + Affinity::Text => { + let lhs_is_text = matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)); + let rhs_is_text = matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)); + + if lhs_is_text || rhs_is_text { + if is_numeric_value(&lhs_temp_reg) { + lhs_converted = stringify_register(&mut lhs_temp_reg); + } + + if is_numeric_value(&rhs_temp_reg) { + rhs_converted = stringify_register(&mut rhs_temp_reg); + } + } + } + + Affinity::Real => { + if matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)) { + lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); + } + + if matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)) { + rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + } + + if let Value::Integer(i) = lhs_temp_reg.get_owned_value() { + lhs_temp_reg = Register::Value(Value::Float(*i as f64)); + lhs_converted = true; + } + + if let Value::Integer(i) = rhs_temp_reg.get_owned_value() { + rhs_temp_reg = Register::Value(Value::Float(*i as f64)); + rhs_converted = true; + } + } + + Affinity::Blob => {} // Do nothing for blob affinity. + } let should_jump = match ( lhs_temp_reg.get_owned_value(), @@ -636,6 +691,8 @@ pub fn op_ne( let target_pc = *target_pc; let nulleq = flags.has_nulleq(); let jump_if_null = flags.has_jump_if_null(); + let affinity = flags.get_affinity(); + println!("execute affinity: {:?}", affinity); let collation = collation.unwrap_or_default(); let lhs_value = state.registers[lhs].get_owned_value(); @@ -665,8 +722,61 @@ pub fn op_ne( let mut lhs_temp_reg = state.registers[lhs].clone(); let mut rhs_temp_reg = state.registers[rhs].clone(); - let lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); - let rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + let mut lhs_converted = false; + let mut rhs_converted = false; + + match affinity { + Affinity::Numeric | Affinity::Integer => { + let lhs_is_text = matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)); + let rhs_is_text = matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)); + + if lhs_is_text || rhs_is_text { + if lhs_is_text { + lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); + } + if rhs_is_text { + rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + } + } + } + + Affinity::Text => { + let lhs_is_text = matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)); + let rhs_is_text = matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)); + + if lhs_is_text || rhs_is_text { + if is_numeric_value(&lhs_temp_reg) { + lhs_converted = stringify_register(&mut lhs_temp_reg); + } + + if is_numeric_value(&rhs_temp_reg) { + rhs_converted = stringify_register(&mut rhs_temp_reg); + } + } + } + + Affinity::Real => { + if matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)) { + lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); + } + + if matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)) { + rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + } + + if let Value::Integer(i) = lhs_temp_reg.get_owned_value() { + lhs_temp_reg = Register::Value(Value::Float(*i as f64)); + lhs_converted = true; + } + + if let Value::Integer(i) = rhs_temp_reg.get_owned_value() { + rhs_temp_reg = Register::Value(Value::Float(*i as f64)); + rhs_converted = true; + } + } + + Affinity::Blob => {} // Do nothing for blob affinity. + } let should_jump = match ( lhs_temp_reg.get_owned_value(), @@ -717,6 +827,8 @@ pub fn op_lt( let rhs = *rhs; let target_pc = *target_pc; let jump_if_null = flags.has_jump_if_null(); + let affinity = flags.get_affinity(); + println!("execute affinity: {:?}", affinity); let collation = collation.unwrap_or_default(); let lhs_value = state.registers[lhs].get_owned_value(); @@ -745,8 +857,61 @@ pub fn op_lt( let mut lhs_temp_reg = state.registers[lhs].clone(); let mut rhs_temp_reg = state.registers[rhs].clone(); - let lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); - let rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + let mut lhs_converted = false; + let mut rhs_converted = false; + + match affinity { + Affinity::Numeric | Affinity::Integer => { + let lhs_is_text = matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)); + let rhs_is_text = matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)); + + if lhs_is_text || rhs_is_text { + if lhs_is_text { + lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); + } + if rhs_is_text { + rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + } + } + } + + Affinity::Text => { + let lhs_is_text = matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)); + let rhs_is_text = matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)); + + if lhs_is_text || rhs_is_text { + if is_numeric_value(&lhs_temp_reg) { + lhs_converted = stringify_register(&mut lhs_temp_reg); + } + + if is_numeric_value(&rhs_temp_reg) { + rhs_converted = stringify_register(&mut rhs_temp_reg); + } + } + } + + Affinity::Real => { + if matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)) { + lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); + } + + if matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)) { + rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + } + + if let Value::Integer(i) = lhs_temp_reg.get_owned_value() { + lhs_temp_reg = Register::Value(Value::Float(*i as f64)); + lhs_converted = true; + } + + if let Value::Integer(i) = rhs_temp_reg.get_owned_value() { + rhs_temp_reg = Register::Value(Value::Float(*i as f64)); + rhs_converted = true; + } + } + + Affinity::Blob => {} // Do nothing for blob affinity. + } let should_jump = match ( lhs_temp_reg.get_owned_value(), @@ -797,6 +962,8 @@ pub fn op_le( let rhs = *rhs; let target_pc = *target_pc; let jump_if_null = flags.has_jump_if_null(); + let affinity = flags.get_affinity(); + println!("execute affinity: {:?}", affinity); let collation = collation.unwrap_or_default(); let lhs_value = state.registers[lhs].get_owned_value(); @@ -825,8 +992,61 @@ pub fn op_le( let mut lhs_temp_reg = state.registers[lhs].clone(); let mut rhs_temp_reg = state.registers[rhs].clone(); - let lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); - let rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + let mut lhs_converted = false; + let mut rhs_converted = false; + + match affinity { + Affinity::Numeric | Affinity::Integer => { + let lhs_is_text = matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)); + let rhs_is_text = matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)); + + if lhs_is_text || rhs_is_text { + if lhs_is_text { + lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); + } + if rhs_is_text { + rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + } + } + } + + Affinity::Text => { + let lhs_is_text = matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)); + let rhs_is_text = matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)); + + if lhs_is_text || rhs_is_text { + if is_numeric_value(&lhs_temp_reg) { + lhs_converted = stringify_register(&mut lhs_temp_reg); + } + + if is_numeric_value(&rhs_temp_reg) { + rhs_converted = stringify_register(&mut rhs_temp_reg); + } + } + } + + Affinity::Real => { + if matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)) { + lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); + } + + if matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)) { + rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + } + + if let Value::Integer(i) = lhs_temp_reg.get_owned_value() { + lhs_temp_reg = Register::Value(Value::Float(*i as f64)); + lhs_converted = true; + } + + if let Value::Integer(i) = rhs_temp_reg.get_owned_value() { + rhs_temp_reg = Register::Value(Value::Float(*i as f64)); + rhs_converted = true; + } + } + + Affinity::Blob => {} // Do nothing for blob affinity. + } let should_jump = match ( lhs_temp_reg.get_owned_value(), @@ -877,6 +1097,8 @@ pub fn op_gt( let rhs = *rhs; let target_pc = *target_pc; let jump_if_null = flags.has_jump_if_null(); + let affinity = flags.get_affinity(); + println!("execute affinity: {:?}", affinity); let collation = collation.unwrap_or_default(); let lhs_value = state.registers[lhs].get_owned_value(); @@ -905,8 +1127,60 @@ pub fn op_gt( let mut lhs_temp_reg = state.registers[lhs].clone(); let mut rhs_temp_reg = state.registers[rhs].clone(); - let lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); - let rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + let mut lhs_converted = false; + let mut rhs_converted = false; + + match affinity { + Affinity::Numeric | Affinity::Integer => { + let lhs_is_text = matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)); + let rhs_is_text = matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)); + + if lhs_is_text || rhs_is_text { + if lhs_is_text { + lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); + } + if rhs_is_text { + rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + } + } + } + + Affinity::Text => { + let lhs_is_text = matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)); + let rhs_is_text = matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)); + + if lhs_is_text || rhs_is_text { + if is_numeric_value(&lhs_temp_reg) { + lhs_converted = stringify_register(&mut lhs_temp_reg); + } + + if is_numeric_value(&rhs_temp_reg) { + rhs_converted = stringify_register(&mut rhs_temp_reg); + } + } + } + + Affinity::Real => { + if matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)) { + lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); + } + if matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)) { + rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + } + + if let Value::Integer(i) = lhs_temp_reg.get_owned_value() { + lhs_temp_reg = Register::Value(Value::Float(*i as f64)); + lhs_converted = true; + } + + if let Value::Integer(i) = rhs_temp_reg.get_owned_value() { + rhs_temp_reg = Register::Value(Value::Float(*i as f64)); + rhs_converted = true; + } + } + + Affinity::Blob => {} // Do nothing for blob affinity. + } let should_jump = match ( lhs_temp_reg.get_owned_value(), @@ -958,6 +1232,8 @@ pub fn op_ge( let rhs = *rhs; let target_pc = *target_pc; let jump_if_null = flags.has_jump_if_null(); + let affinity = flags.get_affinity(); + println!("execute affinity: {:?}", affinity); let collation = collation.unwrap_or_default(); let lhs_value = state.registers[lhs].get_owned_value(); @@ -987,8 +1263,60 @@ pub fn op_ge( let mut lhs_temp_reg = state.registers[lhs].clone(); let mut rhs_temp_reg = state.registers[rhs].clone(); - let lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); - let rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + let mut lhs_converted = false; + let mut rhs_converted = false; + + match affinity { + Affinity::Numeric | Affinity::Integer => { + let lhs_is_text = matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)); + let rhs_is_text = matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)); + + if lhs_is_text || rhs_is_text { + if lhs_is_text { + lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); + } + if rhs_is_text { + rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + } + } + } + + Affinity::Text => { + let lhs_is_text = matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)); + let rhs_is_text = matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)); + + if lhs_is_text || rhs_is_text { + if is_numeric_value(&lhs_temp_reg) { + lhs_converted = stringify_register(&mut lhs_temp_reg); + } + + if is_numeric_value(&rhs_temp_reg) { + rhs_converted = stringify_register(&mut rhs_temp_reg); + } + } + } + + Affinity::Real => { + if matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)) { + lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); + } + if matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)) { + rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); + } + + if let Value::Integer(i) = lhs_temp_reg.get_owned_value() { + lhs_temp_reg = Register::Value(Value::Float(*i as f64)); + lhs_converted = true; + } + + if let Value::Integer(i) = rhs_temp_reg.get_owned_value() { + rhs_temp_reg = Register::Value(Value::Float(*i as f64)); + rhs_converted = true; + } + } + + Affinity::Blob => {} // Do nothing for blob affinity. + } println!( "lhs_converted: {}, rhs_converted: {}", lhs_converted, rhs_converted @@ -6699,6 +7027,24 @@ pub fn apply_numeric_affinity(register: &mut Register, try_for_int: bool) -> boo } } +fn is_numeric_value(reg: &Register) -> bool { + matches!(reg.get_owned_value(), Value::Integer(_) | Value::Float(_)) +} + +fn stringify_register(reg: &mut Register) -> bool { + match reg.get_owned_value() { + Value::Integer(i) => { + *reg = Register::Value(Value::build_text(&i.to_string())); + true + } + Value::Float(f) => { + *reg = Register::Value(Value::build_text(&f.to_string())); + true + } + Value::Text(_) | Value::Null | Value::Blob(_) => false, + } +} + #[cfg(test)] mod tests { use super::*; From f0dda1702ff96c60bc0519ce05f0cf75c017eab6 Mon Sep 17 00:00:00 2001 From: krishvishal Date: Fri, 6 Jun 2025 16:13:06 +0530 Subject: [PATCH 10/20] Fix `AFFINITY_MASK` value --- core/vdbe/insn.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/vdbe/insn.rs b/core/vdbe/insn.rs index c6a13840c..11632e96f 100644 --- a/core/vdbe/insn.rs +++ b/core/vdbe/insn.rs @@ -20,7 +20,7 @@ pub struct CmpInsFlags(usize); impl CmpInsFlags { const NULL_EQ: usize = 0x80; const JUMP_IF_NULL: usize = 0x10; - const AFFINITY_MASK: usize = 0x0F; + const AFFINITY_MASK: usize = 0x47; fn has(&self, flag: usize) -> bool { (self.0 & flag) != 0 From 6c04c18f8714a1714699b7be557d30e27383fec3 Mon Sep 17 00:00:00 2001 From: krishvishal Date: Fri, 6 Jun 2025 20:02:57 +0530 Subject: [PATCH 11/20] Add affinity flag to comparison opcodes --- core/translate/main_loop.rs | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/core/translate/main_loop.rs b/core/translate/main_loop.rs index b82ad3c75..e5731c0eb 100644 --- a/core/translate/main_loop.rs +++ b/core/translate/main_loop.rs @@ -4,7 +4,7 @@ use limbo_sqlite3_parser::ast::{self, SortOrder}; use std::sync::Arc; use crate::{ - schema::{Index, IndexColumn, Table}, + schema::{Affinity, Index, IndexColumn, Table}, translate::{ plan::{DistinctCtx, Distinctness}, result_row::emit_select_result, @@ -1285,14 +1285,29 @@ fn emit_seek_termination( } program.preassign_label_to_next_insn(loop_start); let mut rowid_reg = None; + let mut affinity = None; if !is_index { rowid_reg = Some(program.alloc_register()); program.emit_insn(Insn::RowId { cursor_id: seek_cursor_id, dest: rowid_reg.unwrap(), }); - } + affinity = if let Some(table_ref) = tables + .joined_tables() + .iter() + .find(|t| t.columns().iter().any(|c| c.is_rowid_alias)) + { + if let Some(rowid_col_idx) = table_ref.columns().iter().position(|c| c.is_rowid_alias) { + Some(table_ref.columns()[rowid_col_idx].affinity()) + } else { + Some(Affinity::Numeric) + } + } else { + Some(Affinity::Numeric) + }; + } + println!("#### emit_seek affinity####: {:?}", affinity); match (is_index, termination.op) { (true, SeekOp::GE { .. }) => program.emit_insn(Insn::IdxGE { cursor_id: seek_cursor_id, @@ -1322,28 +1337,36 @@ fn emit_seek_termination( lhs: rowid_reg.unwrap(), rhs: start_reg, target_pc: loop_end, - flags: CmpInsFlags::default().jump_if_null(), + flags: CmpInsFlags::default() + .jump_if_null() + .with_affinity(affinity.unwrap()), collation: program.curr_collation(), }), (false, SeekOp::GT) => program.emit_insn(Insn::Gt { lhs: rowid_reg.unwrap(), rhs: start_reg, target_pc: loop_end, - flags: CmpInsFlags::default().jump_if_null(), + flags: CmpInsFlags::default() + .jump_if_null() + .with_affinity(affinity.unwrap()), collation: program.curr_collation(), }), (false, SeekOp::LE { .. }) => program.emit_insn(Insn::Le { lhs: rowid_reg.unwrap(), rhs: start_reg, target_pc: loop_end, - flags: CmpInsFlags::default().jump_if_null(), + flags: CmpInsFlags::default() + .jump_if_null() + .with_affinity(affinity.unwrap()), collation: program.curr_collation(), }), (false, SeekOp::LT) => program.emit_insn(Insn::Lt { lhs: rowid_reg.unwrap(), rhs: start_reg, target_pc: loop_end, - flags: CmpInsFlags::default().jump_if_null(), + flags: CmpInsFlags::default() + .jump_if_null() + .with_affinity(affinity.unwrap()), collation: program.curr_collation(), }), }; From d13abad4b1ccb6b785fcaaa1a4fc17e456ecc7d3 Mon Sep 17 00:00:00 2001 From: krishvishal Date: Fri, 6 Jun 2025 20:08:18 +0530 Subject: [PATCH 12/20] Handle Blob type together with Text type in op_seek --- core/vdbe/execute.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index 219b87b23..49f27ffa4 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -2692,7 +2692,7 @@ pub fn op_seek( op } } - Value::Text(_) => { + Value::Text(_) | Value::Blob(_) => { match op { SeekOp::GT | SeekOp::GE => { // No integers are > or >= non-numeric text, jump to target (empty result) From e01f4d55f7405afbb657cff5a01849d0937819fb Mon Sep 17 00:00:00 2001 From: krishvishal Date: Fri, 6 Jun 2025 20:09:11 +0530 Subject: [PATCH 13/20] Reduce fuzz iters --- tests/integration/fuzz/mod.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/integration/fuzz/mod.rs b/tests/integration/fuzz/mod.rs index 709e48990..789e39907 100644 --- a/tests/integration/fuzz/mod.rs +++ b/tests/integration/fuzz/mod.rs @@ -72,7 +72,7 @@ mod tests { let (mut rng, seed) = rng_from_time_or_env(); tracing::info!("rowid_seek_fuzz seed: {}", seed); - for iteration in 0..100 { + for iteration in 0..2 { tracing::trace!("rowid_seek_fuzz iteration: {}", iteration); for comp in COMPARISONS.iter() { @@ -88,7 +88,7 @@ mod tests { ); log::trace!("query: {}", query); - + println!("query: {}", query); let limbo_result = limbo_exec_rows(&db, &limbo_conn, &query); let sqlite_result = sqlite_exec_rows(&sqlite_conn, &query); assert_eq!( @@ -119,6 +119,8 @@ mod tests { values.push(val.to_string()); } + values.push("NULL".to_string()); // Man's greatest mistake + values.push("'NULL'".to_string()); // SQLite dared to one up on that mistake values.push("0.0".to_string()); values.push("-0.0".to_string()); values.push("1.5".to_string()); @@ -143,7 +145,7 @@ mod tests { values.push("X''".to_string()); values.push("(1 + 1)".to_string()); - values.push("(SELECT 1)".to_string()); + // values.push("(SELECT 1)".to_string()); subqueries ain't implemented yet homes. values } From 5837f7329f777a4f2acdb89a361a81b723455407 Mon Sep 17 00:00:00 2001 From: krishvishal Date: Fri, 6 Jun 2025 20:27:20 +0530 Subject: [PATCH 14/20] clean up --- core/translate/expr.rs | 11 --------- core/translate/main_loop.rs | 1 - core/vdbe/execute.rs | 45 +---------------------------------- tests/integration/fuzz/mod.rs | 1 - 4 files changed, 1 insertion(+), 57 deletions(-) diff --git a/core/translate/expr.rs b/core/translate/expr.rs index 6b983e49a..a015875fa 100644 --- a/core/translate/expr.rs +++ b/core/translate/expr.rs @@ -458,16 +458,6 @@ pub fn translate_expr( } ast::Expr::Binary(e1, op, e2) => { // Check if both sides of the expression are equivalent and reuse the same register if so - println!("expr: {:?}", &expr); - println!("e1: {:?}, e2: {:?}, op: {:?}", e1, e2, op); - println!( - "expr affinity: {:?}", - get_expr_affinity(e1, referenced_tables) - ); - println!( - "expr affinity: {:?}", - get_expr_affinity(e2, referenced_tables) - ); if exprs_are_equivalent(e1, e2) { let shared_reg = program.alloc_register(); translate_expr(program, referenced_tables, e1, shared_reg, resolver)?; @@ -2247,7 +2237,6 @@ fn emit_binary_insn( ) { affinity = comparison_affinity(lhs_expr, rhs_expr, referenced_tables); } - println!("emit_binary affinity: {:?}", affinity); match op { ast::Operator::NotEquals => { let if_true_label = program.allocate_label(); diff --git a/core/translate/main_loop.rs b/core/translate/main_loop.rs index e5731c0eb..17868d9a7 100644 --- a/core/translate/main_loop.rs +++ b/core/translate/main_loop.rs @@ -1307,7 +1307,6 @@ fn emit_seek_termination( Some(Affinity::Numeric) }; } - println!("#### emit_seek affinity####: {:?}", affinity); match (is_index, termination.op) { (true, SeekOp::GE { .. }) => program.emit_insn(Insn::IdxGE { cursor_id: seek_cursor_id, diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index 49f27ffa4..1d05f1bd7 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -555,7 +555,6 @@ pub fn op_eq( let nulleq = flags.has_nulleq(); let jump_if_null = flags.has_jump_if_null(); let affinity = flags.get_affinity(); - println!("execute affinity: {:?}", affinity); let collation = collation.unwrap_or_default(); let lhs_value = state.registers[lhs].get_owned_value(); @@ -692,7 +691,6 @@ pub fn op_ne( let nulleq = flags.has_nulleq(); let jump_if_null = flags.has_jump_if_null(); let affinity = flags.get_affinity(); - println!("execute affinity: {:?}", affinity); let collation = collation.unwrap_or_default(); let lhs_value = state.registers[lhs].get_owned_value(); @@ -828,7 +826,6 @@ pub fn op_lt( let target_pc = *target_pc; let jump_if_null = flags.has_jump_if_null(); let affinity = flags.get_affinity(); - println!("execute affinity: {:?}", affinity); let collation = collation.unwrap_or_default(); let lhs_value = state.registers[lhs].get_owned_value(); @@ -963,7 +960,6 @@ pub fn op_le( let target_pc = *target_pc; let jump_if_null = flags.has_jump_if_null(); let affinity = flags.get_affinity(); - println!("execute affinity: {:?}", affinity); let collation = collation.unwrap_or_default(); let lhs_value = state.registers[lhs].get_owned_value(); @@ -1098,7 +1094,6 @@ pub fn op_gt( let target_pc = *target_pc; let jump_if_null = flags.has_jump_if_null(); let affinity = flags.get_affinity(); - println!("execute affinity: {:?}", affinity); let collation = collation.unwrap_or_default(); let lhs_value = state.registers[lhs].get_owned_value(); @@ -1233,15 +1228,11 @@ pub fn op_ge( let target_pc = *target_pc; let jump_if_null = flags.has_jump_if_null(); let affinity = flags.get_affinity(); - println!("execute affinity: {:?}", affinity); let collation = collation.unwrap_or_default(); let lhs_value = state.registers[lhs].get_owned_value(); let rhs_value = state.registers[rhs].get_owned_value(); - println!("lhs_value: {:?}, rhs_value: {:?}", lhs_value, rhs_value); - println!("jump_if_null: {}", jump_if_null); - if matches!(lhs_value, Value::Integer(_)) && matches!(rhs_value, Value::Integer(_)) { if lhs_value >= rhs_value { state.pc = target_pc.to_offset_int(); @@ -1317,15 +1308,7 @@ pub fn op_ge( Affinity::Blob => {} // Do nothing for blob affinity. } - println!( - "lhs_converted: {}, rhs_converted: {}", - lhs_converted, rhs_converted - ); - println!( - "converted_lhs: {:?}, converted_rhs: {:?}", - lhs_temp_reg.get_owned_value(), - rhs_temp_reg.get_owned_value() - ); + let should_jump = match ( lhs_temp_reg.get_owned_value(), rhs_temp_reg.get_owned_value(), @@ -2571,7 +2554,6 @@ pub fn op_seek( pager: &Rc, mv_store: Option<&Rc>, ) -> Result { - println!("op_seek"); let (Insn::SeekGE { cursor_id, start_reg, @@ -2641,10 +2623,8 @@ pub fn op_seek( state.pc += 1; } } else { - println!("Table seek branch "); let pc = { let original_value = state.registers[*start_reg].get_owned_value().clone(); - println!("original_value: {:?}", original_value); let mut temp_value = original_value.clone(); let conversion_successful = if matches!(temp_value, Value::Text(_)) { @@ -2657,12 +2637,8 @@ pub fn op_seek( }; let int_key = extract_int_value(&temp_value); - println!("int_key: {}", int_key); let lost_precision = !conversion_successful || !matches!(temp_value, Value::Integer(_)); - println!("lost_precision: {}", lost_precision); - println!("temp_value: {:?}", temp_value); let actual_op = if lost_precision { - println!("Lost precision, actual_op might change"); match &temp_value { Value::Float(f) => { let int_key_as_float = int_key as f64; @@ -2716,21 +2692,16 @@ pub fn op_seek( _ => op, } } else { - println!("No precision lost, using original op: {:?}", op); op }; - println!("actual_op: {:?}", actual_op); let rowid = if matches!(original_value, Value::Null) { - println!("NULL value handling"); match actual_op { SeekOp::GE | SeekOp::GT => { - println!("NULL + GE/GT -> jumping to target"); state.pc = target_pc.to_offset_int(); return Ok(InsnFunctionStepResult::Step); } SeekOp::LE | SeekOp::LT => { - println!("NULL + LE/LT -> jumping to target"); // No integers are < NULL, so jump to target state.pc = target_pc.to_offset_int(); return Ok(InsnFunctionStepResult::Step); @@ -2738,29 +2709,15 @@ pub fn op_seek( _ => unreachable!(), } } else { - println!("Using rowid: {}", int_key); int_key }; let mut cursor = state.get_cursor(*cursor_id); let cursor = cursor.as_btree_mut(); - println!( - "About to call cursor.seek with rowid={}, actual_op={:?}", - rowid, actual_op - ); let found = return_if_io!(cursor.seek(SeekKey::TableRowId(rowid), actual_op)); - println!("cursor.seek returned found={}", found); if !found { - println!( - "Not found, jumping to target_pc={}", - target_pc.to_offset_int() - ); target_pc.to_offset_int() } else { - println!( - "Not found, jumping to target_pc={}", - target_pc.to_offset_int() - ); state.pc + 1 } }; diff --git a/tests/integration/fuzz/mod.rs b/tests/integration/fuzz/mod.rs index 789e39907..d7d5793d3 100644 --- a/tests/integration/fuzz/mod.rs +++ b/tests/integration/fuzz/mod.rs @@ -88,7 +88,6 @@ mod tests { ); log::trace!("query: {}", query); - println!("query: {}", query); let limbo_result = limbo_exec_rows(&db, &limbo_conn, &query); let sqlite_result = sqlite_exec_rows(&sqlite_conn, &query); assert_eq!( From 5a1da026e6dfc002dbdfcb6a67232fc2def604f8 Mon Sep 17 00:00:00 2001 From: krishvishal Date: Sat, 7 Jun 2025 17:08:10 +0530 Subject: [PATCH 15/20] Unify comparison function to reduce code duplication --- core/vdbe/execute.rs | 859 ++++++++----------------------------------- core/vdbe/insn.rs | 12 +- 2 files changed, 165 insertions(+), 706 deletions(-) diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index 1d05f1bd7..efe276ca5 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -5,6 +5,7 @@ use crate::storage::database::FileMemoryStorage; 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::{ error::{LimboError, SQLITE_CONSTRAINT, SQLITE_CONSTRAINT_PRIMARYKEY}, @@ -531,38 +532,176 @@ pub fn op_not_null( Ok(InsnFunctionStepResult::Step) } -pub fn op_eq( +#[derive(Debug, Clone, Copy, PartialEq)] +enum ComparisonOp { + Eq, + Ne, + Lt, + Le, + Gt, + Ge, +} + +impl ComparisonOp { + fn compare(&self, lhs: &Value, rhs: &Value, collation: &CollationSeq) -> bool { + match (lhs, rhs) { + (Value::Text(lhs_text), Value::Text(rhs_text)) => { + let order = collation.compare_strings(lhs_text.as_str(), rhs_text.as_str()); + match self { + ComparisonOp::Eq => order.is_eq(), + ComparisonOp::Ne => order.is_ne(), + ComparisonOp::Lt => order.is_lt(), + ComparisonOp::Le => order.is_le(), + ComparisonOp::Gt => order.is_gt(), + ComparisonOp::Ge => order.is_ge(), + } + } + (_, _) => match self { + ComparisonOp::Eq => *lhs == *rhs, + ComparisonOp::Ne => *lhs != *rhs, + ComparisonOp::Lt => *lhs < *rhs, + ComparisonOp::Le => *lhs <= *rhs, + ComparisonOp::Gt => *lhs > *rhs, + ComparisonOp::Ge => *lhs >= *rhs, + }, + } + } + + fn compare_integers(&self, lhs: &Value, rhs: &Value) -> bool { + match self { + ComparisonOp::Eq => lhs == rhs, + ComparisonOp::Ne => lhs != rhs, + ComparisonOp::Lt => lhs < rhs, + ComparisonOp::Le => lhs <= rhs, + ComparisonOp::Gt => lhs > rhs, + ComparisonOp::Ge => lhs >= rhs, + } + } + + fn handle_nulls(&self, lhs: &Value, rhs: &Value, null_eq: bool, jump_if_null: bool) -> bool { + match self { + ComparisonOp::Eq => { + let both_null = lhs == rhs; + (null_eq && both_null) || (!null_eq && jump_if_null) + } + ComparisonOp::Ne => { + let at_least_one_null = lhs != rhs; + (null_eq && at_least_one_null) || (!null_eq && jump_if_null) + } + ComparisonOp::Lt | ComparisonOp::Le | ComparisonOp::Gt | ComparisonOp::Ge => { + jump_if_null + } + } + } +} + +pub fn op_comparison( program: &Program, state: &mut ProgramState, insn: &Insn, pager: &Rc, mv_store: Option<&Rc>, ) -> Result { - let Insn::Eq { - lhs, - rhs, - target_pc, - flags, - collation, - } = insn - else { - unreachable!("unexpected Insn {:?}", insn) + let (lhs, rhs, target_pc, flags, collation, op) = match insn { + Insn::Eq { + lhs, + rhs, + target_pc, + flags, + collation, + } => ( + *lhs, + *rhs, + *target_pc, + *flags, + collation.unwrap_or_default(), + ComparisonOp::Eq, + ), + Insn::Ne { + lhs, + rhs, + target_pc, + flags, + collation, + } => ( + *lhs, + *rhs, + *target_pc, + *flags, + collation.unwrap_or_default(), + ComparisonOp::Ne, + ), + Insn::Lt { + lhs, + rhs, + target_pc, + flags, + collation, + } => ( + *lhs, + *rhs, + *target_pc, + *flags, + collation.unwrap_or_default(), + ComparisonOp::Lt, + ), + Insn::Le { + lhs, + rhs, + target_pc, + flags, + collation, + } => ( + *lhs, + *rhs, + *target_pc, + *flags, + collation.unwrap_or_default(), + ComparisonOp::Le, + ), + Insn::Gt { + lhs, + rhs, + target_pc, + flags, + collation, + } => ( + *lhs, + *rhs, + *target_pc, + *flags, + collation.unwrap_or_default(), + ComparisonOp::Gt, + ), + Insn::Ge { + lhs, + rhs, + target_pc, + flags, + collation, + } => ( + *lhs, + *rhs, + *target_pc, + *flags, + collation.unwrap_or_default(), + ComparisonOp::Ge, + ), + _ => unreachable!("unexpected Insn {:?}", insn), }; + assert!(target_pc.is_offset()); - let lhs = *lhs; - let rhs = *rhs; - let target_pc = *target_pc; + let nulleq = flags.has_nulleq(); let jump_if_null = flags.has_jump_if_null(); let affinity = flags.get_affinity(); - let collation = collation.unwrap_or_default(); let lhs_value = state.registers[lhs].get_owned_value(); let rhs_value = state.registers[rhs].get_owned_value(); // Fast path for integers if matches!(lhs_value, Value::Integer(_)) && matches!(rhs_value, Value::Integer(_)) { - if lhs_value == rhs_value { + if op.compare_integers(lhs_value, rhs_value) { state.pc = target_pc.to_offset_int(); } else { state.pc += 1; @@ -572,8 +711,7 @@ pub fn op_eq( // Handle NULL values if matches!(lhs_value, Value::Null) || matches!(rhs_value, Value::Null) { - let cond = lhs_value == rhs_value; // Both NULL - if (nulleq && cond) || (!nulleq && jump_if_null) { + if op.handle_nulls(lhs_value, rhs_value, nulleq, jump_if_null) { state.pc = target_pc.to_offset_int(); } else { state.pc += 1; @@ -587,6 +725,7 @@ pub fn op_eq( let mut lhs_converted = false; let mut rhs_converted = false; + // Apply affinity conversions match affinity { Affinity::Numeric | Affinity::Integer => { let lhs_is_text = matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)); @@ -640,696 +779,16 @@ pub fn op_eq( Affinity::Blob => {} // Do nothing for blob affinity. } - let should_jump = match ( + let should_jump = op.compare( lhs_temp_reg.get_owned_value(), rhs_temp_reg.get_owned_value(), - ) { - (Value::Text(lhs_text), Value::Text(rhs_text)) => { - let order = collation.compare_strings(lhs_text.as_str(), rhs_text.as_str()); - order.is_eq() - } - (lhs, rhs) => *lhs == *rhs, - }; + &collation, + ); if lhs_converted { state.registers[lhs] = lhs_temp_reg; } - if rhs_converted { - state.registers[rhs] = rhs_temp_reg; - } - if should_jump { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - - Ok(InsnFunctionStepResult::Step) -} - -pub fn op_ne( - program: &Program, - state: &mut ProgramState, - insn: &Insn, - pager: &Rc, - mv_store: Option<&Rc>, -) -> Result { - let Insn::Ne { - lhs, - rhs, - target_pc, - flags, - collation, - } = insn - else { - unreachable!("unexpected Insn {:?}", insn) - }; - assert!(target_pc.is_offset()); - let lhs = *lhs; - let rhs = *rhs; - let target_pc = *target_pc; - let nulleq = flags.has_nulleq(); - let jump_if_null = flags.has_jump_if_null(); - let affinity = flags.get_affinity(); - let collation = collation.unwrap_or_default(); - - let lhs_value = state.registers[lhs].get_owned_value(); - let rhs_value = state.registers[rhs].get_owned_value(); - - // Fast path for integers - if matches!(lhs_value, Value::Integer(_)) && matches!(rhs_value, Value::Integer(_)) { - if lhs_value != rhs_value { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - return Ok(InsnFunctionStepResult::Step); - } - - // Handle NULL values - if matches!(lhs_value, Value::Null) || matches!(rhs_value, Value::Null) { - let cond = lhs_value != rhs_value; // At least one NULL - if (nulleq && cond) || (!nulleq && jump_if_null) { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - return Ok(InsnFunctionStepResult::Step); - } - - let mut lhs_temp_reg = state.registers[lhs].clone(); - let mut rhs_temp_reg = state.registers[rhs].clone(); - - let mut lhs_converted = false; - let mut rhs_converted = false; - - match affinity { - Affinity::Numeric | Affinity::Integer => { - let lhs_is_text = matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)); - let rhs_is_text = matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)); - - if lhs_is_text || rhs_is_text { - if lhs_is_text { - lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); - } - if rhs_is_text { - rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); - } - } - } - - Affinity::Text => { - let lhs_is_text = matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)); - let rhs_is_text = matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)); - - if lhs_is_text || rhs_is_text { - if is_numeric_value(&lhs_temp_reg) { - lhs_converted = stringify_register(&mut lhs_temp_reg); - } - - if is_numeric_value(&rhs_temp_reg) { - rhs_converted = stringify_register(&mut rhs_temp_reg); - } - } - } - - Affinity::Real => { - if matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)) { - lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); - } - - if matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)) { - rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); - } - - if let Value::Integer(i) = lhs_temp_reg.get_owned_value() { - lhs_temp_reg = Register::Value(Value::Float(*i as f64)); - lhs_converted = true; - } - - if let Value::Integer(i) = rhs_temp_reg.get_owned_value() { - rhs_temp_reg = Register::Value(Value::Float(*i as f64)); - rhs_converted = true; - } - } - - Affinity::Blob => {} // Do nothing for blob affinity. - } - - let should_jump = match ( - lhs_temp_reg.get_owned_value(), - rhs_temp_reg.get_owned_value(), - ) { - (Value::Text(lhs_text), Value::Text(rhs_text)) => { - let order = collation.compare_strings(lhs_text.as_str(), rhs_text.as_str()); - order.is_ne() - } - (lhs, rhs) => *lhs != *rhs, - }; - - if lhs_converted { - state.registers[lhs] = lhs_temp_reg; - } - if rhs_converted { - state.registers[rhs] = rhs_temp_reg; - } - - if should_jump { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - - Ok(InsnFunctionStepResult::Step) -} - -pub fn op_lt( - program: &Program, - state: &mut ProgramState, - insn: &Insn, - pager: &Rc, - mv_store: Option<&Rc>, -) -> Result { - let Insn::Lt { - lhs, - rhs, - target_pc, - flags, - collation, - } = insn - else { - unreachable!("unexpected Insn {:?}", insn) - }; - assert!(target_pc.is_offset()); - let lhs = *lhs; - let rhs = *rhs; - let target_pc = *target_pc; - let jump_if_null = flags.has_jump_if_null(); - let affinity = flags.get_affinity(); - let collation = collation.unwrap_or_default(); - - let lhs_value = state.registers[lhs].get_owned_value(); - let rhs_value = state.registers[rhs].get_owned_value(); - - // Fast path for integers - if matches!(lhs_value, Value::Integer(_)) && matches!(rhs_value, Value::Integer(_)) { - if lhs_value < rhs_value { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - return Ok(InsnFunctionStepResult::Step); - } - - // Handle NULL values - if matches!(lhs_value, Value::Null) || matches!(rhs_value, Value::Null) { - if jump_if_null { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - return Ok(InsnFunctionStepResult::Step); - } - - let mut lhs_temp_reg = state.registers[lhs].clone(); - let mut rhs_temp_reg = state.registers[rhs].clone(); - - let mut lhs_converted = false; - let mut rhs_converted = false; - - match affinity { - Affinity::Numeric | Affinity::Integer => { - let lhs_is_text = matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)); - let rhs_is_text = matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)); - - if lhs_is_text || rhs_is_text { - if lhs_is_text { - lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); - } - if rhs_is_text { - rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); - } - } - } - - Affinity::Text => { - let lhs_is_text = matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)); - let rhs_is_text = matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)); - - if lhs_is_text || rhs_is_text { - if is_numeric_value(&lhs_temp_reg) { - lhs_converted = stringify_register(&mut lhs_temp_reg); - } - - if is_numeric_value(&rhs_temp_reg) { - rhs_converted = stringify_register(&mut rhs_temp_reg); - } - } - } - - Affinity::Real => { - if matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)) { - lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); - } - - if matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)) { - rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); - } - - if let Value::Integer(i) = lhs_temp_reg.get_owned_value() { - lhs_temp_reg = Register::Value(Value::Float(*i as f64)); - lhs_converted = true; - } - - if let Value::Integer(i) = rhs_temp_reg.get_owned_value() { - rhs_temp_reg = Register::Value(Value::Float(*i as f64)); - rhs_converted = true; - } - } - - Affinity::Blob => {} // Do nothing for blob affinity. - } - - let should_jump = match ( - lhs_temp_reg.get_owned_value(), - rhs_temp_reg.get_owned_value(), - ) { - (Value::Text(lhs_text), Value::Text(rhs_text)) => { - let order = collation.compare_strings(lhs_text.as_str(), rhs_text.as_str()); - order.is_lt() - } - (lhs, rhs) => *lhs < *rhs, - }; - - if lhs_converted { - state.registers[lhs] = lhs_temp_reg; - } - if rhs_converted { - state.registers[rhs] = rhs_temp_reg; - } - - if should_jump { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - - Ok(InsnFunctionStepResult::Step) -} - -pub fn op_le( - program: &Program, - state: &mut ProgramState, - insn: &Insn, - pager: &Rc, - mv_store: Option<&Rc>, -) -> Result { - let Insn::Le { - lhs, - rhs, - target_pc, - flags, - collation, - } = insn - else { - unreachable!("unexpected Insn {:?}", insn) - }; - assert!(target_pc.is_offset()); - let lhs = *lhs; - let rhs = *rhs; - let target_pc = *target_pc; - let jump_if_null = flags.has_jump_if_null(); - let affinity = flags.get_affinity(); - let collation = collation.unwrap_or_default(); - - let lhs_value = state.registers[lhs].get_owned_value(); - let rhs_value = state.registers[rhs].get_owned_value(); - - // Fast path for integers - if matches!(lhs_value, Value::Integer(_)) && matches!(rhs_value, Value::Integer(_)) { - if lhs_value <= rhs_value { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - return Ok(InsnFunctionStepResult::Step); - } - - // Handle NULL values - if matches!(lhs_value, Value::Null) || matches!(rhs_value, Value::Null) { - if jump_if_null { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - return Ok(InsnFunctionStepResult::Step); - } - - let mut lhs_temp_reg = state.registers[lhs].clone(); - let mut rhs_temp_reg = state.registers[rhs].clone(); - - let mut lhs_converted = false; - let mut rhs_converted = false; - - match affinity { - Affinity::Numeric | Affinity::Integer => { - let lhs_is_text = matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)); - let rhs_is_text = matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)); - - if lhs_is_text || rhs_is_text { - if lhs_is_text { - lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); - } - if rhs_is_text { - rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); - } - } - } - - Affinity::Text => { - let lhs_is_text = matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)); - let rhs_is_text = matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)); - - if lhs_is_text || rhs_is_text { - if is_numeric_value(&lhs_temp_reg) { - lhs_converted = stringify_register(&mut lhs_temp_reg); - } - - if is_numeric_value(&rhs_temp_reg) { - rhs_converted = stringify_register(&mut rhs_temp_reg); - } - } - } - - Affinity::Real => { - if matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)) { - lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); - } - - if matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)) { - rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); - } - - if let Value::Integer(i) = lhs_temp_reg.get_owned_value() { - lhs_temp_reg = Register::Value(Value::Float(*i as f64)); - lhs_converted = true; - } - - if let Value::Integer(i) = rhs_temp_reg.get_owned_value() { - rhs_temp_reg = Register::Value(Value::Float(*i as f64)); - rhs_converted = true; - } - } - - Affinity::Blob => {} // Do nothing for blob affinity. - } - - let should_jump = match ( - lhs_temp_reg.get_owned_value(), - rhs_temp_reg.get_owned_value(), - ) { - (Value::Text(lhs_text), Value::Text(rhs_text)) => { - let order = collation.compare_strings(lhs_text.as_str(), rhs_text.as_str()); - order.is_le() - } - (lhs, rhs) => *lhs <= *rhs, - }; - - if lhs_converted { - state.registers[lhs] = lhs_temp_reg; - } - if rhs_converted { - state.registers[rhs] = rhs_temp_reg; - } - - if should_jump { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - - Ok(InsnFunctionStepResult::Step) -} - -pub fn op_gt( - program: &Program, - state: &mut ProgramState, - insn: &Insn, - pager: &Rc, - mv_store: Option<&Rc>, -) -> Result { - let Insn::Gt { - lhs, - rhs, - target_pc, - flags, - collation, - } = insn - else { - unreachable!("unexpected Insn {:?}", insn) - }; - assert!(target_pc.is_offset()); - let lhs = *lhs; - let rhs = *rhs; - let target_pc = *target_pc; - let jump_if_null = flags.has_jump_if_null(); - let affinity = flags.get_affinity(); - let collation = collation.unwrap_or_default(); - - let lhs_value = state.registers[lhs].get_owned_value(); - let rhs_value = state.registers[rhs].get_owned_value(); - - // Fast path for integers - if matches!(lhs_value, Value::Integer(_)) && matches!(rhs_value, Value::Integer(_)) { - if lhs_value > rhs_value { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - return Ok(InsnFunctionStepResult::Step); - } - - // Handle NULL values - if matches!(lhs_value, Value::Null) || matches!(rhs_value, Value::Null) { - if jump_if_null { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - return Ok(InsnFunctionStepResult::Step); - } - - let mut lhs_temp_reg = state.registers[lhs].clone(); - let mut rhs_temp_reg = state.registers[rhs].clone(); - - let mut lhs_converted = false; - let mut rhs_converted = false; - - match affinity { - Affinity::Numeric | Affinity::Integer => { - let lhs_is_text = matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)); - let rhs_is_text = matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)); - - if lhs_is_text || rhs_is_text { - if lhs_is_text { - lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); - } - if rhs_is_text { - rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); - } - } - } - - Affinity::Text => { - let lhs_is_text = matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)); - let rhs_is_text = matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)); - - if lhs_is_text || rhs_is_text { - if is_numeric_value(&lhs_temp_reg) { - lhs_converted = stringify_register(&mut lhs_temp_reg); - } - - if is_numeric_value(&rhs_temp_reg) { - rhs_converted = stringify_register(&mut rhs_temp_reg); - } - } - } - - Affinity::Real => { - if matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)) { - lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); - } - if matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)) { - rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); - } - - if let Value::Integer(i) = lhs_temp_reg.get_owned_value() { - lhs_temp_reg = Register::Value(Value::Float(*i as f64)); - lhs_converted = true; - } - - if let Value::Integer(i) = rhs_temp_reg.get_owned_value() { - rhs_temp_reg = Register::Value(Value::Float(*i as f64)); - rhs_converted = true; - } - } - - Affinity::Blob => {} // Do nothing for blob affinity. - } - - let should_jump = match ( - lhs_temp_reg.get_owned_value(), - rhs_temp_reg.get_owned_value(), - ) { - (Value::Text(lhs_text), Value::Text(rhs_text)) => { - let order = collation.compare_strings(lhs_text.as_str(), rhs_text.as_str()); - order.is_gt() - } - (lhs, rhs) => *lhs > *rhs, - }; - - if lhs_converted { - state.registers[lhs] = lhs_temp_reg; - } - if rhs_converted { - state.registers[rhs] = rhs_temp_reg; - } - - if should_jump { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - Ok(InsnFunctionStepResult::Step) -} - -pub fn op_ge( - program: &Program, - state: &mut ProgramState, - insn: &Insn, - pager: &Rc, - mv_store: Option<&Rc>, -) -> Result { - let Insn::Ge { - lhs, - rhs, - target_pc, - flags, - collation, - } = insn - else { - unreachable!("unexpected Insn: {:?}", insn) - }; - - assert!(target_pc.is_offset()); - - let lhs = *lhs; - let rhs = *rhs; - let target_pc = *target_pc; - let jump_if_null = flags.has_jump_if_null(); - let affinity = flags.get_affinity(); - let collation = collation.unwrap_or_default(); - - let lhs_value = state.registers[lhs].get_owned_value(); - let rhs_value = state.registers[rhs].get_owned_value(); - - if matches!(lhs_value, Value::Integer(_)) && matches!(rhs_value, Value::Integer(_)) { - if lhs_value >= rhs_value { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - return Ok(InsnFunctionStepResult::Step); - } - - if matches!(lhs_value, Value::Null) || matches!(rhs_value, Value::Null) { - if jump_if_null { - state.pc = target_pc.to_offset_int(); - } else { - state.pc += 1; - } - return Ok(InsnFunctionStepResult::Step); - } - - let mut lhs_temp_reg = state.registers[lhs].clone(); - let mut rhs_temp_reg = state.registers[rhs].clone(); - - let mut lhs_converted = false; - let mut rhs_converted = false; - - match affinity { - Affinity::Numeric | Affinity::Integer => { - let lhs_is_text = matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)); - let rhs_is_text = matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)); - - if lhs_is_text || rhs_is_text { - if lhs_is_text { - lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); - } - if rhs_is_text { - rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); - } - } - } - - Affinity::Text => { - let lhs_is_text = matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)); - let rhs_is_text = matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)); - - if lhs_is_text || rhs_is_text { - if is_numeric_value(&lhs_temp_reg) { - lhs_converted = stringify_register(&mut lhs_temp_reg); - } - - if is_numeric_value(&rhs_temp_reg) { - rhs_converted = stringify_register(&mut rhs_temp_reg); - } - } - } - - Affinity::Real => { - if matches!(lhs_temp_reg.get_owned_value(), Value::Text(_)) { - lhs_converted = apply_numeric_affinity(&mut lhs_temp_reg, false); - } - if matches!(rhs_temp_reg.get_owned_value(), Value::Text(_)) { - rhs_converted = apply_numeric_affinity(&mut rhs_temp_reg, false); - } - - if let Value::Integer(i) = lhs_temp_reg.get_owned_value() { - lhs_temp_reg = Register::Value(Value::Float(*i as f64)); - lhs_converted = true; - } - - if let Value::Integer(i) = rhs_temp_reg.get_owned_value() { - rhs_temp_reg = Register::Value(Value::Float(*i as f64)); - rhs_converted = true; - } - } - - Affinity::Blob => {} // Do nothing for blob affinity. - } - - let should_jump = match ( - lhs_temp_reg.get_owned_value(), - rhs_temp_reg.get_owned_value(), - ) { - (Value::Text(lhs_text), Value::Text(rhs_text)) => { - let order = collation.compare_strings(lhs_text.as_str(), rhs_text.as_str()); - order.is_ge() - } - - (lhs, rhs) => { - if *lhs >= *rhs { - true - } else { - false - } - } - }; - - if lhs_converted { - state.registers[lhs] = lhs_temp_reg; - } if rhs_converted { state.registers[rhs] = rhs_temp_reg; } diff --git a/core/vdbe/insn.rs b/core/vdbe/insn.rs index 11632e96f..59f875423 100644 --- a/core/vdbe/insn.rs +++ b/core/vdbe/insn.rs @@ -951,12 +951,12 @@ impl Insn { Insn::Move { .. } => execute::op_move, Insn::IfPos { .. } => execute::op_if_pos, Insn::NotNull { .. } => execute::op_not_null, - Insn::Eq { .. } => execute::op_eq, - Insn::Ne { .. } => execute::op_ne, - Insn::Lt { .. } => execute::op_lt, - Insn::Le { .. } => execute::op_le, - Insn::Gt { .. } => execute::op_gt, - Insn::Ge { .. } => execute::op_ge, + Insn::Eq { .. } + | Insn::Ne { .. } + | Insn::Lt { .. } + | Insn::Le { .. } + | Insn::Gt { .. } + | Insn::Ge { .. } => execute::op_comparison, Insn::If { .. } => execute::op_if, Insn::IfNot { .. } => execute::op_if_not, Insn::OpenRead { .. } => execute::op_open_read, From c8da564aebf28eabdc56735182f2632c9a14bcfe Mon Sep 17 00:00:00 2001 From: krishvishal Date: Sat, 7 Jun 2025 17:12:20 +0530 Subject: [PATCH 16/20] smol edit --- core/vdbe/insn.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/vdbe/insn.rs b/core/vdbe/insn.rs index 59f875423..db319f936 100644 --- a/core/vdbe/insn.rs +++ b/core/vdbe/insn.rs @@ -992,10 +992,10 @@ impl Insn { Insn::IdxRowId { .. } => execute::op_idx_row_id, Insn::SeekRowid { .. } => execute::op_seek_rowid, Insn::DeferredSeek { .. } => execute::op_deferred_seek, - Insn::SeekGE { .. } => execute::op_seek, - Insn::SeekGT { .. } => execute::op_seek, - Insn::SeekLE { .. } => execute::op_seek, - Insn::SeekLT { .. } => execute::op_seek, + Insn::SeekGE { .. } + | Insn::SeekGT { .. } + | Insn::SeekLE { .. } + | Insn::SeekLT { .. } => execute::op_seek, Insn::SeekEnd { .. } => execute::op_seek_end, Insn::IdxGE { .. } => execute::op_idx_ge, Insn::IdxGT { .. } => execute::op_idx_gt, From 712c94537ca8ecead288f19f10f0745c6b798198 Mon Sep 17 00:00:00 2001 From: Krishna Vishal Date: Sun, 8 Jun 2025 12:08:48 +0530 Subject: [PATCH 17/20] Add affinity flags to `IS` and `IS NOT` opeartors --- core/translate/expr.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/translate/expr.rs b/core/translate/expr.rs index a015875fa..5d081d2a9 100644 --- a/core/translate/expr.rs +++ b/core/translate/expr.rs @@ -2425,7 +2425,7 @@ fn emit_binary_insn( lhs, rhs, target_pc: if_true_label, - flags: CmpInsFlags::default().null_eq(), + flags: CmpInsFlags::default().null_eq().with_affinity(affinity), collation: program.curr_collation(), }, target_register, @@ -2440,7 +2440,7 @@ fn emit_binary_insn( lhs, rhs, target_pc: if_true_label, - flags: CmpInsFlags::default().null_eq(), + flags: CmpInsFlags::default().null_eq().with_affinity(affinity), collation: program.curr_collation(), }, target_register, From 0d5cbc4f1d25589f9db24a3a4320b96e27f8a59b Mon Sep 17 00:00:00 2001 From: Krishna Vishal Date: Sun, 8 Jun 2025 22:00:48 +0530 Subject: [PATCH 18/20] Add affinity check as a function as `ast::Operator` impl --- core/translate/expr.rs | 13 ++----------- vendored/sqlite3-parser/src/parser/ast/mod.rs | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/core/translate/expr.rs b/core/translate/expr.rs index 5d081d2a9..c1beb32d2 100644 --- a/core/translate/expr.rs +++ b/core/translate/expr.rs @@ -2224,19 +2224,10 @@ fn emit_binary_insn( referenced_tables: Option<&TableReferences>, ) -> Result<()> { let mut affinity = Affinity::Blob; - if matches!( - op, - ast::Operator::Equals - | ast::Operator::NotEquals - | ast::Operator::Less - | ast::Operator::LessEquals - | ast::Operator::Greater - | ast::Operator::GreaterEquals - | ast::Operator::Is - | ast::Operator::IsNot - ) { + if op.is_comparison() { affinity = comparison_affinity(lhs_expr, rhs_expr, referenced_tables); } + match op { ast::Operator::NotEquals => { let if_true_label = program.allocate_label(); diff --git a/vendored/sqlite3-parser/src/parser/ast/mod.rs b/vendored/sqlite3-parser/src/parser/ast/mod.rs index 7512b75d5..6d92c07ba 100644 --- a/vendored/sqlite3-parser/src/parser/ast/mod.rs +++ b/vendored/sqlite3-parser/src/parser/ast/mod.rs @@ -731,6 +731,21 @@ impl Operator { | Operator::NotEquals ) } + + /// Returns true if this operator is a comparison operator that may need affinity conversion + pub fn is_comparison(&self) -> bool { + matches!( + self, + Self::Equals + | Self::NotEquals + | Self::Less + | Self::LessEquals + | Self::Greater + | Self::GreaterEquals + | Self::Is + | Self::IsNot + ) + } } /// Unary operators From 7db6e2dfeae03899fbda77e5db4aa338ae67bb56 Mon Sep 17 00:00:00 2001 From: Krishna Vishal Date: Wed, 11 Jun 2025 00:12:35 +0530 Subject: [PATCH 19/20] Decrease db rows and increase random values --- tests/integration/fuzz/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/fuzz/mod.rs b/tests/integration/fuzz/mod.rs index d7d5793d3..55b8c3a25 100644 --- a/tests/integration/fuzz/mod.rs +++ b/tests/integration/fuzz/mod.rs @@ -51,7 +51,7 @@ mod tests { let insert = format!( "INSERT INTO t VALUES {}", - (1..2000) + (1..100) .map(|x| format!("({})", x)) .collect::>() .join(", ") @@ -104,7 +104,7 @@ mod tests { fn generate_random_comparison_values(rng: &mut ChaCha8Rng) -> Vec { let mut values = Vec::new(); - for _ in 0..5 { + for _ in 0..1000 { let val = rng.random_range(-10000..10000); values.push(val.to_string()); } From 1c6a65ded4dfff64e8bdba8503ad25809e870432 Mon Sep 17 00:00:00 2001 From: Krishna Vishal Date: Wed, 11 Jun 2025 00:44:07 +0530 Subject: [PATCH 20/20] Change seek op match from unit variants to struct variants. --- core/vdbe/execute.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index efe276ca5..ae0fe12ea 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -2112,15 +2112,15 @@ pub fn op_seek( if c > 0 { // If approximation is larger than actual search term match op { - SeekOp::GT => SeekOp::GE, // (x > 4.9) -> (x >= 5) - SeekOp::LE => SeekOp::LT, // (x <= 4.9) -> (x < 5) + SeekOp::GT => SeekOp::GE { eq_only: false }, // (x > 4.9) -> (x >= 5) + SeekOp::LE { .. } => SeekOp::LT, // (x <= 4.9) -> (x < 5) other => other, } } else if c < 0 { // If approximation is smaller than actual search term match op { - SeekOp::LT => SeekOp::LE, // (x < 5.1) -> (x <= 5) - SeekOp::GE => SeekOp::GT, // (x >= 5.1) -> (x > 5) + SeekOp::LT => SeekOp::LE { eq_only: false }, // (x < 5.1) -> (x <= 5) + SeekOp::GE { .. } => SeekOp::GT, // (x >= 5.1) -> (x > 5) other => other, } } else { @@ -2129,12 +2129,12 @@ pub fn op_seek( } Value::Text(_) | Value::Blob(_) => { match op { - SeekOp::GT | SeekOp::GE => { + SeekOp::GT | SeekOp::GE { .. } => { // No integers are > or >= non-numeric text, jump to target (empty result) state.pc = target_pc.to_offset_int(); return Ok(InsnFunctionStepResult::Step); } - SeekOp::LT | SeekOp::LE => { + SeekOp::LT | SeekOp::LE { .. } => { // All integers are < or <= non-numeric text // Move to last position and then use the normal seek logic { @@ -2145,7 +2145,6 @@ pub fn op_seek( state.pc += 1; return Ok(InsnFunctionStepResult::Step); } - _ => unreachable!(), } } _ => op, @@ -2156,16 +2155,15 @@ pub fn op_seek( let rowid = if matches!(original_value, Value::Null) { match actual_op { - SeekOp::GE | SeekOp::GT => { + SeekOp::GE { .. } | SeekOp::GT => { state.pc = target_pc.to_offset_int(); return Ok(InsnFunctionStepResult::Step); } - SeekOp::LE | SeekOp::LT => { + SeekOp::LE { .. } | SeekOp::LT => { // No integers are < NULL, so jump to target state.pc = target_pc.to_offset_int(); return Ok(InsnFunctionStepResult::Step); } - _ => unreachable!(), } } else { int_key