From 8fb4fbf8af8345fe5498b383ddd415de67ed414f Mon Sep 17 00:00:00 2001 From: Piotr Rzysko Date: Sun, 3 Aug 2025 12:24:30 +0200 Subject: [PATCH] Make WhereTerm::consumed a plain bool Now that virtual tables are integrated into the optimizer, this field no longer needs to be wrapped in Cell. --- core/translate/optimizer/join.rs | 16 +++++----- .../optimizer/lift_common_subexpressions.rs | 30 +++++++++---------- core/translate/optimizer/mod.rs | 14 ++++----- core/translate/plan.rs | 12 +++----- core/translate/planner.rs | 7 ++--- core/translate/select.rs | 3 +- 6 files changed, 35 insertions(+), 47 deletions(-) diff --git a/core/translate/optimizer/join.rs b/core/translate/optimizer/join.rs index 88ae71dbf..4518a6cd5 100644 --- a/core/translate/optimizer/join.rs +++ b/core/translate/optimizer/join.rs @@ -501,7 +501,7 @@ fn generate_join_bitmasks(table_number_max_exclusive: usize, how_many: usize) -> #[cfg(test)] mod tests { - use std::{cell::Cell, sync::Arc}; + use std::sync::Arc; use turso_sqlite3_parser::ast::{self, Expr, Operator, SortOrder, TableInternalId}; @@ -1344,7 +1344,7 @@ mod tests { Box::new(Expr::Literal(ast::Literal::Numeric(5.to_string()))), ), from_outer_join: None, - consumed: Cell::new(false), + consumed: false, }]; let table_references = TableReferences::new(joined_tables, vec![]); @@ -1436,7 +1436,7 @@ mod tests { Box::new(Expr::Literal(ast::Literal::Numeric(5.to_string()))), ), from_outer_join: None, - consumed: Cell::new(false), + consumed: false, }, WhereTerm { expr: Expr::Binary( @@ -1450,7 +1450,7 @@ mod tests { Box::new(Expr::Literal(ast::Literal::Numeric(7.to_string()))), ), from_outer_join: None, - consumed: Cell::new(false), + consumed: false, }, ]; @@ -1547,7 +1547,7 @@ mod tests { Box::new(Expr::Literal(ast::Literal::Numeric(5.to_string()))), ), from_outer_join: None, - consumed: Cell::new(false), + consumed: false, }, WhereTerm { expr: Expr::Binary( @@ -1561,7 +1561,7 @@ mod tests { Box::new(Expr::Literal(ast::Literal::Numeric(10.to_string()))), ), from_outer_join: None, - consumed: Cell::new(false), + consumed: false, }, WhereTerm { expr: Expr::Binary( @@ -1575,7 +1575,7 @@ mod tests { Box::new(Expr::Literal(ast::Literal::Numeric(7.to_string()))), ), from_outer_join: None, - consumed: Cell::new(false), + consumed: false, }, ]; @@ -1692,7 +1692,7 @@ mod tests { WhereTerm { expr: Expr::Binary(Box::new(lhs), op, Box::new(rhs)), from_outer_join: None, - consumed: Cell::new(false), + consumed: false, } } diff --git a/core/translate/optimizer/lift_common_subexpressions.rs b/core/translate/optimizer/lift_common_subexpressions.rs index c6684a950..dda4ee0e3 100644 --- a/core/translate/optimizer/lift_common_subexpressions.rs +++ b/core/translate/optimizer/lift_common_subexpressions.rs @@ -1,5 +1,3 @@ -use std::cell::Cell; - use turso_sqlite3_parser::ast::{Expr, Operator}; use crate::{ @@ -115,7 +113,7 @@ pub(crate) fn lift_common_subexpressions_from_binary_or_terms( if found_non_empty_or_branches { // If we found an empty OR branch, we can remove the entire OR term. // E.g. (a AND b) OR (a) OR (a AND c) just becomes a. - where_clause[i].consumed.set(true); + where_clause[i].consumed = true; } else { assert!(new_or_operands_for_original_term.len() > 1); // Update the original WhereTerm's expression with the new OR structure (without common parts). @@ -127,7 +125,7 @@ pub(crate) fn lift_common_subexpressions_from_binary_or_terms( where_clause.push(WhereTerm { expr: common_expr_to_add, from_outer_join: term_from_outer_join, - consumed: Cell::new(false), + consumed: false, }); } @@ -258,7 +256,7 @@ mod tests { let mut where_clause = vec![WhereTerm { expr: or_expr, from_outer_join: None, - consumed: Cell::new(false), + consumed: false, }]; lift_common_subexpressions_from_binary_or_terms(&mut where_clause)?; @@ -269,7 +267,7 @@ mod tests { // 3. b = 1 let nonconsumed_terms = where_clause .iter() - .filter(|term| !term.consumed.get()) + .filter(|term| !term.consumed) .collect::>(); assert_eq!(nonconsumed_terms.len(), 3); assert_eq!( @@ -357,7 +355,7 @@ mod tests { let mut where_clause = vec![WhereTerm { expr: or_expr, from_outer_join: None, - consumed: Cell::new(false), + consumed: false, }]; lift_common_subexpressions_from_binary_or_terms(&mut where_clause)?; @@ -367,7 +365,7 @@ mod tests { // 2. a = 1 let nonconsumed_terms = where_clause .iter() - .filter(|term| !term.consumed.get()) + .filter(|term| !term.consumed) .collect::>(); assert_eq!(nonconsumed_terms.len(), 2); assert_eq!( @@ -424,7 +422,7 @@ mod tests { let mut where_clause = vec![WhereTerm { expr: or_expr.clone(), from_outer_join: None, - consumed: Cell::new(false), + consumed: false, }]; lift_common_subexpressions_from_binary_or_terms(&mut where_clause)?; @@ -432,7 +430,7 @@ mod tests { // Should remain unchanged since no common terms let nonconsumed_terms = where_clause .iter() - .filter(|term| !term.consumed.get()) + .filter(|term| !term.consumed) .collect::>(); assert_eq!(nonconsumed_terms.len(), 1); assert_eq!(nonconsumed_terms[0].expr, or_expr); @@ -491,7 +489,7 @@ mod tests { let mut where_clause = vec![WhereTerm { expr: or_expr, from_outer_join: Some(TableInternalId::default()), // Set from_outer_join - consumed: Cell::new(false), + consumed: false, }]; lift_common_subexpressions_from_binary_or_terms(&mut where_clause)?; @@ -499,7 +497,7 @@ mod tests { // Should have 2 terms, both with from_outer_join set let nonconsumed_terms = where_clause .iter() - .filter(|term| !term.consumed.get()) + .filter(|term| !term.consumed) .collect::>(); assert_eq!(nonconsumed_terms.len(), 2); assert_eq!( @@ -543,7 +541,7 @@ mod tests { let mut where_clause = vec![WhereTerm { expr: single_expr.clone(), from_outer_join: None, - consumed: Cell::new(false), + consumed: false, }]; lift_common_subexpressions_from_binary_or_terms(&mut where_clause)?; @@ -551,7 +549,7 @@ mod tests { // Should remain unchanged let nonconsumed_terms = where_clause .iter() - .filter(|term| !term.consumed.get()) + .filter(|term| !term.consumed) .collect::>(); assert_eq!(nonconsumed_terms.len(), 1); assert_eq!(nonconsumed_terms[0].expr, single_expr); @@ -596,14 +594,14 @@ mod tests { let mut where_clause = vec![WhereTerm { expr: or_expr, from_outer_join: None, - consumed: Cell::new(false), + consumed: false, }]; lift_common_subexpressions_from_binary_or_terms(&mut where_clause)?; let nonconsumed_terms = where_clause .iter() - .filter(|term| !term.consumed.get()) + .filter(|term| !term.consumed) .collect::>(); assert_eq!(nonconsumed_terms.len(), 1); assert_eq!(nonconsumed_terms[0].expr, a_expr); diff --git a/core/translate/optimizer/mod.rs b/core/translate/optimizer/mod.rs index fd54fe510..31a2b9579 100644 --- a/core/translate/optimizer/mod.rs +++ b/core/translate/optimizer/mod.rs @@ -319,13 +319,11 @@ fn optimize_table_access( let constraint = &constraints_per_table[table_idx].constraints[cref.constraint_vec_pos]; assert!( - !where_clause[constraint.where_clause_pos.0].consumed.get(), + !where_clause[constraint.where_clause_pos.0].consumed, "trying to consume a where clause term twice: {:?}", where_clause[constraint.where_clause_pos.0] ); - where_clause[constraint.where_clause_pos.0] - .consumed - .set(true); + where_clause[constraint.where_clause_pos.0].consumed = true; } if let Some(index) = &index { joined_tables[table_idx].op = Operation::Search(Search::Seek { @@ -428,9 +426,7 @@ fn build_vtab_scan_op( let (pred_idx, _) = vtab_constraint.unpack_plan_info(); let constraint = &table_constraints.constraints[pred_idx]; if usage.omit { - where_clause[constraint.where_clause_pos.0] - .consumed - .set(true); + where_clause[constraint.where_clause_pos.0].consumed = true; } let expr = constraint.get_constraining_expr(where_clause); constraints[zero_based_argv_index] = Some(expr); @@ -476,7 +472,7 @@ fn eliminate_constant_conditions( let predicate = &where_clause[i]; if predicate.expr.is_always_true()? { // true predicates can be removed since they don't affect the result - where_clause[i].consumed.set(true); + where_clause[i].consumed = true; i += 1; } else if predicate.expr.is_always_false()? { // any false predicate in a list of conjuncts (AND-ed predicates) will make the whole list false, @@ -487,7 +483,7 @@ fn eliminate_constant_conditions( } where_clause .iter_mut() - .for_each(|term| term.consumed.set(true)); + .for_each(|term| term.consumed = true); return Ok(ConstantConditionEliminationResult::ImpossibleCondition); } else { i += 1; diff --git a/core/translate/plan.rs b/core/translate/plan.rs index aab415519..9c05b7fcc 100644 --- a/core/translate/plan.rs +++ b/core/translate/plan.rs @@ -1,4 +1,4 @@ -use std::{cell::Cell, cmp::Ordering, sync::Arc}; +use std::{cmp::Ordering, sync::Arc}; use turso_sqlite3_parser::ast::{self, SortOrder}; use crate::{ @@ -84,16 +84,12 @@ pub struct WhereTerm { /// A term may have been consumed e.g. if: /// - it has been converted into a constraint in a seek key /// - it has been removed due to being trivially true or false - /// - /// FIXME: this can be made into a simple `bool` once we move the virtual table constraint resolution - /// code out of `init_loop()`, because that's the only place that requires a mutable reference to the where clause - /// that causes problems to other code that needs immutable references to the where clause. - pub consumed: Cell, + pub consumed: bool, } impl WhereTerm { pub fn should_eval_before_loop(&self, join_order: &[JoinOrderMember]) -> bool { - if self.consumed.get() { + if self.consumed { return false; } let Ok(eval_at) = self.eval_at(join_order) else { @@ -103,7 +99,7 @@ impl WhereTerm { } pub fn should_eval_at_loop(&self, loop_idx: usize, join_order: &[JoinOrderMember]) -> bool { - if self.consumed.get() { + if self.consumed { return false; } let Ok(eval_at) = self.eval_at(join_order) else { diff --git a/core/translate/planner.rs b/core/translate/planner.rs index e8d2eaa58..ee99b5c53 100644 --- a/core/translate/planner.rs +++ b/core/translate/planner.rs @@ -1,4 +1,3 @@ -use std::cell::Cell; use std::sync::Arc; use super::{ @@ -662,7 +661,7 @@ pub fn parse_where( out_where_clause.push(WhereTerm { expr, from_outer_join: None, - consumed: Cell::new(false), + consumed: false, }); } Ok(()) @@ -950,7 +949,7 @@ fn parse_join( } else { None }, - consumed: Cell::new(false), + consumed: false, }); } } @@ -1031,7 +1030,7 @@ fn parse_join( } else { None }, - consumed: Cell::new(false), + consumed: false, }); } using = Some(distinct_names); diff --git a/core/translate/select.rs b/core/translate/select.rs index 35ffa50d2..ae1079bef 100644 --- a/core/translate/select.rs +++ b/core/translate/select.rs @@ -16,7 +16,6 @@ use crate::vdbe::builder::{ProgramBuilderOpts, TableRefIdCounter}; use crate::vdbe::insn::Insn; use crate::{schema::Schema, vdbe::builder::ProgramBuilder, Result}; use crate::{Connection, SymbolTable}; -use std::cell::Cell; use std::sync::Arc; use turso_sqlite3_parser::ast::{self, CompoundSelect, Expr, SortOrder}; use turso_sqlite3_parser::ast::{ResultColumn, SelectInner}; @@ -661,7 +660,7 @@ fn add_vtab_predicates_to_where_clause( plan.where_clause.push(WhereTerm { expr, from_outer_join: None, - consumed: Cell::new(false), + consumed: false, }); } Ok(())