diff --git a/.cargo/config.toml b/.cargo/config.toml new file mode 100644 index 000000000..2586c8988 --- /dev/null +++ b/.cargo/config.toml @@ -0,0 +1,2 @@ +[env] +LIBSQLITE3_FLAGS = "-DSQLITE_ENABLE_MATH_FUNCTIONS" # necessary for rusqlite dependency in order to bundle SQLite with math functions included diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 98a4851db..0e9c1c36d 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -27,6 +27,8 @@ jobs: steps: - uses: actions/checkout@v3 - uses: Swatinem/rust-cache@v2 + with: + prefix-key: "v1-rust" # can be updated if we need to reset caches due to non-trivial change in the dependencies (for example, custom env var were set for single workspace project) - name: Set up Python 3.10 uses: actions/setup-python@v5 with: diff --git a/core/mvcc/database/mod.rs b/core/mvcc/database/mod.rs index 4ae85f195..ca0425ecb 100644 --- a/core/mvcc/database/mod.rs +++ b/core/mvcc/database/mod.rs @@ -318,8 +318,9 @@ impl MvStore MvStore MvStore( } } -pub(crate) fn is_version_visible( - txs: &SkipMap>, - tx: &Transaction, - rv: &RowVersion, -) -> bool { - is_begin_visible(txs, tx, rv) && is_end_visible(txs, tx, rv) +impl RowVersion { + pub fn is_visible_to( + &self, + tx: &Transaction, + txs: &SkipMap>, + ) -> bool { + is_begin_visible(txs, tx, self) && is_end_visible(txs, tx, self) + } } fn is_begin_visible( diff --git a/core/mvcc/database/tests.rs b/core/mvcc/database/tests.rs index b317a15d2..f3dcabca9 100644 --- a/core/mvcc/database/tests.rs +++ b/core/mvcc/database/tests.rs @@ -699,7 +699,7 @@ fn test_snapshot_isolation_tx_visible1() { }, }; tracing::debug!("Testing visibility of {row_version:?}"); - is_version_visible(&txs, ¤t_tx, &row_version) + row_version.is_visible_to(¤t_tx, &txs) }; // begin visible: transaction committed with ts < current_tx.begin_ts diff --git a/core/translate/expr.rs b/core/translate/expr.rs index cff8f0c85..6553eecfe 100644 --- a/core/translate/expr.rs +++ b/core/translate/expr.rs @@ -1849,15 +1849,24 @@ pub fn translate_expr( MathFuncArity::Binary => { let args = expect_arguments_exact!(args, 2, math_func); - let reg1 = program.alloc_register(); - let _ = - translate_expr(program, referenced_tables, &args[0], reg1, resolver)?; - let reg2 = program.alloc_register(); - let _ = - translate_expr(program, referenced_tables, &args[1], reg2, resolver)?; + let start_reg = program.alloc_registers(2); + let _ = translate_expr( + program, + referenced_tables, + &args[0], + start_reg, + resolver, + )?; + let _ = translate_expr( + program, + referenced_tables, + &args[1], + start_reg + 1, + resolver, + )?; program.emit_insn(Insn::Function { constant_mask: 0, - start_reg: target_register + 1, + start_reg, dest: target_register, func: func_ctx, }); diff --git a/core/vdbe/mod.rs b/core/vdbe/mod.rs index d86a488bd..a81a1c687 100644 --- a/core/vdbe/mod.rs +++ b/core/vdbe/mod.rs @@ -1399,8 +1399,8 @@ impl Program { let record_from_regs: Record = make_owned_record(&state.registers, start_reg, num_regs); if let Some(ref idx_record) = *cursor.record()? { - // omit the rowid from the idx_record, which is the last value - if idx_record.get_values()[..idx_record.len() - 1] + // Compare against the same number of values + if idx_record.get_values()[..record_from_regs.len()] >= record_from_regs.get_values()[..] { state.pc = target_pc.to_offset_int(); @@ -1423,8 +1423,8 @@ impl Program { let record_from_regs: Record = make_owned_record(&state.registers, start_reg, num_regs); if let Some(ref idx_record) = *cursor.record()? { - // omit the rowid from the idx_record, which is the last value - if idx_record.get_values()[..idx_record.len() - 1] + // Compare against the same number of values + if idx_record.get_values()[..record_from_regs.len()] <= record_from_regs.get_values()[..] { state.pc = target_pc.to_offset_int(); @@ -1447,8 +1447,8 @@ impl Program { let record_from_regs: Record = make_owned_record(&state.registers, start_reg, num_regs); if let Some(ref idx_record) = *cursor.record()? { - // omit the rowid from the idx_record, which is the last value - if idx_record.get_values()[..idx_record.len() - 1] + // Compare against the same number of values + if idx_record.get_values()[..record_from_regs.len()] > record_from_regs.get_values()[..] { state.pc = target_pc.to_offset_int(); @@ -1471,8 +1471,8 @@ impl Program { let record_from_regs: Record = make_owned_record(&state.registers, start_reg, num_regs); if let Some(ref idx_record) = *cursor.record()? { - // omit the rowid from the idx_record, which is the last value - if idx_record.get_values()[..idx_record.len() - 1] + // Compare against the same number of values + if idx_record.get_values()[..record_from_regs.len()] < record_from_regs.get_values()[..] { state.pc = target_pc.to_offset_int(); @@ -2250,7 +2250,11 @@ impl Program { ScalarFunc::Substr | ScalarFunc::Substring => { let str_value = &state.registers[*start_reg]; let start_value = &state.registers[*start_reg + 1]; - let length_value = &state.registers[*start_reg + 2]; + let length_value = if func.arg_count == 3 { + Some(&state.registers[*start_reg + 2]) + } else { + None + }; let result = exec_substring(str_value, start_value, length_value); state.registers[*dest] = result; } @@ -3286,39 +3290,31 @@ fn exec_nullif(first_value: &OwnedValue, second_value: &OwnedValue) -> OwnedValu fn exec_substring( str_value: &OwnedValue, start_value: &OwnedValue, - length_value: &OwnedValue, + length_value: Option<&OwnedValue>, ) -> OwnedValue { - if let (OwnedValue::Text(str), OwnedValue::Integer(start), OwnedValue::Integer(length)) = - (str_value, start_value, length_value) - { - let start = *start as usize; - let str_len = str.as_str().len(); + if let (OwnedValue::Text(str), OwnedValue::Integer(start)) = (str_value, start_value) { + let str_len = str.as_str().len() as i64; - if start > str_len { - return OwnedValue::build_text(""); - } - - let start_idx = start - 1; - let end = if *length != -1 { - start_idx + *length as usize + // The left-most character of X is number 1. + // If Y is negative then the first character of the substring is found by counting from the right rather than the left. + let first_position = if *start < 0 { + str_len.saturating_sub((*start).abs()) } else { - str_len + *start - 1 }; - let substring = &str.as_str()[start_idx..end.min(str_len)]; - - OwnedValue::build_text(substring) - } else if let (OwnedValue::Text(str), OwnedValue::Integer(start)) = (str_value, start_value) { - let start = *start as usize; - let str_len = str.as_str().len(); - - if start > str_len { - return OwnedValue::build_text(""); - } - - let start_idx = start - 1; - let substring = &str.as_str()[start_idx..str_len]; - - OwnedValue::build_text(substring) + // If Z is negative then the abs(Z) characters preceding the Y-th character are returned. + let last_position = match length_value { + Some(OwnedValue::Integer(length)) => first_position + *length, + _ => str_len, + }; + let (start, end) = if first_position <= last_position { + (first_position, last_position) + } else { + (last_position, first_position) + }; + OwnedValue::build_text( + &str.as_str()[start.clamp(-0, str_len) as usize..end.clamp(0, str_len) as usize], + ) } else { OwnedValue::Null } @@ -4393,7 +4389,7 @@ mod tests { let length_value = OwnedValue::Integer(3); let expected_val = OwnedValue::build_text("lim"); assert_eq!( - exec_substring(&str_value, &start_value, &length_value), + exec_substring(&str_value, &start_value, Some(&length_value)), expected_val ); @@ -4402,7 +4398,7 @@ mod tests { let length_value = OwnedValue::Integer(10); let expected_val = OwnedValue::build_text("limbo"); assert_eq!( - exec_substring(&str_value, &start_value, &length_value), + exec_substring(&str_value, &start_value, Some(&length_value)), expected_val ); @@ -4411,7 +4407,7 @@ mod tests { let length_value = OwnedValue::Integer(3); let expected_val = OwnedValue::build_text(""); assert_eq!( - exec_substring(&str_value, &start_value, &length_value), + exec_substring(&str_value, &start_value, Some(&length_value)), expected_val ); @@ -4420,7 +4416,7 @@ mod tests { let length_value = OwnedValue::Null; let expected_val = OwnedValue::build_text("mbo"); assert_eq!( - exec_substring(&str_value, &start_value, &length_value), + exec_substring(&str_value, &start_value, Some(&length_value)), expected_val ); @@ -4429,7 +4425,7 @@ mod tests { let length_value = OwnedValue::Null; let expected_val = OwnedValue::build_text(""); assert_eq!( - exec_substring(&str_value, &start_value, &length_value), + exec_substring(&str_value, &start_value, Some(&length_value)), expected_val ); } diff --git a/testing/scalar-functions.test b/testing/scalar-functions.test index 47cca09d4..2ba0d58a4 100755 --- a/testing/scalar-functions.test +++ b/testing/scalar-functions.test @@ -535,6 +535,21 @@ do_execsql_test substr-2-args { SELECT substr('limbo', 3); } {mbo} +do_execsql_test substr-cases { + SELECT substr('limbo', 0); + SELECT substr('limbo', 0, 3); + SELECT substr('limbo', -2); + SELECT substr('limbo', -2, 1); + SELECT substr('limbo', -10, 7); + SELECT substr('limbo', 10, -7); +} {limbo +li +bo +b +li +mbo +} + do_execsql_test substring-3-args { SELECT substring('limbo', 1, 3); } {lim} diff --git a/tests/integration/fuzz/mod.rs b/tests/integration/fuzz/mod.rs index b6d363af4..010d9cb4e 100644 --- a/tests/integration/fuzz/mod.rs +++ b/tests/integration/fuzz/mod.rs @@ -167,7 +167,7 @@ mod tests { } #[test] - pub fn logical_expression_fuzz_ex1() { + pub fn fuzz_ex() { let _ = env_logger::try_init(); let db = TempDatabase::new_empty(); let limbo_conn = db.connect_limbo(); @@ -182,6 +182,7 @@ mod tests { "SELECT CASE ( NULL < NULL ) WHEN ( 0 ) THEN ( NULL ) ELSE ( 2.0 ) END;", "SELECT (COALESCE(0, COALESCE(0, 0)));", "SELECT CAST((1 > 0) AS INTEGER);", + "SELECT substr('ABC', -1)", ] { let limbo = limbo_exec_rows(&db, &limbo_conn, query); let sqlite = sqlite_exec_rows(&sqlite_conn, query); @@ -193,6 +194,111 @@ mod tests { } } + #[test] + pub fn math_expression_fuzz_run() { + let _ = env_logger::try_init(); + let g = GrammarGenerator::new(); + let (expr, expr_builder) = g.create_handle(); + let (bin_op, bin_op_builder) = g.create_handle(); + let (scalar, scalar_builder) = g.create_handle(); + let (paren, paren_builder) = g.create_handle(); + + paren_builder + .concat("") + .push_str("(") + .push(expr) + .push_str(")") + .build(); + + bin_op_builder + .concat(" ") + .push(expr) + .push( + g.create() + .choice() + .options_str(["+", "-", "/", "*"]) + .build(), + ) + .push(expr) + .build(); + + scalar_builder + .choice() + .option( + g.create() + .concat("") + .push( + g.create() + .choice() + .options_str([ + "acos", "acosh", "asin", "asinh", "atan", "atanh", "ceil", + "ceiling", "cos", "cosh", "degrees", "exp", "floor", "ln", "log", + "log10", "log2", "radians", "sin", "sinh", "sqrt", "tan", "tanh", + "trunc", + ]) + .build(), + ) + .push_str("(") + .push(expr) + .push_str(")") + .build(), + ) + .option( + g.create() + .concat("") + .push( + g.create() + .choice() + .options_str(["atan2", "log", "mod", "pow", "power"]) + .build(), + ) + .push_str("(") + .push(g.create().concat("").push(expr).repeat(2..3, ", ").build()) + .push_str(")") + .build(), + ) + .build(); + + expr_builder + .choice() + .options_str(["-2.0", "-1.0", "0.0", "0.5", "1.0", "2.0"]) + .option_w(bin_op, 10.0) + .option_w(paren, 10.0) + .option_w(scalar, 10.0) + .build(); + + let sql = g.create().concat(" ").push_str("SELECT").push(expr).build(); + + let db = TempDatabase::new_empty(); + let limbo_conn = db.connect_limbo(); + let sqlite_conn = rusqlite::Connection::open_in_memory().unwrap(); + + let (mut rng, seed) = rng_from_time(); + log::info!("seed: {}", seed); + for _ in 0..1024 { + let query = g.generate(&mut rng, sql, 50); + log::info!("query: {}", query); + let limbo = limbo_exec_row(&limbo_conn, &query); + let sqlite = sqlite_exec_row(&sqlite_conn, &query); + match (&limbo[0], &sqlite[0]) { + // compare only finite results because some evaluations are not so stable around infinity + (rusqlite::types::Value::Real(limbo), rusqlite::types::Value::Real(sqlite)) + if limbo.is_finite() && sqlite.is_finite() => + { + assert!( + (limbo - sqlite).abs() < 1e-9 + || (limbo - sqlite) / (limbo.abs().max(sqlite.abs())) < 1e-9, + "query: {}, limbo: {:?}, sqlite: {:?}", + query, + limbo, + sqlite + ) + } + _ => {} + } + } + } + #[test] pub fn string_expression_fuzz_run() { let _ = env_logger::try_init(); @@ -201,6 +307,20 @@ mod tests { let (bin_op, bin_op_builder) = g.create_handle(); let (scalar, scalar_builder) = g.create_handle(); let (paren, paren_builder) = g.create_handle(); + let (number, number_builder) = g.create_handle(); + + number_builder + .choice() + .option_symbol(rand_int(-5..10)) + .option( + g.create() + .concat(" ") + .push(number) + .push(g.create().choice().options_str(["+", "-", "*"]).build()) + .push(number) + .build(), + ) + .build(); paren_builder .concat("") @@ -262,6 +382,37 @@ mod tests { .push_str(")") .build(), ) + .option( + g.create() + .concat("") + .push(g.create().choice().options_str(["replace"]).build()) + .push_str("(") + .push(g.create().concat("").push(expr).repeat(3..4, ", ").build()) + .push_str(")") + .build(), + ) + .option( + g.create() + .concat("") + .push( + g.create() + .choice() + .options_str(["substr", "substring"]) + .build(), + ) + .push_str("(") + .push(expr) + .push_str(", ") + .push( + g.create() + .concat("") + .push(number) + .repeat(1..3, ", ") + .build(), + ) + .push_str(")") + .build(), + ) .build(); expr_builder