diff --git a/core/translate/select.rs b/core/translate/select.rs index 7c01edd9e..38e6e32d9 100644 --- a/core/translate/select.rs +++ b/core/translate/select.rs @@ -578,7 +578,8 @@ fn translate_tables_end(program: &mut ProgramBuilder, loops: &[LoopInfo]) { // iterate in reverse order as we open cursors in order for table_loop in loops.iter().rev() { let cursor_id = table_loop.open_cursor; - if let Plan::Scan { .. } = table_loop.plan { + if let Plan::Scan = table_loop.plan { + // If we're scanning a table, we need to emit a Next instruction to fetch the next row. program.resolve_label(table_loop.next_row_label, program.offset()); program.emit_insn(Insn::NextAsync { cursor_id }); program.emit_insn_with_label_dependency( @@ -613,7 +614,7 @@ fn translate_table_open_cursor( }); program.emit_insn(Insn::OpenReadAwait); - let has_indexable_where_term = w.terms.iter().any(|term| { + let has_where_term_where_rowid_index_usable = w.terms.iter().any(|term| { matches!( term.expr, WhereExpr::SeekRowid(SeekRowid { table: t, .. }) if *t == table.identifier @@ -621,7 +622,7 @@ fn translate_table_open_cursor( }); LoopInfo { identifier: table.identifier.clone(), - plan: if has_indexable_where_term { + plan: if has_where_term_where_rowid_index_usable { Plan::SeekRowid } else { Plan::Scan @@ -712,10 +713,22 @@ fn translate_table_open_loop( early_terminate_label: BranchOffset, ) -> Result<()> { if let Some(left_join) = loop_info.left_join_maybe.as_ref() { + // In a left join loop, initialize the left join match flag to false + // If the condition checks pass, it will eventually be set to true + // If not, NULLs will be emitted for the right table for this row in the outer table. left_join_match_flag_initialize(program, left_join); } if let Plan::Scan = loop_info.plan { + // If we're scanning, we need to rewind the cursor to the beginning of the table + // before we start processing the rows in the loop. + // Consider a nested loop query like: + // SELECT * FROM a JOIN b ON a.someprop = b.someprop; + // We need to rewind the cursor to the beginning of b for each row in a, + // so that we can iterate over all rows in b for each row in a. + // + // If we're not scanning, we're seeking by rowid, so we don't need to rewind the cursor, + // since we're only going to be reading one row. program.emit_insn(Insn::RewindAsync { cursor_id: loop_info.open_cursor, }); diff --git a/core/translate/where_clause.rs b/core/translate/where_clause.rs index 22fc4296c..7456b1d2b 100644 --- a/core/translate/where_clause.rs +++ b/core/translate/where_clause.rs @@ -14,7 +14,7 @@ use sqlite3_parser::ast::{self}; #[derive(Debug)] pub struct SeekRowid<'a> { pub table: &'a String, - pub rowid: &'a ast::Expr, + pub rowid_expr: &'a ast::Expr, } #[derive(Debug)] @@ -57,22 +57,22 @@ pub fn split_constraint_to_terms<'a>( } expr => { if expr.is_always_true()? { + // Terms that are always true can be skipped, as they don't constrain the result set in any way. continue; } let term = WhereTerm { expr: { - let seekrowid_candidate = select.src_tables.iter().rev().find_map(|t| { - let indexable = expr - .check_seekrowid_candidate(&t.identifier, select) - .unwrap_or(None); - indexable - }); + let seekrowid_candidate = select + .src_tables + .iter() + .rev() + .find_map(|t| { + expr.check_seekrowid_candidate(&t.identifier, select) + .unwrap_or(None) + }) + .map(|s| WhereExpr::SeekRowid(s)); - if let Some(seekrowid) = seekrowid_candidate { - WhereExpr::SeekRowid(seekrowid) - } else { - WhereExpr::Expr(expr) - } + seekrowid_candidate.unwrap_or(WhereExpr::Expr(expr)) }, evaluate_at_tbl: match outer_join_table_name { Some(table) => { @@ -110,7 +110,11 @@ pub fn split_constraint_to_terms<'a>( // E.g. 'SELECT * FROM t1 JOIN t2 ON false' can be evaluated at the cursor for t1. let table_refs = introspect_expression_for_table_refs(select, expr)?; - // Get the innermost loop that matches any table_refs, and if not found, fall back to outermost loop + // Get the innermost loop that matches any table_refs, and if not found, fall back to outermost loop. + // The reasoning is that if a condition depends on a table, it should be evaluated at the cursor for that table, + // and if it depends on multiple tables, it should be evaluated inside the innermost loop that contains all of them. + // If a condition doesn't depend on any table, it doesn't functionally matter where it is evaluated, but + // the most efficient place to evaluate it is at the outermost loop. let tbl = processed_where_clause @@ -181,7 +185,7 @@ pub fn process_where<'a>(select: &'a Select) -> Result> } // Sort the terms by the loop_order. - // Inside a loop, sort by indexable_by, so that we can evaluate the most selective terms first. + // Inside a loop, put the most selective terms first, which currently means SeekRowid terms first (since we literally narrow down to a single row). wc.terms.sort_by(|a, b| { let a_loop = wc @@ -321,7 +325,13 @@ pub fn translate_processed_where<'a>( } WhereExpr::SeekRowid(s) => { let computed_rowid_reg = program.alloc_register(); - let _ = translate_expr(program, select, s.rowid, computed_rowid_reg, cursor_hint)?; + let _ = translate_expr( + program, + select, + s.rowid_expr, + computed_rowid_reg, + cursor_hint, + )?; program.emit_insn_with_label_dependency( Insn::SeekRowid { cursor_id: current_loop.open_cursor, @@ -882,6 +892,7 @@ pub enum ConstantCondition { } pub trait Evaluatable<'a> { + // Returns the constant condition if the expression is a constant expression e.g. '1' fn check_constant(&self) -> Result>; fn is_always_true(&self) -> Result { Ok(self @@ -893,8 +904,10 @@ pub trait Evaluatable<'a> { .check_constant()? .map_or(false, |c| c == ConstantCondition::AlwaysFalse)) } - fn is_primary_key_of_table(&self, select: &'a Select) -> Result>; + // Returns the table identifier if the expression is the primary key of a table + fn check_primary_key(&self, select: &'a Select) -> Result>; fn references_positive_integer(&self, select: &'a Select) -> bool; + // Checks if the expression is a candidate for seekrowid optimization fn check_seekrowid_candidate( &'a self, table_identifier: &'a String, @@ -909,11 +922,11 @@ impl<'a> Evaluatable<'a> for ast::Expr { n.parse::().map_or(false, |n| n > 0) } expr => expr - .is_primary_key_of_table(select) + .check_primary_key(select) .map_or(false, |t| t.is_some()), } } - fn is_primary_key_of_table(&self, select: &'a Select) -> Result> { + fn check_primary_key(&self, select: &'a Select) -> Result> { match self { ast::Expr::Id(ident) => { let ident = normalize_ident(&ident.0); @@ -972,20 +985,20 @@ impl<'a> Evaluatable<'a> for ast::Expr { let lhs = lhs.as_ref(); let rhs = rhs.as_ref(); - if let Some(table) = lhs.is_primary_key_of_table(select)? { + if let Some(table) = lhs.check_primary_key(select)? { if table == table_identifier && rhs.references_positive_integer(select) { return Ok(Some(SeekRowid { table: table_identifier, - rowid: rhs, + rowid_expr: rhs, })); } } - if let Some(table) = rhs.is_primary_key_of_table(select)? { + if let Some(table) = rhs.check_primary_key(select)? { if table == table_identifier && lhs.references_positive_integer(select) { return Ok(Some(SeekRowid { table: table_identifier, - rowid: lhs, + rowid_expr: lhs, })); } }