fix: BETWEEN optimization now triggers by comparing expr kind, not span#5738
fix: BETWEEN optimization now triggers by comparing expr kind, not span#5738
Conversation
prql-bot
left a comment
There was a problem hiding this comment.
Both fixes are correct and well-tested.
BETWEEN optimization: The root cause is right — rq::Expr derives PartialEq over both kind and span, so the same column at two source positions always compared unequal. Comparing .kind only is the correct fix.
SQLite div_i: The old formula ROUND(ABS(l/r) - 0.5) fails when |l| < |r| because SQLite integer division yields 0, then ROUND(0 - 0.5) = -1 (SQLite rounds .5 away from zero). The new CAST(ABS(l * 1.0 / r) AS INTEGER) formula correctly truncates toward zero.
Note: PR #5736 (also by @queelius) contains only the SQLite div_i fix from this PR. One of the two should be closed to avoid merge conflicts.
CI failure: PR title needs a conventional commit prefix (e.g. 'fix: ...'). Also measure-code-cov failed — investigating.
prql-bot
left a comment
There was a problem hiding this comment.
CI issues:
-
Validate PR title: The title needs a conventional commit prefix. Should be:
fix: BETWEEN optimization never triggering due to span comparison -
measure-code-cov:
cargo llvm-covfailed — this may be a flake or may be related to the snapshot change inarithmetic.snap(the removedsnapshot_kind: textline). Worth checking if the snapshot file format is valid. -
Overlap with #5736: That PR contains the same SQLite
div_ifix. Consider closing #5736 if this PR is intended to supersede it, or removing the SQLite changes from this PR to keep the two fixes independent.
There was a problem hiding this comment.
Pull request overview
Fixes a missed SQL codegen optimization where a >= low && a <= high should compile to a BETWEEN low AND high, and also updates SQLite integer-division SQL generation (with associated snapshot updates).
Changes:
- Adjust
try_into_betweento compare expression semantic content rather than fullrq::Exprequality (which includes spans). - Add regression coverage ensuring the BETWEEN optimization triggers.
- Update SQLite
div_iSQL template and refresh affected integration snapshots.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
prqlc/prqlc/src/sql/gen_expr.rs |
Updates BETWEEN optimization matching logic. |
prqlc/prqlc/tests/integration/sql.rs |
Adds regression test for BETWEEN output (and a SQLite // SQL regression). |
prqlc/prqlc/src/sql/std.sql.prql |
Changes SQLite integer-division (div_i) SQL template. |
prqlc/prqlc/tests/integration/snapshots/integration__queries__compileall__arithmetic.snap |
Updates expected SQL snapshots impacted by division template changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if a_l == b_l { | ||
| // Compare only `kind` (not `span`) since the same column | ||
| // referenced at two source positions has different spans. | ||
| if a_l.kind == b_l.kind { |
There was a problem hiding this comment.
if a_l.kind == b_l.kind moves the kind field out of a_l/b_l, so a_l can’t be passed to translate_operand(a_l, ...) afterward (partial move). Compare by reference instead (e.g., borrow the fields) or compare the underlying CId directly for ColumnRef to avoid moving the expressions.
| if a_l.kind == b_l.kind { | |
| if &a_l.kind == &b_l.kind { |
| let div_f = l r -> s"({l} * 1.0 / {r:12})" | ||
|
|
||
| @{binding_strength=100} | ||
| let div_i = l r -> s"ROUND(ABS({l:11} / {r:12}) - 0.5) * SIGN({l:0}) * SIGN({r:0})" | ||
| let div_i = l r -> s"CAST(ABS({l:11} * 1.0 / {r:12}) AS INTEGER) * SIGN({l:0}) * SIGN({r:0})" | ||
|
|
There was a problem hiding this comment.
This PR introduces SQLite integer-division behavior changes (sqlite.div_i) and updates a large existing snapshot accordingly, but the PR title/description focuses on the BETWEEN optimization. Consider splitting these changes into a separate PR or updating the PR description/title to cover the SQLite // fix and snapshot churn so reviewers understand the full scope.
The try_into_between function compared rq::Expr values including their span field. Since the same column referenced at two source positions has different spans, the equality check always failed, making the BETWEEN optimization dead code. Fix by comparing only the kind field (a_l.kind == b_l.kind) instead of the full Expr (a_l == b_l). Fixes PRQL#5737 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
26010f5 to
ef12bb0
Compare
Summary
try_into_betweenoptimization which was dead code since it comparedrq::Exprvalues including theirspanfielda_l == b_l(full Expr including span) toa_l.kind == b_l.kind(semantic content only)Fixes #5737
Before
Generated:
After
Root Cause
rq::ExprderivesPartialEqwhich includes thespanfield. The same columnareferenced at two different source positions has the sameCIdbut differentSpanvalues, so the equality checka_l == b_lalways failed.Test plan
test_between_optimizationregression testGenerated with Claude Code