Skip to content

Commit

Permalink
Merge 'translate_condition_expr(): fix cases where 1. we jump on fals…
Browse files Browse the repository at this point in the history 
…e and 2. either operand is NULL' from Jussi Saurio

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
  • Loading branch information
@penberg
penberg committed Jan 20, 2025
2 parents 2cea85c + ce15ad7 commit c27427d
Show file tree
Hide file tree
Showing 8 changed files with 140 additions and 51 deletions.
26 changes: 20 additions & 6 deletions core/translate/expr.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
}
}
Expand All @@ -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,
});
}
}};
Expand Down Expand Up @@ -324,13 +326,15 @@ 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.
program.emit_insn(Insn::Ne {
lhs: lhs_reg,
rhs: rhs_reg,
target_pc: condition_metadata.jump_target_when_false,
jump_if_null: true,
});
}
}
Expand All @@ -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'.
Expand Down Expand Up @@ -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,
});
}
}
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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...
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion core/translate/group_by.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions core/translate/main_loop.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,7 @@ pub fn open_loop(
lhs: rowid_reg,
rhs: cmp_reg,
target_pc: loop_end,
jump_if_null: false,
});
}
}
Expand All @@ -498,6 +499,7 @@ pub fn open_loop(
lhs: rowid_reg,
rhs: cmp_reg,
target_pc: loop_end,
jump_if_null: false,
});
}
}
Expand Down
10 changes: 8 additions & 2 deletions core/vdbe/builder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,55 +158,61 @@ impl ProgramBuilder {
lhs: _lhs,
rhs: _rhs,
target_pc,
..
} => {
resolve(target_pc, "Eq");
}
Insn::Ne {
lhs: _lhs,
rhs: _rhs,
target_pc,
..
} => {
resolve(target_pc, "Ne");
}
Insn::Lt {
lhs: _lhs,
rhs: _rhs,
target_pc,
..
} => {
resolve(target_pc, "Lt");
}
Insn::Le {
lhs: _lhs,
rhs: _rhs,
target_pc,
..
} => {
resolve(target_pc, "Le");
}
Insn::Gt {
lhs: _lhs,
rhs: _rhs,
target_pc,
..
} => {
resolve(target_pc, "Gt");
}
Insn::Ge {
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");
}
Expand Down
14 changes: 10 additions & 4 deletions core/vdbe/explain.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ pub fn insn_to_str(
lhs,
rhs,
target_pc,
..
} => (
"Eq",
*lhs as i32,
Expand All @@ -227,6 +228,7 @@ pub fn insn_to_str(
lhs,
rhs,
target_pc,
..
} => (
"Ne",
*lhs as i32,
Expand All @@ -245,6 +247,7 @@ pub fn insn_to_str(
lhs,
rhs,
target_pc,
..
} => (
"Lt",
*lhs as i32,
Expand All @@ -258,6 +261,7 @@ pub fn insn_to_str(
lhs,
rhs,
target_pc,
..
} => (
"Le",
*lhs as i32,
Expand All @@ -276,6 +280,7 @@ pub fn insn_to_str(
lhs,
rhs,
target_pc,
..
} => (
"Gt",
*lhs as i32,
Expand All @@ -289,6 +294,7 @@ pub fn insn_to_str(
lhs,
rhs,
target_pc,
..
} => (
"Ge",
*lhs as i32,
Expand All @@ -306,25 +312,25 @@ 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()),
),
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()),
Expand Down
28 changes: 22 additions & 6 deletions core/vdbe/insn.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading

0 comments on commit c27427d

Please sign in to comment.