aljaz/turso
1
0
Fork 0
mirror of https://github.com/aljazceru/turso.git synced 2025-12-29 05:54:21 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge 'translate_condition_expr(): fix cases where 1. we jump on false and 2. either operand is NULL' from Jussi Saurio

Browse Source 
Change explanation is in the code comment for `Insn::Eq`:
```
        /// Jump if either of the operands is null. Used for "jump when false" logic.
        /// Eg. "SELECT * FROM users WHERE id = NULL" becomes:
        /// <JUMP TO NEXT ROW IF id != NULL>
        /// Without the jump_if_null flag it would not jump because the logical comparison "id != NULL" is never true.
        /// This flag indicates that if either is null we should still jump.
        jump_if_null: bool,
```
Closes #754
Excerpt from SQLite bytecode documentation for e.g. `Lt`:
> If the SQLITE_JUMPIFNULL bit of P5 is set and either reg(P1) or
reg(P3) is NULL then the take the jump. If the SQLITE_JUMPIFNULL bit is
clear then fall through if either operand is NULL.
I didn't want to start putting these flags into a bitmask so I just
added a separate boolean. Probably for sqlite `EXPLAIN` compatibility we
should, IF we want to be exactly compatible (which we aren't anyway atm)

Closes #755
This commit is contained in:
Pekka Enberg
2025-01-20 19:40:08 +02:00
parent 2cea85c13b ce15ad7d32
commit c27427d644
8 changed files with 140 additions and 51 deletions
Download Patch File Download Diff File Expand all files Collapse all files

26
core/translate/expr.rs
View File

@@ -23,13 +23,13 @@ fn emit_cond_jump(program: &mut ProgramBuilder, cond_meta: ConditionMetadata, re
program.emit_insn(Insn::If {
reg,
target_pc: cond_meta.jump_target_when_true,
null_reg: reg,
jump_if_null: false,
});
} else {
program.emit_insn(Insn::IfNot {
reg,
target_pc: cond_meta.jump_target_when_false,
null_reg: reg,
jump_if_null: true,
});
}
}
@@ -47,12 +47,14 @@ macro_rules! emit_cmp_insn {
lhs: $lhs,
rhs: $rhs,
target_pc: $cond.jump_target_when_true,
jump_if_null: false,
});
} else {
$program.emit_insn(Insn::$op_false {
lhs: $lhs,
rhs: $rhs,
target_pc: $cond.jump_target_when_false,
jump_if_null: true,
});
}
}};
@@ -324,6 +326,7 @@ pub fn translate_condition_expr(
lhs: lhs_reg,
rhs: rhs_reg,
target_pc: jump_target_when_true,
jump_if_null: false,
});
} else {
// If this is the last condition, we need to jump to the 'jump_target_when_false' label if there is no match.
@@ -331,6 +334,7 @@ pub fn translate_condition_expr(
lhs: lhs_reg,
rhs: rhs_reg,
target_pc: condition_metadata.jump_target_when_false,
jump_if_null: true,
});
}
}
@@ -351,6 +355,7 @@ pub fn translate_condition_expr(
lhs: lhs_reg,
rhs: rhs_reg,
target_pc: condition_metadata.jump_target_when_false,
jump_if_null: true,
});
}
// If we got here, then none of the conditions were a match, so we jump to the 'jump_target_when_true' label if 'jump_if_condition_is_true'.
@@ -414,13 +419,13 @@ pub fn translate_condition_expr(
program.emit_insn(Insn::IfNot {
reg: cur_reg,
target_pc: condition_metadata.jump_target_when_true,
null_reg: cur_reg,
jump_if_null: false,
});
} else {
program.emit_insn(Insn::If {
reg: cur_reg,
target_pc: condition_metadata.jump_target_when_false,
null_reg: cur_reg,
jump_if_null: true,
});
}
}
@@ -477,6 +482,7 @@ pub fn translate_expr(
lhs: e1_reg,
rhs: e2_reg,
target_pc: if_true_label,
jump_if_null: false,
},
target_register,
if_true_label,
@@ -492,6 +498,7 @@ pub fn translate_expr(
lhs: e1_reg,
rhs: e2_reg,
target_pc: if_true_label,
jump_if_null: false,
},
target_register,
if_true_label,
@@ -507,6 +514,7 @@ pub fn translate_expr(
lhs: e1_reg,
rhs: e2_reg,
target_pc: if_true_label,
jump_if_null: false,
},
target_register,
if_true_label,
@@ -522,6 +530,7 @@ pub fn translate_expr(
lhs: e1_reg,
rhs: e2_reg,
target_pc: if_true_label,
jump_if_null: false,
},
target_register,
if_true_label,
@@ -537,6 +546,7 @@ pub fn translate_expr(
lhs: e1_reg,
rhs: e2_reg,
target_pc: if_true_label,
jump_if_null: false,
},
target_register,
if_true_label,
@@ -552,6 +562,7 @@ pub fn translate_expr(
lhs: e1_reg,
rhs: e2_reg,
target_pc: if_true_label,
jump_if_null: false,
},
target_register,
if_true_label,
@@ -630,6 +641,7 @@ pub fn translate_expr(
lhs: e1_reg,
rhs: e2_reg,
target_pc: if_true_label,
jump_if_null: false,
},
target_register,
if_true_label,
@@ -643,6 +655,7 @@ pub fn translate_expr(
lhs: e1_reg,
rhs: e2_reg,
target_pc: if_true_label,
jump_if_null: false,
},
target_register,
if_true_label,
@@ -706,12 +719,13 @@ pub fn translate_expr(
lhs: base_reg,
rhs: expr_reg,
target_pc: next_case_label,
jump_if_null: false,
}),
// CASE WHEN 0 THEN 0 ELSE 1 becomes ifnot 0 branch to next clause
None => program.emit_insn(Insn::IfNot {
reg: expr_reg,
target_pc: next_case_label,
null_reg: 1,
jump_if_null: true,
}),
};
// THEN...
@@ -1065,7 +1079,7 @@ pub fn translate_expr(
program.emit_insn(Insn::IfNot {
reg: temp_reg,
target_pc: jump_target_when_false,
null_reg: 1,
jump_if_null: true,
});
translate_expr(
program,

2
core/translate/group_by.rs
View File

@@ -288,7 +288,7 @@ pub fn emit_group_by<'a>(
program.emit_insn(Insn::If {
target_pc: label_acc_indicator_set_flag_true,
reg: reg_data_in_acc_flag,
null_reg: 0, // unused in this case
jump_if_null: false,
});
// Read the group by columns for a finished group

2
core/translate/main_loop.rs
View File

@@ -477,6 +477,7 @@ pub fn open_loop(
lhs: rowid_reg,
rhs: cmp_reg,
target_pc: loop_end,
jump_if_null: false,
});
}
}
@@ -498,6 +499,7 @@ pub fn open_loop(
lhs: rowid_reg,
rhs: cmp_reg,
target_pc: loop_end,
jump_if_null: false,
});
}
}

10
core/vdbe/builder.rs
View File

@@ -158,6 +158,7 @@ impl ProgramBuilder {
lhs: _lhs,
rhs: _rhs,
target_pc,
..
} => {
resolve(target_pc, "Eq");
}
@@ -165,6 +166,7 @@ impl ProgramBuilder {
lhs: _lhs,
rhs: _rhs,
target_pc,
..
} => {
resolve(target_pc, "Ne");
}
@@ -172,6 +174,7 @@ impl ProgramBuilder {
lhs: _lhs,
rhs: _rhs,
target_pc,
..
} => {
resolve(target_pc, "Lt");
}
@@ -179,6 +182,7 @@ impl ProgramBuilder {
lhs: _lhs,
rhs: _rhs,
target_pc,
..
} => {
resolve(target_pc, "Le");
}
@@ -186,6 +190,7 @@ impl ProgramBuilder {
lhs: _lhs,
rhs: _rhs,
target_pc,
..
} => {
resolve(target_pc, "Gt");
}
@@ -193,20 +198,21 @@ impl ProgramBuilder {
lhs: _lhs,
rhs: _rhs,
target_pc,
..
} => {
resolve(target_pc, "Ge");
}
Insn::If {
reg: _reg,
target_pc,
null_reg: _,
jump_if_null: _,
} => {
resolve(target_pc, "If");
}
Insn::IfNot {
reg: _reg,
target_pc,
null_reg: _,
jump_if_null: _,
} => {
resolve(target_pc, "IfNot");
}

14
core/vdbe/explain.rs
View File

@@ -209,6 +209,7 @@ pub fn insn_to_str(
lhs,
rhs,
target_pc,
..
} => (
"Eq",
*lhs as i32,
@@ -227,6 +228,7 @@ pub fn insn_to_str(
lhs,
rhs,
target_pc,
..
} => (
"Ne",
*lhs as i32,
@@ -245,6 +247,7 @@ pub fn insn_to_str(
lhs,
rhs,
target_pc,
..
} => (
"Lt",
*lhs as i32,
@@ -258,6 +261,7 @@ pub fn insn_to_str(
lhs,
rhs,
target_pc,
..
} => (
"Le",
*lhs as i32,
@@ -276,6 +280,7 @@ pub fn insn_to_str(
lhs,
rhs,
target_pc,
..
} => (
"Gt",
*lhs as i32,
@@ -289,6 +294,7 @@ pub fn insn_to_str(
lhs,
rhs,
target_pc,
..
} => (
"Ge",
*lhs as i32,
@@ -306,12 +312,12 @@ pub fn insn_to_str(
Insn::If {
reg,
target_pc,
null_reg,
jump_if_null,
} => (
"If",
*reg as i32,
target_pc.to_debug_int(),
*null_reg as i32,
*jump_if_null as i32,
OwnedValue::build_text(Rc::new("".to_string())),
0,
format!("if r[{}] goto {}", reg, target_pc.to_debug_int()),
@@ -319,12 +325,12 @@ pub fn insn_to_str(
Insn::IfNot {
reg,
target_pc,
null_reg,
jump_if_null,
} => (
"IfNot",
*reg as i32,
target_pc.to_debug_int(),
*null_reg as i32,
*jump_if_null as i32,
OwnedValue::build_text(Rc::new("".to_string())),
0,
format!("if !r[{}] goto {}", reg, target_pc.to_debug_int()),

28
core/vdbe/insn.rs
View File

@@ -107,50 +107,66 @@ pub enum Insn {
lhs: usize,
rhs: usize,
target_pc: BranchOffset,
/// Jump if either of the operands is null. Used for "jump when false" logic.
/// Eg. "SELECT * FROM users WHERE id = NULL" becomes:
/// <JUMP TO NEXT ROW IF id != NULL>
/// Without the jump_if_null flag it would not jump because the logical comparison "id != NULL" is never true.
/// This flag indicates that if either is null we should still jump.
jump_if_null: bool,
},
// Compare two registers and jump to the given PC if they are not equal.
Ne {
lhs: usize,
rhs: usize,
target_pc: BranchOffset,
/// Jump if either of the operands is null. Used for "jump when false" logic.
jump_if_null: bool,
},
// Compare two registers and jump to the given PC if the left-hand side is less than the right-hand side.
Lt {
lhs: usize,
rhs: usize,
target_pc: BranchOffset,
/// Jump if either of the operands is null. Used for "jump when false" logic.
jump_if_null: bool,
},
// Compare two registers and jump to the given PC if the left-hand side is less than or equal to the right-hand side.
Le {
lhs: usize,
rhs: usize,
target_pc: BranchOffset,
/// Jump if either of the operands is null. Used for "jump when false" logic.
jump_if_null: bool,
},
// Compare two registers and jump to the given PC if the left-hand side is greater than the right-hand side.
Gt {
lhs: usize,
rhs: usize,
target_pc: BranchOffset,
/// Jump if either of the operands is null. Used for "jump when false" logic.
jump_if_null: bool,
},
// Compare two registers and jump to the given PC if the left-hand side is greater than or equal to the right-hand side.
Ge {
lhs: usize,
rhs: usize,
target_pc: BranchOffset,
/// Jump if either of the operands is null. Used for "jump when false" logic.
jump_if_null: bool,
},
/// Jump to target_pc if r\[reg\] != 0 or (r\[reg\] == NULL && r\[null_reg\] != 0)
/// Jump to target_pc if r\[reg\] != 0 or (r\[reg\] == NULL && r\[jump_if_null\] != 0)
If {
reg: usize, // P1
target_pc: BranchOffset, // P2
/// P3. If r\[reg\] is null, jump iff r\[null_reg\] != 0
null_reg: usize,
/// P3. If r\[reg\] is null, jump iff r\[jump_if_null\] != 0
jump_if_null: bool,
},
/// Jump to target_pc if r\[reg\] != 0 or (r\[reg\] == NULL && r\[null_reg\] != 0)
/// Jump to target_pc if r\[reg\] != 0 or (r\[reg\] == NULL && r\[jump_if_null\] != 0)
IfNot {
reg: usize, // P1
target_pc: BranchOffset, // P2
/// P3. If r\[reg\] is null, jump iff r\[null_reg\] != 0
null_reg: usize,
/// P3. If r\[reg\] is null, jump iff r\[jump_if_null\] != 0
jump_if_null: bool,
},
// Open a cursor for reading.
OpenReadAsync {

83
core/vdbe/mod.rs
View File

@@ -512,6 +512,7 @@ impl Program {
lhs,
rhs,
target_pc,
jump_if_null,
} => {
assert!(target_pc.is_offset());
let lhs = *lhs;
@@ -519,7 +520,11 @@ impl Program {
let target_pc = *target_pc;
match (&state.registers[lhs], &state.registers[rhs]) {
(_, OwnedValue::Null) | (OwnedValue::Null, _) => {
state.pc += 1;
if *jump_if_null {
state.pc = target_pc.to_offset_int();
} else {
state.pc += 1;
}
}
_ => {
if state.registers[lhs] == state.registers[rhs] {
@@ -534,6 +539,7 @@ impl Program {
lhs,
rhs,
target_pc,
jump_if_null,
} => {
assert!(target_pc.is_offset());
let lhs = *lhs;
@@ -541,7 +547,11 @@ impl Program {
let target_pc = *target_pc;
match (&state.registers[lhs], &state.registers[rhs]) {
(_, OwnedValue::Null) | (OwnedValue::Null, _) => {
state.pc += 1;
if *jump_if_null {
state.pc = target_pc.to_offset_int();
} else {
state.pc += 1;
}
}
_ => {
if state.registers[lhs] != state.registers[rhs] {
@@ -556,6 +566,7 @@ impl Program {
lhs,
rhs,
target_pc,
jump_if_null,
} => {
assert!(target_pc.is_offset());
let lhs = *lhs;
@@ -563,7 +574,11 @@ impl Program {
let target_pc = *target_pc;
match (&state.registers[lhs], &state.registers[rhs]) {
(_, OwnedValue::Null) | (OwnedValue::Null, _) => {
state.pc += 1;
if *jump_if_null {
state.pc = target_pc.to_offset_int();
} else {
state.pc += 1;
}
}
_ => {
if state.registers[lhs] < state.registers[rhs] {
@@ -578,6 +593,7 @@ impl Program {
lhs,
rhs,
target_pc,
jump_if_null,
} => {
assert!(target_pc.is_offset());
let lhs = *lhs;
@@ -585,7 +601,11 @@ impl Program {
let target_pc = *target_pc;
match (&state.registers[lhs], &state.registers[rhs]) {
(_, OwnedValue::Null) | (OwnedValue::Null, _) => {
state.pc += 1;
if *jump_if_null {
state.pc = target_pc.to_offset_int();
} else {
state.pc += 1;
}
}
_ => {
if state.registers[lhs] <= state.registers[rhs] {
@@ -600,6 +620,7 @@ impl Program {
lhs,
rhs,
target_pc,
jump_if_null,
} => {
assert!(target_pc.is_offset());
let lhs = *lhs;
@@ -607,7 +628,11 @@ impl Program {
let target_pc = *target_pc;
match (&state.registers[lhs], &state.registers[rhs]) {
(_, OwnedValue::Null) | (OwnedValue::Null, _) => {
state.pc += 1;
if *jump_if_null {
state.pc = target_pc.to_offset_int();
} else {
state.pc += 1;
}
}
_ => {
if state.registers[lhs] > state.registers[rhs] {
@@ -622,6 +647,7 @@ impl Program {
lhs,
rhs,
target_pc,
jump_if_null,
} => {
assert!(target_pc.is_offset());
let lhs = *lhs;
@@ -629,7 +655,11 @@ impl Program {
let target_pc = *target_pc;
match (&state.registers[lhs], &state.registers[rhs]) {
(_, OwnedValue::Null) | (OwnedValue::Null, _) => {
state.pc += 1;
if *jump_if_null {
state.pc = target_pc.to_offset_int();
} else {
state.pc += 1;
}
}
_ => {
if state.registers[lhs] >= state.registers[rhs] {
@@ -643,10 +673,10 @@ impl Program {
Insn::If {
reg,
target_pc,
null_reg,
jump_if_null,
} => {
assert!(target_pc.is_offset());
if exec_if(&state.registers[*reg], &state.registers[*null_reg], false) {
if exec_if(&state.registers[*reg], *jump_if_null, false) {
state.pc = target_pc.to_offset_int();
} else {
state.pc += 1;
@@ -655,10 +685,10 @@ impl Program {
Insn::IfNot {
reg,
target_pc,
null_reg,
jump_if_null,
} => {
assert!(target_pc.is_offset());
if exec_if(&state.registers[*reg], &state.registers[*null_reg], true) {
if exec_if(&state.registers[*reg], *jump_if_null, true) {
state.pc = target_pc.to_offset_int();
} else {
state.pc += 1;
@@ -3011,15 +3041,11 @@ fn exec_zeroblob(req: &OwnedValue) -> OwnedValue {
}
// exec_if returns whether you should jump
fn exec_if(reg: &OwnedValue, null_reg: &OwnedValue, not: bool) -> bool {
fn exec_if(reg: &OwnedValue, jump_if_null: bool, not: bool) -> bool {
match reg {
OwnedValue::Integer(0) | OwnedValue::Float(0.0) => not,
OwnedValue::Integer(_) | OwnedValue::Float(_) => !not,
OwnedValue::Null => match null_reg {
OwnedValue::Integer(0) | OwnedValue::Float(0.0) => false,
OwnedValue::Integer(_) | OwnedValue::Float(_) => true,
_ => false,
},
OwnedValue::Null => jump_if_null,
_ => false,
}
}
@@ -3841,29 +3867,24 @@ mod tests {
#[test]
fn test_exec_if() {
let reg = OwnedValue::Integer(0);
let null_reg = OwnedValue::Integer(0);
assert!(!exec_if(&reg, &null_reg, false));
assert!(exec_if(&reg, &null_reg, true));
assert!(!exec_if(&reg, false, false));
assert!(exec_if(&reg, false, true));
let reg = OwnedValue::Integer(1);
let null_reg = OwnedValue::Integer(0);
assert!(exec_if(&reg, &null_reg, false));
assert!(!exec_if(&reg, &null_reg, true));
assert!(exec_if(&reg, false, false));
assert!(!exec_if(&reg, false, true));
let reg = OwnedValue::Null;
let null_reg = OwnedValue::Integer(0);
assert!(!exec_if(&reg, &null_reg, false));
assert!(!exec_if(&reg, &null_reg, true));
assert!(!exec_if(&reg, false, false));
assert!(!exec_if(&reg, false, true));
let reg = OwnedValue::Null;
let null_reg = OwnedValue::Integer(1);
assert!(exec_if(&reg, &null_reg, false));
assert!(exec_if(&reg, &null_reg, true));
assert!(exec_if(&reg, true, false));
assert!(exec_if(&reg, true, true));
let reg = OwnedValue::Null;
let null_reg = OwnedValue::Null;
assert!(!exec_if(&reg, &null_reg, false));
assert!(!exec_if(&reg, &null_reg, true));
assert!(!exec_if(&reg, false, false));
assert!(!exec_if(&reg, false, true));
}
#[test]

26
testing/where.test
View File

@@ -382,4 +382,28 @@ do_execsql_test nested-parens-and-inside-or-regression-test {
AND
(id = 6 OR FALSE)
);
} {1}
} {1}
# Regression tests for binary conditional jump comparisons where one operand is null
# Test behavior of binary comparisons (=,>,<,>=,<=,!=) when one operand is NULL
# Each test has 3 variants:
# 1. Simple comparison with NULL (should return empty)
# 2. Comparison with NULL OR id=1 (should return Jamie)
# 3. Comparison with NULL AND id=1 (should return empty)
foreach {operator} {
=
>
<
>=
<=
!=
} {
# Simple NULL comparison
do_execsql_test where-binary-one-operand-null-$operator "select * from users where first_name $operator NULL" {}
# NULL comparison OR id=1
do_execsql_test where-binary-one-operand-null-or-$operator "select first_name from users where first_name $operator NULL OR id = 1" {Jamie}
# NULL comparison AND id=1
do_execsql_test where-binary-one-operand-null-and-$operator "select first_name from users where first_name $operator NULL AND id = 1" {}
}