Skip to content

Commit

Permalink
Merge 'implement is and is not where constraints' from Glauber Costa
Browse files Browse the repository at this point in the history
The main difference between = and != is how null values are handled.
SQLite passes a flag "NULLEQ" to Eq and Ne to disambiguate that.
In the presence of that flag, NULL = NULL.
Some prep work is done to make sure we can pass a flag instead of a
boolean to Eq and Ne. I looked into the bitflags crate but got a bit
scared with the list of dependencies.
Warning:
The following query produces a different result for Limbo:
```
select * from demo where value is null or id == 2;
```
I strongly suspect the issue is with the OR implementation, though. The
bytecode generated is quite different.

Reviewed-by: Jussi Saurio <[email protected]>

Closes #847
  • Loading branch information
penberg committed Feb 1, 2025
2 parents 83f9290 + 3c77797 commit 20d3399
Show file tree
Hide file tree
Showing 11 changed files with 301 additions and 85 deletions.
4 changes: 2 additions & 2 deletions COMPAT.md
Original file line number Diff line number Diff line change
Expand Up @@ -186,8 +186,8 @@ Feature support of [sqlite expr syntax](https://www.sqlite.org/lang_expr.html).
| (NOT) GLOB | Yes | |
| (NOT) REGEXP | No | |
| (NOT) MATCH | No | |
| IS (NOT) | No | |
| IS (NOT) DISTINCT FROM | No | |
| IS (NOT) | Yes | |
| IS (NOT) DISTINCT FROM | Yes | |
| (NOT) BETWEEN ... AND ... | No | |
| (NOT) IN (subquery) | No | |
| (NOT) EXISTS (subquery) | No | |
Expand Down
226 changes: 177 additions & 49 deletions core/translate/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@ use crate::function::JsonFunc;
use crate::function::{Func, FuncCtx, MathFuncArity, ScalarFunc};
use crate::schema::Type;
use crate::util::normalize_ident;
use crate::vdbe::{builder::ProgramBuilder, insn::Insn, BranchOffset};
use crate::vdbe::{
builder::ProgramBuilder,
insn::{CmpInsFlags, Insn},
BranchOffset,
};
use crate::Result;

use super::emitter::Resolver;
Expand Down Expand Up @@ -40,26 +44,76 @@ macro_rules! emit_cmp_insn {
$op_true:ident,
$op_false:ident,
$lhs:expr,
$rhs:expr
$rhs:expr,
$tables:expr,
$resolver: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,
rhs: $rhs,
lhs: lhs_reg,
rhs: rhs_reg,
target_pc: $cond.jump_target_when_true,
jump_if_null: false,
flags: CmpInsFlags::default(),
});
} else {
$program.emit_insn(Insn::$op_false {
lhs: $lhs,
rhs: $rhs,
lhs: lhs_reg,
rhs: rhs_reg,
target_pc: $cond.jump_target_when_false,
jump_if_null: true,
flags: CmpInsFlags::default().jump_if_null(),
});
}
}};
}

macro_rules! emit_cmp_null_insn {
(
$program:expr,
$cond:expr,
$op_true:ident,
$op_false:ident,
$op_null:ident,
$lhs:expr,
$rhs:expr,
$tables:expr,
$resolver: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,
});
} 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,
target_pc: $cond.jump_target_when_false,
});
} 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(),
});
}
}
}};
}

macro_rules! expect_arguments_exact {
(
$args:expr,
Expand Down Expand Up @@ -204,35 +258,109 @@ pub fn translate_condition_expr(
resolver,
)?;
}
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 => todo!(),
ast::Operator::IsNot => todo!(),
_ => {
todo!("op {:?} not implemented", op);
}
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::Literal(lit) => match lit {
ast::Literal::Numeric(val) => {
let maybe_int = val.parse::<i64>();
Expand Down Expand Up @@ -326,15 +454,15 @@ pub fn translate_condition_expr(
lhs: lhs_reg,
rhs: rhs_reg,
target_pc: jump_target_when_true,
jump_if_null: false,
flags: CmpInsFlags::default(),
});
} else {
// If this is the last condition, we need to jump to the 'jump_target_when_false' label if there is no match.
program.emit_insn(Insn::Ne {
lhs: lhs_reg,
rhs: rhs_reg,
target_pc: condition_metadata.jump_target_when_false,
jump_if_null: true,
flags: CmpInsFlags::default().jump_if_null(),
});
}
}
Expand All @@ -355,7 +483,7 @@ pub fn translate_condition_expr(
lhs: lhs_reg,
rhs: rhs_reg,
target_pc: condition_metadata.jump_target_when_false,
jump_if_null: true,
flags: CmpInsFlags::default().jump_if_null(),
});
}
// 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'.
Expand Down Expand Up @@ -448,7 +576,7 @@ pub fn translate_condition_expr(
let cur_reg = program.alloc_register();
translate_expr(program, Some(referenced_tables), expr, cur_reg, resolver)?;
program.emit_insn(Insn::IsNull {
src: cur_reg,
reg: cur_reg,
target_pc: condition_metadata.jump_target_when_false,
});
}
Expand Down Expand Up @@ -498,7 +626,7 @@ pub fn translate_expr(
lhs: e1_reg,
rhs: e2_reg,
target_pc: if_true_label,
jump_if_null: false,
flags: CmpInsFlags::default(),
},
target_register,
if_true_label,
Expand All @@ -514,7 +642,7 @@ pub fn translate_expr(
lhs: e1_reg,
rhs: e2_reg,
target_pc: if_true_label,
jump_if_null: false,
flags: CmpInsFlags::default(),
},
target_register,
if_true_label,
Expand All @@ -530,7 +658,7 @@ pub fn translate_expr(
lhs: e1_reg,
rhs: e2_reg,
target_pc: if_true_label,
jump_if_null: false,
flags: CmpInsFlags::default(),
},
target_register,
if_true_label,
Expand All @@ -546,7 +674,7 @@ pub fn translate_expr(
lhs: e1_reg,
rhs: e2_reg,
target_pc: if_true_label,
jump_if_null: false,
flags: CmpInsFlags::default(),
},
target_register,
if_true_label,
Expand All @@ -562,7 +690,7 @@ pub fn translate_expr(
lhs: e1_reg,
rhs: e2_reg,
target_pc: if_true_label,
jump_if_null: false,
flags: CmpInsFlags::default(),
},
target_register,
if_true_label,
Expand All @@ -578,7 +706,7 @@ pub fn translate_expr(
lhs: e1_reg,
rhs: e2_reg,
target_pc: if_true_label,
jump_if_null: false,
flags: CmpInsFlags::default(),
},
target_register,
if_true_label,
Expand Down Expand Up @@ -671,7 +799,7 @@ pub fn translate_expr(
lhs: e1_reg,
rhs: e2_reg,
target_pc: if_true_label,
jump_if_null: false,
flags: CmpInsFlags::default(),
},
target_register,
if_true_label,
Expand All @@ -685,7 +813,7 @@ pub fn translate_expr(
lhs: e1_reg,
rhs: e2_reg,
target_pc: if_true_label,
jump_if_null: false,
flags: CmpInsFlags::default(),
},
target_register,
if_true_label,
Expand Down Expand Up @@ -756,7 +884,7 @@ pub fn translate_expr(
lhs: base_reg,
rhs: expr_reg,
target_pc: next_case_label,
jump_if_null: false,
flags: CmpInsFlags::default(),
}),
// CASE WHEN 0 THEN 0 ELSE 1 becomes ifnot 0 branch to next clause
None => program.emit_insn(Insn::IfNot {
Expand Down
6 changes: 3 additions & 3 deletions core/translate/main_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::{
translate::result_row::emit_select_result,
vdbe::{
builder::{CursorType, ProgramBuilder},
insn::Insn,
insn::{CmpInsFlags, Insn},
BranchOffset,
},
Result,
Expand Down Expand Up @@ -477,7 +477,7 @@ pub fn open_loop(
lhs: rowid_reg,
rhs: cmp_reg,
target_pc: loop_end,
jump_if_null: false,
flags: CmpInsFlags::default(),
});
}
}
Expand All @@ -499,7 +499,7 @@ pub fn open_loop(
lhs: rowid_reg,
rhs: cmp_reg,
target_pc: loop_end,
jump_if_null: false,
flags: CmpInsFlags::default(),
});
}
}
Expand Down
2 changes: 1 addition & 1 deletion core/vdbe/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ impl ProgramBuilder {
Insn::IdxGT { target_pc, .. } => {
resolve(target_pc, "IdxGT");
}
Insn::IsNull { src: _, target_pc } => {
Insn::IsNull { reg: _, target_pc } => {
resolve(target_pc, "IsNull");
}
_ => continue,
Expand Down
Loading

0 comments on commit 20d3399

Please sign in to comment.