From f8b3b06163715a858a3952cb68a060b04b57d9b8 Mon Sep 17 00:00:00 2001 From: Jussi Saurio Date: Wed, 15 Jan 2025 14:12:08 +0200 Subject: [PATCH 1/2] Expr: fix recursive binary operation logic --- core/translate/expr.rs | 82 +++++++++---------------------------- core/translate/group_by.rs | 1 - core/translate/main_loop.rs | 4 -- testing/where.test | 18 ++++++++ 4 files changed, 38 insertions(+), 67 deletions(-) diff --git a/core/translate/expr.rs b/core/translate/expr.rs index 4a0677d8d..28e21e81f 100644 --- a/core/translate/expr.rs +++ b/core/translate/expr.rs @@ -16,7 +16,6 @@ pub struct ConditionMetadata { pub jump_if_condition_is_true: bool, pub jump_target_when_true: BranchOffset, pub jump_target_when_false: BranchOffset, - pub parent_op: Option, } fn emit_cond_jump(program: &mut ProgramBuilder, cond_meta: ConditionMetadata, reg: usize) { @@ -137,87 +136,46 @@ pub fn translate_condition_expr( match expr { ast::Expr::Between { .. } => todo!(), ast::Expr::Binary(lhs, ast::Operator::And, rhs) => { - // In a binary AND, never jump to the 'jump_target_when_true' label on the first condition, because - // the second condition must also be true. + // In a binary AND, never jump to the parent 'jump_target_when_true' label on the first condition, because + // the second condition must also be true. Instead we instruct the child expression to jump to a local + // true label. + let jump_target_when_true = program.allocate_label(); let _ = translate_condition_expr( program, referenced_tables, lhs, ConditionMetadata { - jump_if_condition_is_true: false, - // Mark that the parent op for sub-expressions is AND - parent_op: Some(ast::Operator::And), + jump_target_when_true, ..condition_metadata }, resolver, ); + program.resolve_label(jump_target_when_true, program.offset()); let _ = translate_condition_expr( program, referenced_tables, rhs, - ConditionMetadata { - parent_op: Some(ast::Operator::And), - ..condition_metadata - }, + condition_metadata, resolver, ); } ast::Expr::Binary(lhs, ast::Operator::Or, rhs) => { - if matches!(condition_metadata.parent_op, Some(ast::Operator::And)) { - // we are inside a bigger AND expression, so we do NOT jump to parent's 'true' if LHS or RHS is true. - // we only short-circuit the parent's false label if LHS and RHS are both false. - let local_true_label = program.allocate_label(); - let local_false_label = program.allocate_label(); + let jump_target_when_false = program.allocate_label(); - // evaluate LHS in normal OR fashion, short-circuit local if true - let lhs_metadata = ConditionMetadata { - jump_if_condition_is_true: true, - jump_target_when_true: local_true_label, - jump_target_when_false: local_false_label, - parent_op: Some(ast::Operator::Or), - }; - translate_condition_expr(program, referenced_tables, lhs, lhs_metadata, resolver)?; + let lhs_metadata = ConditionMetadata { + jump_if_condition_is_true: true, + jump_target_when_false, + ..condition_metadata + }; - // if lhs was false, we land here: - program.resolve_label(local_false_label, program.offset()); + translate_condition_expr(program, referenced_tables, lhs, lhs_metadata, resolver)?; - // evaluate rhs with normal OR: short-circuit if true, go to local_true - let rhs_metadata = ConditionMetadata { - jump_if_condition_is_true: true, - jump_target_when_true: local_true_label, - jump_target_when_false: condition_metadata.jump_target_when_false, - // if rhs is also false => parent's false - parent_op: Some(ast::Operator::Or), - }; - translate_condition_expr(program, referenced_tables, rhs, rhs_metadata, resolver)?; - - // if we get here, both lhs+rhs are false: explicit jump to parent's false - program.emit_insn(Insn::Goto { - target_pc: condition_metadata.jump_target_when_false, - }); - // local_true: we do not jump to parent's "true" label because the parent is AND, - // so we want to keep evaluating the rest - program.resolve_label(local_true_label, program.offset()); - } else { - let jump_target_when_false = program.allocate_label(); - - let lhs_metadata = ConditionMetadata { - jump_if_condition_is_true: true, - jump_target_when_false, - parent_op: Some(ast::Operator::Or), - ..condition_metadata - }; - - translate_condition_expr(program, referenced_tables, lhs, lhs_metadata, resolver)?; - - // if LHS was false, we land here: - program.resolve_label(jump_target_when_false, program.offset()); - let rhs_metadata = ConditionMetadata { - parent_op: Some(ast::Operator::Or), - ..condition_metadata - }; - translate_condition_expr(program, referenced_tables, rhs, rhs_metadata, resolver)?; - } + // if LHS was false, we land here: + program.resolve_label(jump_target_when_false, program.offset()); + let rhs_metadata = ConditionMetadata { + ..condition_metadata + }; + translate_condition_expr(program, referenced_tables, rhs, rhs_metadata, resolver)?; } ast::Expr::Binary(lhs, op, rhs) => { let lhs_reg = translate_and_mark(program, Some(referenced_tables), lhs, resolver)?; diff --git a/core/translate/group_by.rs b/core/translate/group_by.rs index 2b6e7afb3..5855caa06 100644 --- a/core/translate/group_by.rs +++ b/core/translate/group_by.rs @@ -386,7 +386,6 @@ pub fn emit_group_by<'a>( jump_if_condition_is_true: false, jump_target_when_false: group_by_end_without_emitting_row_label, jump_target_when_true: BranchOffset::Placeholder, // not used. FIXME: this is a bug. HAVING can have e.g. HAVING a OR b. - parent_op: None, }, &t_ctx.resolver, )?; diff --git a/core/translate/main_loop.rs b/core/translate/main_loop.rs index 33f76a84e..a19fe66f7 100644 --- a/core/translate/main_loop.rs +++ b/core/translate/main_loop.rs @@ -230,7 +230,6 @@ pub fn open_loop( jump_if_condition_is_true: false, jump_target_when_true, jump_target_when_false: next, - parent_op: None, }; translate_condition_expr( program, @@ -279,7 +278,6 @@ pub fn open_loop( jump_if_condition_is_true: false, jump_target_when_true, jump_target_when_false, - parent_op: None, }; for predicate in predicates.iter() { translate_condition_expr( @@ -352,7 +350,6 @@ pub fn open_loop( jump_if_condition_is_true: false, jump_target_when_true, jump_target_when_false: next, - parent_op: None, }; translate_condition_expr( program, @@ -537,7 +534,6 @@ pub fn open_loop( jump_if_condition_is_true: false, jump_target_when_true, jump_target_when_false: next, - parent_op: None, }; translate_condition_expr( program, diff --git a/testing/where.test b/testing/where.test index c613f784b..b3154cd7c 100755 --- a/testing/where.test +++ b/testing/where.test @@ -365,3 +365,21 @@ do_execsql_test nested-parens-conditionals-and-double-or { 8171|Andrea|Lee|dgarrison@example.com|001-594-430-0646|452 Anthony Stravenue|Sandraville|CA|28572|12 9110|Anthony|Barrett|steven05@example.net|(562)928-9177x8454|86166 Foster Inlet Apt. 284|North Jeffreyburgh|CA|80147|97 9279|Annette|Lynn|joanne37@example.com|(272)700-7181|2676 Laura Points Apt. 683|Tristanville|NY|48646|91}} + +# Regression test for nested parens + OR + AND. This returned 0 rows before the fix. +# It should always return 1 row because it is true for id = 6. +do_execsql_test nested-parens-and-inside-or-regression-test { + SELECT count(1) FROM users + WHERE ( + ( + ( + (id != 5) + AND + (id = 5 OR TRUE) + ) + OR FALSE + ) + AND + (id = 6 OR FALSE) + ); +} {1} \ No newline at end of file From 84ef8a8951b847429dbf9ec559a8a733de6dc0a5 Mon Sep 17 00:00:00 2001 From: Jussi Saurio Date: Wed, 15 Jan 2025 14:24:40 +0200 Subject: [PATCH 2/2] translate_condition_expr(): unify how the AND and OR cases look --- core/translate/expr.rs | 45 ++++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/core/translate/expr.rs b/core/translate/expr.rs index 28e21e81f..8886f704a 100644 --- a/core/translate/expr.rs +++ b/core/translate/expr.rs @@ -137,10 +137,10 @@ pub fn translate_condition_expr( ast::Expr::Between { .. } => todo!(), ast::Expr::Binary(lhs, ast::Operator::And, rhs) => { // In a binary AND, never jump to the parent 'jump_target_when_true' label on the first condition, because - // the second condition must also be true. Instead we instruct the child expression to jump to a local + // the second condition MUST also be true. Instead we instruct the child expression to jump to a local // true label. let jump_target_when_true = program.allocate_label(); - let _ = translate_condition_expr( + translate_condition_expr( program, referenced_tables, lhs, @@ -149,33 +149,40 @@ pub fn translate_condition_expr( ..condition_metadata }, resolver, - ); + )?; program.resolve_label(jump_target_when_true, program.offset()); - let _ = translate_condition_expr( + translate_condition_expr( program, referenced_tables, rhs, condition_metadata, resolver, - ); + )?; } ast::Expr::Binary(lhs, ast::Operator::Or, rhs) => { + // In a binary OR, never jump to the parent 'jump_target_when_false' label on the first condition, because + // the second condition CAN also be true. Instead we instruct the child expression to jump to a local + // false label. let jump_target_when_false = program.allocate_label(); - - let lhs_metadata = ConditionMetadata { - jump_if_condition_is_true: true, - jump_target_when_false, - ..condition_metadata - }; - - translate_condition_expr(program, referenced_tables, lhs, lhs_metadata, resolver)?; - - // if LHS was false, we land here: + translate_condition_expr( + program, + referenced_tables, + lhs, + ConditionMetadata { + jump_if_condition_is_true: true, + jump_target_when_false, + ..condition_metadata + }, + resolver, + )?; program.resolve_label(jump_target_when_false, program.offset()); - let rhs_metadata = ConditionMetadata { - ..condition_metadata - }; - translate_condition_expr(program, referenced_tables, rhs, rhs_metadata, resolver)?; + translate_condition_expr( + program, + referenced_tables, + rhs, + condition_metadata, + resolver, + )?; } ast::Expr::Binary(lhs, op, rhs) => { let lhs_reg = translate_and_mark(program, Some(referenced_tables), lhs, resolver)?;