From f88a4d6ac6c3b4c53a339274370a492ab608bdb3 Mon Sep 17 00:00:00 2001 From: Jussi Saurio Date: Mon, 20 Jan 2025 16:54:39 +0200 Subject: [PATCH 1/3] Add jump_if_null to cmp insns to account for either operand being NULL --- core/translate/expr.rs | 26 ++++++++--- core/translate/group_by.rs | 2 +- core/translate/main_loop.rs | 2 + core/vdbe/builder.rs | 10 +++- core/vdbe/explain.rs | 14 ++++-- core/vdbe/insn.rs | 28 ++++++++--- core/vdbe/mod.rs | 92 ++++++++++++++++++++++++++----------- testing/where.test | 81 +++++++++++++++++++++++++++++++- 8 files changed, 208 insertions(+), 47 deletions(-) diff --git a/core/translate/expr.rs b/core/translate/expr.rs index c16ee8ae3..87e3029d1 100644 --- a/core/translate/expr.rs +++ b/core/translate/expr.rs @@ -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: reg, }); } else { program.emit_insn(Insn::IfNot { reg, target_pc: cond_meta.jump_target_when_false, - null_reg: reg, + jump_if_null: reg, }); } } @@ -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: false, }); } } @@ -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: false, }); } // 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: cur_reg, }); } else { program.emit_insn(Insn::If { reg: cur_reg, target_pc: condition_metadata.jump_target_when_false, - null_reg: cur_reg, + jump_if_null: cur_reg, }); } } @@ -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: 1, }), }; // 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: 1, }); translate_expr( program, diff --git a/core/translate/group_by.rs b/core/translate/group_by.rs index c58d5f56a..6420b54d1 100644 --- a/core/translate/group_by.rs +++ b/core/translate/group_by.rs @@ -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: 0, // unused in this case }); // Read the group by columns for a finished group diff --git a/core/translate/main_loop.rs b/core/translate/main_loop.rs index cc62d3986..9ce449b9e 100644 --- a/core/translate/main_loop.rs +++ b/core/translate/main_loop.rs @@ -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, }); } } diff --git a/core/vdbe/builder.rs b/core/vdbe/builder.rs index 07ef77307..08b35d9e3 100644 --- a/core/vdbe/builder.rs +++ b/core/vdbe/builder.rs @@ -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"); } diff --git a/core/vdbe/explain.rs b/core/vdbe/explain.rs index 2eb0a341d..1264f918a 100644 --- a/core/vdbe/explain.rs +++ b/core/vdbe/explain.rs @@ -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()), diff --git a/core/vdbe/insn.rs b/core/vdbe/insn.rs index 05cb11a79..07c54fd0e 100644 --- a/core/vdbe/insn.rs +++ b/core/vdbe/insn.rs @@ -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: + /// + /// 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: usize, }, - /// 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: usize, }, // Open a cursor for reading. OpenReadAsync { diff --git a/core/vdbe/mod.rs b/core/vdbe/mod.rs index 723f7b5ec..e7960b20a 100644 --- a/core/vdbe/mod.rs +++ b/core/vdbe/mod.rs @@ -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,14 @@ 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], + &state.registers[*jump_if_null], + false, + ) { state.pc = target_pc.to_offset_int(); } else { state.pc += 1; @@ -655,10 +689,14 @@ 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], + &state.registers[*jump_if_null], + true, + ) { state.pc = target_pc.to_offset_int(); } else { state.pc += 1; @@ -3011,11 +3049,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: &OwnedValue, not: bool) -> bool { match reg { OwnedValue::Integer(0) | OwnedValue::Float(0.0) => not, OwnedValue::Integer(_) | OwnedValue::Float(_) => !not, - OwnedValue::Null => match null_reg { + OwnedValue::Null => match jump_if_null { OwnedValue::Integer(0) | OwnedValue::Float(0.0) => false, OwnedValue::Integer(_) | OwnedValue::Float(_) => true, _ => false, @@ -3841,29 +3879,29 @@ mod tests { #[test] fn test_exec_if() { let reg = OwnedValue::Integer(0); - let null_reg = OwnedValue::Integer(0); - assert!(!exec_if(®, &null_reg, false)); - assert!(exec_if(®, &null_reg, true)); + let jump_if_null = OwnedValue::Integer(0); + assert!(!exec_if(®, &jump_if_null, false)); + assert!(exec_if(®, &jump_if_null, true)); let reg = OwnedValue::Integer(1); - let null_reg = OwnedValue::Integer(0); - assert!(exec_if(®, &null_reg, false)); - assert!(!exec_if(®, &null_reg, true)); + let jump_if_null = OwnedValue::Integer(0); + assert!(exec_if(®, &jump_if_null, false)); + assert!(!exec_if(®, &jump_if_null, true)); let reg = OwnedValue::Null; - let null_reg = OwnedValue::Integer(0); - assert!(!exec_if(®, &null_reg, false)); - assert!(!exec_if(®, &null_reg, true)); + let jump_if_null = OwnedValue::Integer(0); + assert!(!exec_if(®, &jump_if_null, false)); + assert!(!exec_if(®, &jump_if_null, true)); let reg = OwnedValue::Null; - let null_reg = OwnedValue::Integer(1); - assert!(exec_if(®, &null_reg, false)); - assert!(exec_if(®, &null_reg, true)); + let jump_if_null = OwnedValue::Integer(1); + assert!(exec_if(®, &jump_if_null, false)); + assert!(exec_if(®, &jump_if_null, true)); let reg = OwnedValue::Null; - let null_reg = OwnedValue::Null; - assert!(!exec_if(®, &null_reg, false)); - assert!(!exec_if(®, &null_reg, true)); + let jump_if_null = OwnedValue::Null; + assert!(!exec_if(®, &jump_if_null, false)); + assert!(!exec_if(®, &jump_if_null, true)); } #[test] diff --git a/testing/where.test b/testing/where.test index b3154cd7c..eee28b247 100755 --- a/testing/where.test +++ b/testing/where.test @@ -382,4 +382,83 @@ do_execsql_test nested-parens-and-inside-or-regression-test { AND (id = 6 OR FALSE) ); -} {1} \ No newline at end of file +} {1} + +# Regression tests for binary conditional jump comparisons where one operand is null +do_execsql_test where-binary-one-operand-null-1 { + select * from users where first_name = NULL; +} {} + +do_execsql_test where-binary-one-operand-null-2 { + select first_name from users where first_name = NULL OR id = 1; +} {Jamie} + +do_execsql_test where-binary-one-operand-null-3 { + select first_name from users where first_name = NULL AND id = 1; +} {} + +do_execsql_test where-binary-one-operand-null-4 { + select * from users where first_name > NULL; +} {} + +do_execsql_test where-binary-one-operand-null-5 { + select first_name from users where first_name > NULL OR id = 1; +} {Jamie} + +do_execsql_test where-binary-one-operand-null-6 { + select first_name from users where first_name > NULL AND id = 1; +} {} + +do_execsql_test where-binary-one-operand-null-7 { + select * from users where first_name < NULL; +} {} + +do_execsql_test where-binary-one-operand-null-8 { + select first_name from users where first_name < NULL OR id = 1; +} {Jamie} + +do_execsql_test where-binary-one-operand-null-9 { + select first_name from users where first_name < NULL AND id = 1; +} {} + +do_execsql_test where-binary-one-operand-null-10 { + select * from users where first_name >= NULL; +} {} + +do_execsql_test where-binary-one-operand-null-11 { + select first_name from users where first_name >= NULL OR id = 1; +} {Jamie} + +do_execsql_test where-binary-one-operand-null-12 { + select first_name from users where first_name >= NULL AND id = 1; +} {} + +do_execsql_test where-binary-one-operand-null-13 { + select * from users where first_name <= NULL; +} {} + +do_execsql_test where-binary-one-operand-null-14 { + select first_name from users where first_name <= NULL OR id = 1; +} {Jamie} + +do_execsql_test where-binary-one-operand-null-15 { + select first_name from users where first_name <= NULL AND id = 1; +} {} + +do_execsql_test where-binary-one-operand-null-16 { + select * from users where first_name != NULL; +} {} + +do_execsql_test where-binary-one-operand-null-17 { + select first_name from users where first_name != NULL OR id = 1; +} {Jamie} + +do_execsql_test where-binary-one-operand-null-18 { + select first_name from users where first_name != NULL AND id = 1; +} {} + + + + + + From 2cd9118be6f434a386f9c8a0e57b921d869155bc Mon Sep 17 00:00:00 2001 From: Jussi Saurio Date: Mon, 20 Jan 2025 17:08:18 +0200 Subject: [PATCH 2/3] Fix jump_if_true to be a bool literal in places where it was used as a register number --- core/translate/expr.rs | 16 +++++++------- core/translate/group_by.rs | 2 +- core/vdbe/insn.rs | 4 ++-- core/vdbe/mod.rs | 45 ++++++++++++-------------------------- 4 files changed, 25 insertions(+), 42 deletions(-) diff --git a/core/translate/expr.rs b/core/translate/expr.rs index 87e3029d1..349a5f2b3 100644 --- a/core/translate/expr.rs +++ b/core/translate/expr.rs @@ -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, - jump_if_null: reg, + jump_if_null: false, }); } else { program.emit_insn(Insn::IfNot { reg, target_pc: cond_meta.jump_target_when_false, - jump_if_null: reg, + jump_if_null: true, }); } } @@ -334,7 +334,7 @@ pub fn translate_condition_expr( lhs: lhs_reg, rhs: rhs_reg, target_pc: condition_metadata.jump_target_when_false, - jump_if_null: false, + jump_if_null: true, }); } } @@ -355,7 +355,7 @@ pub fn translate_condition_expr( lhs: lhs_reg, rhs: rhs_reg, target_pc: condition_metadata.jump_target_when_false, - jump_if_null: 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'. @@ -419,13 +419,13 @@ pub fn translate_condition_expr( program.emit_insn(Insn::IfNot { reg: cur_reg, target_pc: condition_metadata.jump_target_when_true, - jump_if_null: cur_reg, + jump_if_null: false, }); } else { program.emit_insn(Insn::If { reg: cur_reg, target_pc: condition_metadata.jump_target_when_false, - jump_if_null: cur_reg, + jump_if_null: true, }); } } @@ -725,7 +725,7 @@ pub fn translate_expr( None => program.emit_insn(Insn::IfNot { reg: expr_reg, target_pc: next_case_label, - jump_if_null: 1, + jump_if_null: true, }), }; // THEN... @@ -1079,7 +1079,7 @@ pub fn translate_expr( program.emit_insn(Insn::IfNot { reg: temp_reg, target_pc: jump_target_when_false, - jump_if_null: 1, + jump_if_null: true, }); translate_expr( program, diff --git a/core/translate/group_by.rs b/core/translate/group_by.rs index 6420b54d1..dfb92f20a 100644 --- a/core/translate/group_by.rs +++ b/core/translate/group_by.rs @@ -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, - jump_if_null: 0, // unused in this case + jump_if_null: false, }); // Read the group by columns for a finished group diff --git a/core/vdbe/insn.rs b/core/vdbe/insn.rs index 07c54fd0e..399aea8a8 100644 --- a/core/vdbe/insn.rs +++ b/core/vdbe/insn.rs @@ -159,14 +159,14 @@ pub enum Insn { reg: usize, // P1 target_pc: BranchOffset, // P2 /// P3. If r\[reg\] is null, jump iff r\[jump_if_null\] != 0 - jump_if_null: usize, + jump_if_null: bool, }, /// 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\[jump_if_null\] != 0 - jump_if_null: usize, + jump_if_null: bool, }, // Open a cursor for reading. OpenReadAsync { diff --git a/core/vdbe/mod.rs b/core/vdbe/mod.rs index e7960b20a..b8c5f9474 100644 --- a/core/vdbe/mod.rs +++ b/core/vdbe/mod.rs @@ -676,11 +676,7 @@ impl Program { jump_if_null, } => { assert!(target_pc.is_offset()); - if exec_if( - &state.registers[*reg], - &state.registers[*jump_if_null], - false, - ) { + if exec_if(&state.registers[*reg], *jump_if_null, false) { state.pc = target_pc.to_offset_int(); } else { state.pc += 1; @@ -692,11 +688,7 @@ impl Program { jump_if_null, } => { assert!(target_pc.is_offset()); - if exec_if( - &state.registers[*reg], - &state.registers[*jump_if_null], - true, - ) { + if exec_if(&state.registers[*reg], *jump_if_null, true) { state.pc = target_pc.to_offset_int(); } else { state.pc += 1; @@ -3049,15 +3041,11 @@ fn exec_zeroblob(req: &OwnedValue) -> OwnedValue { } // exec_if returns whether you should jump -fn exec_if(reg: &OwnedValue, jump_if_null: &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 jump_if_null { - OwnedValue::Integer(0) | OwnedValue::Float(0.0) => false, - OwnedValue::Integer(_) | OwnedValue::Float(_) => true, - _ => false, - }, + OwnedValue::Null => jump_if_null, _ => false, } } @@ -3879,29 +3867,24 @@ mod tests { #[test] fn test_exec_if() { let reg = OwnedValue::Integer(0); - let jump_if_null = OwnedValue::Integer(0); - assert!(!exec_if(®, &jump_if_null, false)); - assert!(exec_if(®, &jump_if_null, true)); + assert!(!exec_if(®, false, false)); + assert!(exec_if(®, false, true)); let reg = OwnedValue::Integer(1); - let jump_if_null = OwnedValue::Integer(0); - assert!(exec_if(®, &jump_if_null, false)); - assert!(!exec_if(®, &jump_if_null, true)); + assert!(exec_if(®, false, false)); + assert!(!exec_if(®, false, true)); let reg = OwnedValue::Null; - let jump_if_null = OwnedValue::Integer(0); - assert!(!exec_if(®, &jump_if_null, false)); - assert!(!exec_if(®, &jump_if_null, true)); + assert!(!exec_if(®, false, false)); + assert!(!exec_if(®, false, true)); let reg = OwnedValue::Null; - let jump_if_null = OwnedValue::Integer(1); - assert!(exec_if(®, &jump_if_null, false)); - assert!(exec_if(®, &jump_if_null, true)); + assert!(exec_if(®, true, false)); + assert!(exec_if(®, true, true)); let reg = OwnedValue::Null; - let jump_if_null = OwnedValue::Null; - assert!(!exec_if(®, &jump_if_null, false)); - assert!(!exec_if(®, &jump_if_null, true)); + assert!(!exec_if(®, false, false)); + assert!(!exec_if(®, false, true)); } #[test] From ce15ad7d32f7d8908c1fe9ed77d5547e3d378f06 Mon Sep 17 00:00:00 2001 From: Jussi Saurio Date: Mon, 20 Jan 2025 17:30:04 +0200 Subject: [PATCH 3/3] Simplify added tests with foreach --- testing/where.test | 99 +++++++++++----------------------------------- 1 file changed, 22 insertions(+), 77 deletions(-) diff --git a/testing/where.test b/testing/where.test index eee28b247..dded0a21b 100755 --- a/testing/where.test +++ b/testing/where.test @@ -385,80 +385,25 @@ do_execsql_test nested-parens-and-inside-or-regression-test { } {1} # Regression tests for binary conditional jump comparisons where one operand is null -do_execsql_test where-binary-one-operand-null-1 { - select * from users where first_name = NULL; -} {} - -do_execsql_test where-binary-one-operand-null-2 { - select first_name from users where first_name = NULL OR id = 1; -} {Jamie} - -do_execsql_test where-binary-one-operand-null-3 { - select first_name from users where first_name = NULL AND id = 1; -} {} - -do_execsql_test where-binary-one-operand-null-4 { - select * from users where first_name > NULL; -} {} - -do_execsql_test where-binary-one-operand-null-5 { - select first_name from users where first_name > NULL OR id = 1; -} {Jamie} - -do_execsql_test where-binary-one-operand-null-6 { - select first_name from users where first_name > NULL AND id = 1; -} {} - -do_execsql_test where-binary-one-operand-null-7 { - select * from users where first_name < NULL; -} {} - -do_execsql_test where-binary-one-operand-null-8 { - select first_name from users where first_name < NULL OR id = 1; -} {Jamie} - -do_execsql_test where-binary-one-operand-null-9 { - select first_name from users where first_name < NULL AND id = 1; -} {} - -do_execsql_test where-binary-one-operand-null-10 { - select * from users where first_name >= NULL; -} {} - -do_execsql_test where-binary-one-operand-null-11 { - select first_name from users where first_name >= NULL OR id = 1; -} {Jamie} - -do_execsql_test where-binary-one-operand-null-12 { - select first_name from users where first_name >= NULL AND id = 1; -} {} - -do_execsql_test where-binary-one-operand-null-13 { - select * from users where first_name <= NULL; -} {} - -do_execsql_test where-binary-one-operand-null-14 { - select first_name from users where first_name <= NULL OR id = 1; -} {Jamie} - -do_execsql_test where-binary-one-operand-null-15 { - select first_name from users where first_name <= NULL AND id = 1; -} {} - -do_execsql_test where-binary-one-operand-null-16 { - select * from users where first_name != NULL; -} {} - -do_execsql_test where-binary-one-operand-null-17 { - select first_name from users where first_name != NULL OR id = 1; -} {Jamie} - -do_execsql_test where-binary-one-operand-null-18 { - select first_name from users where first_name != NULL AND id = 1; -} {} - - - - - - +# 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" {} +}