Repository: tursodatabase/limbo (language: rust)

Issue:
OR behavior seems broken
This *could* be a bug in the new IS / ISNOT implementation, but very likely it is the logic for OR, since the bytecode we generate is different, and it seems just that we are being asked to do the wrong comparison.

Data:
```
sqlite> .dump
PRAGMA foreign_keys=OFF;
BEGIN TRANSACTION;
CREATE TABLE demo (id INTEGER, value TEXT);
INSERT INTO demo VALUES(1,'A');
INSERT INTO demo VALUES(2,NULL);
INSERT INTO demo VALUES(3,'B');
INSERT INTO demo VALUES(4,NULL);
INSERT INTO demo VALUES(5,'C');
COMMIT;
```
SQLite

```
sqlite> select * from demo where value is null or id == 2;
2|
4|
sqlite> explain select * from demo where value is null or id == 2;
addr  opcode         p1    p2    p3    p4             p5  comment
----  -------------  ----  ----  ----  -------------  --  -------------
0     Init           0     12    0                    0   Start at 12
1     OpenRead       0     2     0     2              0   root=2 iDb=0; demo
2     Rewind         0     11    0                    0
3       Column         0     1     1                    128 r[1]= cursor 0 column 1
4       IsNull         1     7     0                    0   if r[1]==NULL goto 7 <==== NOTICE IsNull is generated here
5       Column         0     0     1                    0   r[1]= cursor 0 column 0
6       Ne             2     10    1     BINARY-8       84  if r[1]!=r[2] goto 10
7       Column         0     0     3                    0   r[3]= cursor 0 column 0
8       Column         0     1     4                    0   r[4]= cursor 0 column 1
9       ResultRow      3     2     0                    0   output=r[3..4]
10    Next           0     3     0                    1
11    Halt           0     0     0                    0
12    Transaction    0     0     1     0              1   usesStmtJournal=0
13    Integer        2     2     0                    0   r[2]=2
14    Goto           0     1     0                    0
```

Limbo:

```
limbo> select * from demo where value is not null or id == 2;
2|
limbo> explain select * from demo where value is null or id == 2;
addr  opcode             p1    p2    p3    p4             p5  comment
----  -----------------  ----  ----  ----  -------------  --  -------
0     Init               0     15    0                    0   Start at 15
1     OpenReadAsync      0     2     0                    0   table=demo, root=2
2     OpenReadAwait      0     0     0                    0
3     RewindAsync        0     0     0                    0
4     RewindAwait        0     14    0                    0   Rewind table demo
5       Column           0     1     3                    0   r[3]=demo.value
6       NotNull          3     7     0                    0   r[3]!=NULL -> goto 7.  <=== NOTICE NotNull is generated here
7       Column           0     0     4                    0   r[4]=demo.id
8       Ne               4     5     12                   0   if r[4]!=r[5] goto 12
9       Column           0     0     1                    0   r[1]=demo.id
10      Column           0     1     2                    0   r[2]=demo.value
11      ResultRow        1     2     0                    0   output=r[1..2]
12    NextAsync          0     0     0                    0
13    NextAwait          0     5     0                    0
14    Halt               0     0     0                    0
15    Transaction        0     0     0                    0
16    Integer            2     5     0                    0   r[5]=2
17    Goto               0     1     0                    0
```


Discussion / hints:

Fix patch:
diff --git a/core/translate/expr.rs b/core/translate/expr.rs
index 1c275f9da..7484abcaf 100644
--- a/core/translate/expr.rs
+++ b/core/translate/expr.rs
@@ -44,23 +44,19 @@ macro_rules! emit_cmp_insn {
         $op_true:ident,
         $op_false:ident,
         $lhs:expr,
-        $rhs:expr,
-        $tables:expr,
-        $resolver:expr
+        $rhs:expr
     ) => {{
-        let lhs_reg = translate_and_mark($program, Some($tables), $lhs, $resolver)?;
-        let rhs_reg = translate_and_mark($program, Some($tables), $rhs, $resolver)?;
         if $cond.jump_if_condition_is_true {
             $program.emit_insn(Insn::$op_true {
-                lhs: lhs_reg,
-                rhs: rhs_reg,
+                lhs: $lhs,
+                rhs: $rhs,
                 target_pc: $cond.jump_target_when_true,
                 flags: CmpInsFlags::default(),
             });
         } else {
             $program.emit_insn(Insn::$op_false {
-                lhs: lhs_reg,
-                rhs: rhs_reg,
+                lhs: $lhs,
+                rhs: $rhs,
                 target_pc: $cond.jump_target_when_false,
                 flags: CmpInsFlags::default().jump_if_null(),
             });
@@ -74,42 +70,23 @@ macro_rules! emit_cmp_null_insn {
         $cond:expr,
         $op_true:ident,
         $op_false:ident,
-        $op_null:ident,
         $lhs:expr,
-        $rhs:expr,
-        $tables:expr,
-        $resolver:expr
+        $rhs:expr
     ) => {{
-        if **$rhs == ast::Expr::Literal(ast::Literal::Null) {
-            let lhs_reg = translate_and_mark($program, Some($tables), $lhs, $resolver)?;
-            $program.emit_insn(Insn::$op_null {
-                reg: lhs_reg,
-                target_pc: $cond.jump_target_when_false,
+        if $cond.jump_if_condition_is_true {
+            $program.emit_insn(Insn::$op_true {
+                lhs: $lhs,
+                rhs: $rhs,
+                target_pc: $cond.jump_target_when_true,
+                flags: CmpInsFlags::default().null_eq(),
             });
-        } else if **$lhs == ast::Expr::Literal(ast::Literal::Null) {
-            let rhs_reg = translate_and_mark($program, Some($tables), $rhs, $resolver)?;
-            $program.emit_insn(Insn::$op_null {
-                reg: rhs_reg,
+        } else {
+            $program.emit_insn(Insn::$op_false {
+                lhs: $lhs,
+                rhs: $rhs,
                 target_pc: $cond.jump_target_when_false,
+                flags: CmpInsFlags::default().null_eq(),
             });
-        } else {
-            let lhs_reg = translate_and_mark($program, Some($tables), $lhs, $resolver)?;
-            let rhs_reg = translate_and_mark($program, Some($tables), $rhs, $resolver)?;
-            if $cond.jump_if_condition_is_true {
-                $program.emit_insn(Insn::$op_true {
-                    lhs: lhs_reg,
-                    rhs: rhs_reg,
-                    target_pc: $cond.jump_target_when_true,
-                    flags: CmpInsFlags::default().null_eq(),
-                });
-            } else {
-                $program.emit_insn(Insn::$op_false {
-                    lhs: lhs_reg,
-                    rhs: rhs_reg,
-                    target_pc: $cond.jump_target_when_false,
-                    flags: CmpInsFlags::default().jump_if_null().null_eq(),
-                });
-            }
         }
     }};
 }
@@ -258,109 +235,39 @@ pub fn translate_condition_expr(
                 resolver,
             )?;
         }
-        ast::Expr::Binary(lhs, op, rhs) => match op {
-            ast::Operator::Greater => {
-                emit_cmp_insn!(
-                    program,
-                    condition_metadata,
-                    Gt,
-                    Le,
-                    lhs,
-                    rhs,
-                    referenced_tables,
-                    resolver
-                )
-            }
-            ast::Operator::GreaterEquals => {
-                emit_cmp_insn!(
-                    program,
-                    condition_metadata,
-                    Ge,
-                    Lt,
-                    lhs,
-                    rhs,
-                    referenced_tables,
-                    resolver
-                )
-            }
-            ast::Operator::Less => {
-                emit_cmp_insn!(
-                    program,
-                    condition_metadata,
-                    Lt,
-                    Ge,
-                    lhs,
-                    rhs,
-                    referenced_tables,
-                    resolver
-                )
-            }
-            ast::Operator::LessEquals => {
-                emit_cmp_insn!(
-                    program,
-                    condition_metadata,
-                    Le,
-                    Gt,
-                    lhs,
-                    rhs,
-                    referenced_tables,
-                    resolver
-                )
-            }
-            ast::Operator::Equals => {
-                emit_cmp_insn!(
-                    program,
-                    condition_metadata,
-                    Eq,
-                    Ne,
-                    lhs,
-                    rhs,
-                    referenced_tables,
-                    resolver
-                )
-            }
-            ast::Operator::NotEquals => {
-                emit_cmp_insn!(
-                    program,
-                    condition_metadata,
-                    Ne,
-                    Eq,
-                    lhs,
-                    rhs,
-                    referenced_tables,
-                    resolver
-                )
-            }
-            ast::Operator::Is => {
-                emit_cmp_null_insn!(
-                    program,
-                    condition_metadata,
-                    Eq,
-                    Ne,
-                    NotNull,
-                    lhs,
-                    rhs,
-                    referenced_tables,
-                    resolver
-                )
-            }
-            ast::Operator::IsNot => {
-                emit_cmp_null_insn!(
-                    program,
-                    condition_metadata,
-                    Ne,
-                    Eq,
-                    IsNull,
-                    lhs,
-                    rhs,
-                    referenced_tables,
-                    resolver
-                )
-            }
-            _ => {
-                todo!("op {:?} not implemented", op);
+        ast::Expr::Binary(lhs, op, rhs) => {
+            let lhs_reg = translate_and_mark(program, Some(referenced_tables), lhs, resolver)?;
+            let rhs_reg = translate_and_mark(program, Some(referenced_tables), rhs, resolver)?;
+            match op {
+                ast::Operator::Greater => {
+                    emit_cmp_insn!(program, condition_metadata, Gt, Le, lhs_reg, rhs_reg)
+                }
+                ast::Operator::GreaterEquals => {
+                    emit_cmp_insn!(program, condition_metadata, Ge, Lt, lhs_reg, rhs_reg)
+                }
+                ast::Operator::Less => {
+                    emit_cmp_insn!(program, condition_metadata, Lt, Ge, lhs_reg, rhs_reg)
+                }
+                ast::Operator::LessEquals => {
+                    emit_cmp_insn!(program, condition_metadata, Le, Gt, lhs_reg, rhs_reg)
+                }
+                ast::Operator::Equals => {
+                    emit_cmp_insn!(program, condition_metadata, Eq, Ne, lhs_reg, rhs_reg)
+                }
+                ast::Operator::NotEquals => {
+                    emit_cmp_insn!(program, condition_metadata, Ne, Eq, lhs_reg, rhs_reg)
+                }
+                ast::Operator::Is => {
+                    emit_cmp_null_insn!(program, condition_metadata, Eq, Ne, lhs_reg, rhs_reg)
+                }
+                ast::Operator::IsNot => {
+                    emit_cmp_null_insn!(program, condition_metadata, Ne, Eq, lhs_reg, rhs_reg)
+                }
+                _ => {
+                    todo!("op {:?} not implemented", op);
+                }
             }
-        },
+        }
         ast::Expr::Literal(lit) => match lit {
             ast::Literal::Numeric(val) => {
                 let maybe_int = val.parse::<i64>();


Test patch: