Merge 'Fix null compare operations not giving null' from Vrishabh

In limbo when we do any compare operations like `Eq, gt, lt, gte, lte` with nulls , we were actually giving the result as true where as sqlite3 gives null. This is because if we had a null, we were incorrectly going to conditional branch and not increment program by 1. Also the sqlite generates `ZeroOrNull` op in these cases (https://github.com/sqlite/sqlite/blob/version-3.45.3/src/expr.c#L4644) but we were generating a Integer instruction. The below outputs can give a clearer picture. This PR aims to fix this. sqlite3 output ``` SQLite version 3.48.0 Enter ".help" for usage hints. Connected to a transient in-memory database. Use ".open FILENAME" to reopen on a persistent database. sqlite> select 8 = null; sqlite> select 8 > null; sqlite> explain select 8 > null; addr opcode p1 p2 p3 p4 p5 comment ---- ------------- ---- ---- ---- ------------- -- ------------- 0 Init 0 6 0 0 1 Integer 1 1 0 0 2 Gt 3 4 2 64 3 ZeroOrNull 2 1 3 0 4 ResultRow 1 1 0 0 5 Halt 0 0 0 0 6 Integer 8 2 0 0 7 Null 0 3 0 0 8 Goto 0 1 0 0 sqlite> explain select 8 = null; addr opcode p1 p2 p3 p4 p5 comment ---- ------------- ---- ---- ---- ------------- -- ------------- 0 Init 0 6 0 0 1 Integer 1 1 0 0 2 Eq 3 4 2 64 3 ZeroOrNull 2 1 3 0 4 ResultRow 1 1 0 0 5 Halt 0 0 0 0 6 Integer 8 2 0 0 7 Null 0 3 0 0 8 Goto 0 1 0 0 ``` Limbo Output ``` Limbo v0.0.12 Enter ".help" for usage hints. Connected to a transient in-memory database. Use ".open FILENAME" to reopen on a persistent database limbo> select 8 = null; 1 limbo> select 8 > null; 1 limbo> explain select 8 > null; addr opcode p1 p2 p3 p4 p5 comment ---- ----------------- ---- ---- ---- ------------- -- ------- 0 Init 0 8 0 0 Start at 8 1 Integer 8 2 0 0 r[2]=8 2 Null 0 3 0 0 r[3]=NULL 3 Integer 1 1 0 0 r[1]=1 4 Gt 2 3 6 0 if r[2]>r[3] goto 6 5 Integer 0 1 0 0 r[1]=0 6 ResultRow 1 1 0 0 output=r[1] 7 Halt 0 0 0 0 8 Transaction 0 0 0 0 9 Goto 0 1 0 0 limbo> explain select 8 = null; addr opcode p1 p2 p3 p4 p5 comment ---- ----------------- ---- ---- ---- ------------- -- ------- 0 Init 0 8 0 0 Start at 8 1 Integer 8 2 0 0 r[2]=8 2 Null 0 3 0 0 r[3]=NULL 3 Integer 1 1 0 0 r[1]=1 4 Eq 2 3 6 0 if r[2]==r[3] goto 6 5 Integer 0 1 0 0 r[1]=0 6 ResultRow 1 1 0 0 output=r[1] 7 Halt 0 0 0 0 8 Transaction 0 0 0 0 9 Goto 0 1 0 0 limbo> ``` Limbo Output with this PR ``` Limbo v0.0.12 Enter ".help" for usage hints. Connected to a transient in-memory database. Use ".open FILENAME" to reopen on a persistent database limbo> select 8 = null; limbo> select 8 > null; limbo> explain select 8 > null; addr opcode p1 p2 p3 p4 p5 comment ---- ----------------- ---- ---- ---- ------------- -- ------- 0 Init 0 8 0 0 Start at 8 1 Integer 8 2 0 0 r[2]=8 2 Null 0 3 0 0 r[3]=NULL 3 Integer 1 1 0 0 r[1]=1 4 Gt 2 3 6 0 if r[2]>r[3] goto 6 5 ZeroOrNull 2 1 3 0 ((r[2]=NULL)|(r[3]=NULL)) ? r[1]=NULL : r[1]=0 6 ResultRow 1 1 0 0 output=r[1] 7 Halt 0 0 0 0 8 Transaction 0 0 0 0 9 Goto 0 1 0 0 limbo> explain select 8 = null; addr opcode p1 p2 p3 p4 p5 comment ---- ----------------- ---- ---- ---- ------------- -- ------- 0 Init 0 8 0 0 Start at 8 1 Integer 8 2 0 0 r[2]=8 2 Null 0 3 0 0 r[3]=NULL 3 Integer 1 1 0 0 r[1]=1 4 Eq 2 3 6 0 if r[2]==r[3] goto 6 5 ZeroOrNull 2 1 3 0 ((r[2]=NULL)|(r[3]=NULL)) ? r[1]=NULL : r[1]=0 6 ResultRow 1 1 0 0 output=r[1] 7 Halt 0 0 0 0 8 Transaction 0 0 0 0 9 Goto 0 1 0 0 ``` Closes #733
tursodatabase · Jan 19, 2025 · 3e28541 · 3e28541
2 parents cdcc985 + e16b349
commit 3e28541
Show file tree

Hide file tree

Showing 6 changed files with 147 additions and 12 deletions.
diff --git a/COMPAT.md b/COMPAT.md
@@ -483,6 +483,7 @@ Modifiers:
 | Variable       | No     |
 | VerifyCookie   | No     |
 | Yield          | Yes    |
+| ZeroOrNull     | Yes    |
 
 ##  Extensions
 

diff --git a/core/translate/expr.rs b/core/translate/expr.rs
@@ -453,7 +453,7 @@ pub fn translate_expr(
             match op {
                 ast::Operator::NotEquals => {
                     let if_true_label = program.allocate_label();
-                    wrap_eval_jump_expr(
+                    wrap_eval_jump_expr_zero_or_null(
                         program,
                         Insn::Ne {
                             lhs: e1_reg,
@@ -462,11 +462,13 @@ pub fn translate_expr(
                         },
                         target_register,
                         if_true_label,
+                        e1_reg,
+                        e2_reg,
                     );
                 }
                 ast::Operator::Equals => {
                     let if_true_label = program.allocate_label();
-                    wrap_eval_jump_expr(
+                    wrap_eval_jump_expr_zero_or_null(
                         program,
                         Insn::Eq {
                             lhs: e1_reg,
@@ -475,11 +477,13 @@ pub fn translate_expr(
                         },
                         target_register,
                         if_true_label,
+                        e1_reg,
+                        e2_reg,
                     );
                 }
                 ast::Operator::Less => {
                     let if_true_label = program.allocate_label();
-                    wrap_eval_jump_expr(
+                    wrap_eval_jump_expr_zero_or_null(
                         program,
                         Insn::Lt {
                             lhs: e1_reg,
@@ -488,11 +492,13 @@ pub fn translate_expr(
                         },
                         target_register,
                         if_true_label,
+                        e1_reg,
+                        e2_reg,
                     );
                 }
                 ast::Operator::LessEquals => {
                     let if_true_label = program.allocate_label();
-                    wrap_eval_jump_expr(
+                    wrap_eval_jump_expr_zero_or_null(
                         program,
                         Insn::Le {
                             lhs: e1_reg,
@@ -501,11 +507,13 @@ pub fn translate_expr(
                         },
                         target_register,
                         if_true_label,
+                        e1_reg,
+                        e2_reg,
                     );
                 }
                 ast::Operator::Greater => {
                     let if_true_label = program.allocate_label();
-                    wrap_eval_jump_expr(
+                    wrap_eval_jump_expr_zero_or_null(
                         program,
                         Insn::Gt {
                             lhs: e1_reg,
@@ -514,11 +522,13 @@ pub fn translate_expr(
                         },
                         target_register,
                         if_true_label,
+                        e1_reg,
+                        e2_reg,
                     );
                 }
                 ast::Operator::GreaterEquals => {
                     let if_true_label = program.allocate_label();
-                    wrap_eval_jump_expr(
+                    wrap_eval_jump_expr_zero_or_null(
                         program,
                         Insn::Ge {
                             lhs: e1_reg,
@@ -527,6 +537,8 @@ pub fn translate_expr(
                         },
                         target_register,
                         if_true_label,
+                        e1_reg,
+                        e2_reg,
                     );
                 }
                 ast::Operator::Add => {
@@ -1796,6 +1808,27 @@ fn wrap_eval_jump_expr(
     program.preassign_label_to_next_insn(if_true_label);
 }
 
+fn wrap_eval_jump_expr_zero_or_null(
+    program: &mut ProgramBuilder,
+    insn: Insn,
+    target_register: usize,
+    if_true_label: BranchOffset,
+    e1_reg: usize,
+    e2_reg: usize,
+) {
+    program.emit_insn(Insn::Integer {
+        value: 1, // emit True by default
+        dest: target_register,
+    });
+    program.emit_insn(insn);
+    program.emit_insn(Insn::ZeroOrNull {
+        rg1: e1_reg,
+        rg2: e2_reg,
+        dest: target_register,
+    });
+    program.preassign_label_to_next_insn(if_true_label);
+}
+
 pub fn maybe_apply_affinity(col_type: Type, target_register: usize, program: &mut ProgramBuilder) {
     if col_type == crate::schema::Type::Real {
         program.emit_insn(Insn::RealAffinity {

diff --git a/core/vdbe/explain.rs b/core/vdbe/explain.rs
@@ -1071,6 +1071,18 @@ pub fn insn_to_str(
                 0,
                 format!("r[{}]=parameter({})", *dest, *index),
             ),
+            Insn::ZeroOrNull { rg1, rg2, dest } => (
+                "ZeroOrNull",
+                *rg1 as i32,
+                *dest as i32,
+                *rg2 as i32,
+                OwnedValue::build_text(Rc::new("".to_string())),
+                0,
+                format!(
+                    "((r[{}]=NULL)|(r[{}]=NULL)) ? r[{}]=NULL : r[{}]=0",
+                    rg1, rg2, dest, dest
+                ),
+            ),
         };
     format!(
         "{:<4}  {:<17}  {:<4}  {:<4}  {:<4}  {:<13}  {:<2}  {}",

diff --git a/core/vdbe/insn.rs b/core/vdbe/insn.rs
@@ -509,6 +509,13 @@ pub enum Insn {
         index: NonZero<usize>,
         dest: usize,
     },
+    /// If either register is null put null else put 0
+    ZeroOrNull {
+        /// Source register (P1).
+        rg1: usize,
+        rg2: usize,
+        dest: usize,
+    },
 }
 
 fn cast_text_to_numerical(value: &str) -> OwnedValue {

diff --git a/core/vdbe/mod.rs b/core/vdbe/mod.rs
@@ -495,7 +495,7 @@ impl Program {
                     let target_pc = *target_pc;
                     match (&state.registers[lhs], &state.registers[rhs]) {
                         (_, OwnedValue::Null) | (OwnedValue::Null, _) => {
-                            state.pc = target_pc.to_offset_int();
+                            state.pc += 1;
                         }
                         _ => {
                             if state.registers[lhs] == state.registers[rhs] {
@@ -517,7 +517,7 @@ impl Program {
                     let target_pc = *target_pc;
                     match (&state.registers[lhs], &state.registers[rhs]) {
                         (_, OwnedValue::Null) | (OwnedValue::Null, _) => {
-                            state.pc = target_pc.to_offset_int();
+                            state.pc += 1;
                         }
                         _ => {
                             if state.registers[lhs] != state.registers[rhs] {
@@ -539,7 +539,7 @@ impl Program {
                     let target_pc = *target_pc;
                     match (&state.registers[lhs], &state.registers[rhs]) {
                         (_, OwnedValue::Null) | (OwnedValue::Null, _) => {
-                            state.pc = target_pc.to_offset_int();
+                            state.pc += 1;
                         }
                         _ => {
                             if state.registers[lhs] < state.registers[rhs] {
@@ -561,7 +561,7 @@ impl Program {
                     let target_pc = *target_pc;
                     match (&state.registers[lhs], &state.registers[rhs]) {
                         (_, OwnedValue::Null) | (OwnedValue::Null, _) => {
-                            state.pc = target_pc.to_offset_int();
+                            state.pc += 1;
                         }
                         _ => {
                             if state.registers[lhs] <= state.registers[rhs] {
@@ -583,7 +583,7 @@ impl Program {
                     let target_pc = *target_pc;
                     match (&state.registers[lhs], &state.registers[rhs]) {
                         (_, OwnedValue::Null) | (OwnedValue::Null, _) => {
-                            state.pc = target_pc.to_offset_int();
+                            state.pc += 1;
                         }
                         _ => {
                             if state.registers[lhs] > state.registers[rhs] {
@@ -605,7 +605,7 @@ impl Program {
                     let target_pc = *target_pc;
                     match (&state.registers[lhs], &state.registers[rhs]) {
                         (_, OwnedValue::Null) | (OwnedValue::Null, _) => {
-                            state.pc = target_pc.to_offset_int();
+                            state.pc += 1;
                         }
                         _ => {
                             if state.registers[lhs] >= state.registers[rhs] {
@@ -2255,6 +2255,16 @@ impl Program {
                         .clone();
                     state.pc += 1;
                 }
+                Insn::ZeroOrNull { rg1, rg2, dest } => {
+                    if state.registers[*rg1] == OwnedValue::Null
+                        || state.registers[*rg2] == OwnedValue::Null
+                    {
+                        state.registers[*dest] = OwnedValue::Null
+                    } else {
+                        state.registers[*dest] = OwnedValue::Integer(0);
+                    }
+                    state.pc += 1;
+                }
             }
         }
     }