Preserve BETWEEN range constraints without LHS re-eval#5411
Preserve BETWEEN range constraints without LHS re-eval#5411kumarUjjawal wants to merge 4 commits intotursodatabase:mainfrom
Conversation
|
Few Thoughts:
Any feedback on this would be much appreciated. |
| } | ||
| ast::Expr::Between { .. } => { | ||
| crate::bail_parse_error!("BETWEEN expression should have been rewritten in optmizer") | ||
| ast::Expr::Between { |
There was a problem hiding this comment.
any way to share code between this and translate_expr?
core/translate/expr.rs
Outdated
| program.reset_collation(); | ||
|
|
||
| // Resolve collation for the first comparison (start vs lhs). | ||
| let lower_collation_ctx = resolve_collation_ctx(start_collation_ctx, lhs_collation_ctx); |
There was a problem hiding this comment.
can get_collseq_from_expr be used for this?
There was a problem hiding this comment.
get_collseq_from_expr returns only a CollationSeq and loses the “explicit COLLATE vs column-derived” signal used to implement SQLite’s precedence rules.
core/translate/expr.rs
Outdated
| ast::Expr::Binary(Box::new(lower), ast::Operator::And, Box::new(upper)) | ||
| }; | ||
| } | ||
| // Between is handled natively in translate_expr/translate_condition_expr, |
There was a problem hiding this comment.
This comment doesn't need to exist
| ( | ||
| self.operator | ||
| .as_ast_operator() | ||
| .expect("expected an ast operator for BETWEEN constraint"), |
| if let ast::Expr::Between { lhs, .. } = &where_term.expr { | ||
| lhs.as_ref() | ||
| } else { | ||
| panic!("Expected a valid binary expression"); |
There was a problem hiding this comment.
Why are we deconstructing &where_term.expr twice? What is this panic message?
| let defer_cross_table_constraints = | ||
| hash_join_build_only_tables.contains(&table_idx); | ||
| let mut used_constraints_per_term: HashMap<usize, usize> = HashMap::default(); | ||
| for cref in constraint_refs.iter() { |
There was a problem hiding this comment.
what is being done here?
There was a problem hiding this comment.
We track how many constraints from each WHERE term are used so we only consume the term once (and only if both bounds are used). Added comments for explaination.
core/translate/optimizer/mod.rs
Outdated
| "trying to consume a where clause term twice: {where_term:?}", | ||
| ); | ||
| if where_term.consumed { | ||
| if matches!(where_term.expr, ast::Expr::Between { .. }) { |
There was a problem hiding this comment.
why this specialcasing?
There was a problem hiding this comment.
it prevents double-consumption of the same WHERE term and avoids dropping the predicate when only one bound is used by the index.
8f14201 to
0cf6237
Compare
Description
Motivation and context
BETWEEN previously rewrote to two binary comparisons, which duplicated the LHS expression and could re-run expensive subqueries per row. The new translation evaluates LHS once, but we still need optimizer-level range constraints for SQLite compatibility and index usage. This change restores range planning while preserving single-evaluation semantics, and adds
a regression snapshot test.
Closes #5152
Description of AI Usage
Had a pair programming session with codex, the pr body is also written by codex.