Skip to content

Commit 25b56b5

Browse files
jussisauriojussisaurio
committed
translate_condition_expr(): fix cases where 1. we jump on false and 2. either operand is NULL
1 parent 9369f06 commit 25b56b5

File tree

2 files changed

+188
-31
lines changed

2 files changed

+188
-31
lines changed

‎core/translate/expr.rs

Lines changed: 156 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -33,30 +33,6 @@ fn emit_cond_jump(program: &mut ProgramBuilder, cond_meta: ConditionMetadata, re
3333
});
3434
}
3535
}
36-
macro_rules! emit_cmp_insn {
37-
(
38-
$program:expr,
39-
$cond:expr,
40-
$op_true:ident,
41-
$op_false:ident,
42-
$lhs:expr,
43-
$rhs:expr
44-
) => {{
45-
if $cond.jump_if_condition_is_true {
46-
$program.emit_insn(Insn::$op_true {
47-
lhs: $lhs,
48-
rhs: $rhs,
49-
target_pc: $cond.jump_target_when_true,
50-
});
51-
} else {
52-
$program.emit_insn(Insn::$op_false {
53-
lhs: $lhs,
54-
rhs: $rhs,
55-
target_pc: $cond.jump_target_when_false,
56-
});
57-
}
58-
}};
59-
}
6036

6137
macro_rules! expect_arguments_exact {
6238
(
@@ -144,6 +120,66 @@ macro_rules! expect_arguments_even {
144120
}};
145121
}
146122

123+
/// Wraps a comparison instruction (Eq, Ne, Gt, Lt, Ge, Le) with a null/zero check.
124+
/// This is needed so that when we compare two operands where either one is null,
125+
/// and expect to jump when the condition is false, we can immediately jump to the false target.
126+
///
127+
/// The fundamental raison d'etre of this is that the way our "jump when false" logic works is
128+
/// that it inverts the conditional check, e.g. Eq becomes Ne, Gt becomes Le, etc.
129+
/// and we jump if that inverted condition is _TRUE_.
130+
///
131+
/// However, if either of the operands is null, the comparison will always be false,
132+
/// so the "jump when false" logic will not work as expected.
133+
/// Instead, in those cases we emit a separate instruction that jumps to the false target
134+
/// if either of the operands is null.
135+
///
136+
/// Example where the operator-inversion logic works as expected:
137+
/// SELECT * from users WHERE id = 5 becomes:
138+
/// <SKIP THIS ROW IF id != 5>
139+
/// -> works ok.
140+
///
141+
/// Example where it does not work:
142+
/// SELECT * from users WHERE id = NULL
143+
/// -> becomes:
144+
/// <SKIP THIS ROW IF id != NULL>
145+
/// -> does NOT jump to the false target since "id != NULL" is false,
146+
/// and erroneously continues execution.
147+
///
148+
/// This is why we need to emit a separate instruction that jumps to the false target
149+
/// if either of the operands is null.
150+
fn wrap_cmp_insn_zero_or_null_cond(
151+
program: &mut ProgramBuilder,
152+
lhs: usize,
153+
rhs: usize,
154+
cond_meta: ConditionMetadata,
155+
op_true: Insn,
156+
op_false: Insn,
157+
) {
158+
if cond_meta.jump_if_condition_is_true {
159+
// If checking for true and we have null, the comparison will always be false
160+
// and we won't jump in any case.
161+
program.emit_insn(op_true);
162+
} else {
163+
// Allocate a new register for the null/zero check result
164+
let null_check_reg = program.alloc_register();
165+
166+
// Check if either operand is null/zero
167+
program.emit_insn(Insn::ZeroOrNull {
168+
rg1: lhs,
169+
rg2: rhs,
170+
dest: null_check_reg,
171+
});
172+
// If checking for false and we have null, jump to the false target
173+
// because the comparison is false
174+
program.emit_insn(Insn::IsNull {
175+
src: null_check_reg,
176+
target_pc: cond_meta.jump_target_when_false,
177+
});
178+
// Otherwise emit the false comparison
179+
program.emit_insn(op_false);
180+
}
181+
}
182+
147183
pub fn translate_condition_expr(
148184
program: &mut ProgramBuilder,
149185
referenced_tables: &[TableReference],
@@ -207,22 +243,112 @@ pub fn translate_condition_expr(
207243
let rhs_reg = translate_and_mark(program, Some(referenced_tables), rhs, resolver)?;
208244
match op {
209245
ast::Operator::Greater => {
210-
emit_cmp_insn!(program, condition_metadata, Gt, Le, lhs_reg, rhs_reg)
246+
wrap_cmp_insn_zero_or_null_cond(
247+
program,
248+
lhs_reg,
249+
rhs_reg,
250+
condition_metadata,
251+
Insn::Gt {
252+
lhs: lhs_reg,
253+
rhs: rhs_reg,
254+
target_pc: condition_metadata.jump_target_when_true,
255+
},
256+
Insn::Le {
257+
lhs: lhs_reg,
258+
rhs: rhs_reg,
259+
target_pc: condition_metadata.jump_target_when_false,
260+
},
261+
);
211262
}
212263
ast::Operator::GreaterEquals => {
213-
emit_cmp_insn!(program, condition_metadata, Ge, Lt, lhs_reg, rhs_reg)
264+
wrap_cmp_insn_zero_or_null_cond(
265+
program,
266+
lhs_reg,
267+
rhs_reg,
268+
condition_metadata,
269+
Insn::Ge {
270+
lhs: lhs_reg,
271+
rhs: rhs_reg,
272+
target_pc: condition_metadata.jump_target_when_true,
273+
},
274+
Insn::Lt {
275+
lhs: lhs_reg,
276+
rhs: rhs_reg,
277+
target_pc: condition_metadata.jump_target_when_false,
278+
},
279+
);
214280
}
215281
ast::Operator::Less => {
216-
emit_cmp_insn!(program, condition_metadata, Lt, Ge, lhs_reg, rhs_reg)
282+
wrap_cmp_insn_zero_or_null_cond(
283+
program,
284+
lhs_reg,
285+
rhs_reg,
286+
condition_metadata,
287+
Insn::Lt {
288+
lhs: lhs_reg,
289+
rhs: rhs_reg,
290+
target_pc: condition_metadata.jump_target_when_true,
291+
},
292+
Insn::Ge {
293+
lhs: lhs_reg,
294+
rhs: rhs_reg,
295+
target_pc: condition_metadata.jump_target_when_false,
296+
},
297+
);
217298
}
218299
ast::Operator::LessEquals => {
219-
emit_cmp_insn!(program, condition_metadata, Le, Gt, lhs_reg, rhs_reg)
300+
wrap_cmp_insn_zero_or_null_cond(
301+
program,
302+
lhs_reg,
303+
rhs_reg,
304+
condition_metadata,
305+
Insn::Le {
306+
lhs: lhs_reg,
307+
rhs: rhs_reg,
308+
target_pc: condition_metadata.jump_target_when_true,
309+
},
310+
Insn::Gt {
311+
lhs: lhs_reg,
312+
rhs: rhs_reg,
313+
target_pc: condition_metadata.jump_target_when_false,
314+
},
315+
);
220316
}
221317
ast::Operator::Equals => {
222-
emit_cmp_insn!(program, condition_metadata, Eq, Ne, lhs_reg, rhs_reg)
318+
wrap_cmp_insn_zero_or_null_cond(
319+
program,
320+
lhs_reg,
321+
rhs_reg,
322+
condition_metadata,
323+
Insn::Eq {
324+
lhs: lhs_reg,
325+
rhs: rhs_reg,
326+
target_pc: condition_metadata.jump_target_when_true,
327+
},
328+
Insn::Ne {
329+
lhs: lhs_reg,
330+
rhs: rhs_reg,
331+
target_pc: condition_metadata.jump_target_when_false,
332+
},
333+
);
223334
}
224335
ast::Operator::NotEquals => {
225-
emit_cmp_insn!(program, condition_metadata, Ne, Eq, lhs_reg, rhs_reg)
336+
wrap_cmp_insn_zero_or_null_cond(
337+
program,
338+
lhs_reg,
339+
rhs_reg,
340+
condition_metadata,
341+
Insn::Ne {
342+
lhs: lhs_reg,
343+
rhs: rhs_reg,
344+
target_pc: condition_metadata.jump_target_when_true,
345+
},
346+
Insn::Eq {
347+
lhs: lhs_reg,
348+
rhs: rhs_reg,
349+
target_pc: condition_metadata.jump_target_when_false,
350+
},
351+
);
226352
}
227353
ast::Operator::Is => todo!(),
228354
ast::Operator::IsNot => todo!(),

‎testing/where.test

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -382,4 +382,35 @@ do_execsql_test nested-parens-and-inside-or-regression-test {
382382
AND
383383
(id = 6 OR FALSE)
384384
);
385-
} {1}
385+
} {1}
386+
387+
# Regression tests for binary conditional jump comparisons where one operand is null
388+
do_execsql_test where-binary-one-operand-null-1 {
389+
select * from users where first_name = NULL;
390+
} {}
391+
392+
do_execsql_test where-binary-one-operand-null-2 {
393+
select first_name from users where first_name = NULL OR id = 1;
394+
} {Jamie}
395+
396+
do_execsql_test where-binary-one-operand-null-3 {
397+
select first_name from users where first_name = NULL AND id = 1;
398+
} {}
399+
400+
do_execsql_test where-binary-one-operand-null-4 {
401+
select first_name from users where NULL = first_name;
402+
} {}
403+
404+
do_execsql_test where-binary-one-operand-null-5 {
405+
select first_name from users where NULL = first_name OR id = 1;
406+
} {Jamie}
407+
408+
do_execsql_test where-binary-one-operand-null-6 {
409+
select first_name from users where NULL = first_name AND id = 1;
410+
} {}
411+
412+
413+
414+
415+
416+

0 commit comments

Comments
 (0)