aljaz/turso
1
0
Fork 0
mirror of https://github.com/aljazceru/turso.git synced 2025-12-26 20:44:23 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge 'Expr: fix recursive binary operation logic' from Jussi Saurio

Browse Source 
I believe this closes #682
```
limbo> CREATE TABLE proficient_barrett (imaginative_etrebilal BLOB,lovely_wilson BLOB);
INSERT INTO proficient_barrett VALUES (X'656E67726F7373696E675F636861636F', X'776F6E64726F75735F626F75726E65');
limbo> SELECT * FROM proficient_barrett
WHERE (
    (
        (
            (
                imaginative_etrebilal != X'6661766F7261626C655F636F726573'
                OR
                (imaginative_etrebilal > X'656E67726F7373696E675F6368616439')
            )
            AND
            (
                imaginative_etrebilal = X'656E676167696E675F6E6163696F6E616C'
                OR
                TRUE
            )
        )
        OR
        FALSE
    )
    AND
    (
        imaginative_etrebilal > X'656E67726F7373696E675F63686164F6'
        OR
        TRUE
    )
);
engrossing_chaco|wondrous_bourne
```
@PThorpe92 I don't think we need the `parent_op` machinery at all, we
just need to not jump to the `jump_target_when_true` label given by the
parent if we are evaluating the first condition of an AND.
related: https://github.com/tursodatabase/limbo/pull/633

Reviewed-by: Preston Thorpe <cory.pride83@gmail.com>

Closes #698
This commit is contained in:
Pekka Enberg
2025-01-15 18:32:59 +02:00
parent 4fafaba607 84ef8a8951
commit 7c549bc978
4 changed files with 51 additions and 73 deletions
Download Patch File Download Diff File Expand all files Collapse all files

101
core/translate/expr.rs
View File

@@ -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<ast::Operator>,
}
fn emit_cond_jump(program: &mut ProgramBuilder, cond_meta: ConditionMetadata, reg: usize) {
@@ -137,87 +136,53 @@ 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.
let _ = translate_condition_expr(
// 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();
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,
);
let _ = translate_condition_expr(
)?;
program.resolve_label(jump_target_when_true, program.offset());
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();
translate_condition_expr(
program,
referenced_tables,
lhs,
ConditionMetadata {
parent_op: Some(ast::Operator::And),
jump_if_condition_is_true: true,
jump_target_when_false,
..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();
// 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)?;
// if lhs was false, we land here:
program.resolve_label(local_false_label, program.offset());
// 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)?;
}
)?;
program.resolve_label(jump_target_when_false, program.offset());
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)?;

1
core/translate/group_by.rs
View File

@@ -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,
)?;

4
core/translate/main_loop.rs
View File

@@ -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,

18
testing/where.test
View File

@@ -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}