Conversation
Combining nested `take` ranges added the bounds without checking for overflow, and `LIMIT` rendered any value at or above 2^32 with sqlparser's `long` flag, appending an `L` that no dialect parses.
prql-bot
left a comment
There was a problem hiding this comment.
Self-review. Two issues in the overflow handling, both fixed in a follow-up commit on this branch.
An end-bound overflow that the intersection discards was rejected. The shifted end is intersected with the enclosing range's end on the next line (range.end = current.end.or_map(range.end, i64::min)), and once the shift has run past i64::MAX the enclosing end is necessarily the smaller of the two — so the overflow never reaches the output. from a | take 2..10 | take ..9223372036854775807 errored, though the intersection is rows 2–10: the same query with any bound that doesn't overflow, e.g. take 2..10 | take ..100, compiles to LIMIT 9 OFFSET 1. The end bound now falls back to the enclosing end when the shift overflows, and errors only when the enclosing range is unbounded and there is no representable answer.
The error underlined the wrong bound. The span was taken from range.start with range.end only as a fallback, so on a take whose end overflowed the caret landed on the start bound. from a | take 2.. | take 3..9223372036854775807 pointed at 3 under "take bounds are too large" — a bound that is not too large. Each bound now carries its own span.
Both are verified by new inline snapshot tests; cargo insta test -p prqlc --lib (98) and --test integration (509) pass, and cargo fmt --check plus cargo clippy -p prqlc --lib --no-default-features --features=default,lsp -- -D warnings are clean.
The rest reads correctly: the inlined start match is OrMap::or_map's body verbatim, so the non-overflow path is unchanged; lowering's validate_take_range does reject bounds below 1, so shift_bound's a - 1 cannot underflow as its doc claims; and every other Value::Number in gen_expr.rs already passes false for the long flag, so expr_of_i64 was the only site emitting the stray L.
An end bound that overflows while being shifted onto the enclosing range is intersected with that range's end on the next line, and the enclosing end is necessarily the smaller of the two once the shift has run past i64::MAX — so `take 2..10 | take ..9223372036854775807` has the same answer as `take 2..10 | take ..100`. Fall back to the enclosing end in that case, and error only when the enclosing range is unbounded. Give each bound its own span, so a `take` whose end overflowed no longer underlines its start bound.
prql-bot
left a comment
There was a problem hiding this comment.
Self-review of the follow-up commit. The end-bound fallback it adds is sound — an end that overflows while being shifted is necessarily larger than the enclosing end, so the intersection would have discarded it — but the same argument applies to the start bound and wasn't carried across.
from a | take 9223372036854775807..9223372036854775807 | take 2.. reports "take bounds are too large to combine with the enclosing take", though the answer is representable. A shifted start past i64::MAX exceeds every representable end, so the combined range selects nothing, and range_of_ranges already has a representation for that: the e < s branch returns Range { start: None, end: Some(0) }, which is how from a | take 2..3 | take 5.. compiles to LIMIT 0 today. Only an unbounded enclosing end — take 9223372036854775807.. | take 2.. — leaves no representable answer and warrants the error.
I'm pushing the fix: the start arm falls back the way the end arm does, an empty_range() helper replaces the literal duplicated at both sites, and a test pins LIMIT 0 for the bounded case while the existing test still pins the error for the unbounded one.
…sing end is bounded
prql-bot
left a comment
There was a problem hiding this comment.
Self-review of the follow-up commit. One point, on code the previous round added rather than on this commit.
shift_bound's doc assigns roles to its two parameters — "a 1-based range bound" shifted by "the start of the range it is nested in" — and justifies a - 1 with lowering having already rejected bounds below 1. The end call site passes them the other way round, shift_bound(current.start.unwrap_or(1), b, end_span), so there the subtracted operand is a start accumulated across earlier loop iterations, not a lowering-validated bound. The arithmetic is unaffected — the sum is commutative and both operands go through checked_ — but the doc's safety argument doesn't hold at that call site as written, so a reader checking one against the other has to re-derive why it is still sound.
Pushing the fix: the parameters become bound and enclosing_start, and the end call site passes the bound first, the way the start one already does. cargo fmt --check, cargo clippy -p prqlc --lib --no-default-features --features=default,lsp -- -D warnings, and the 99 prqlc --lib tests are clean on it.
I also refreshed the PR description: it still described the compile error as unconditional, which the two follow-up commits narrowed to the cases that have no representable answer.
…h call sites The doc justifies subtracting from the first parameter with lowering having rejected bounds below 1, but the end call site passed the enclosing start there instead. The sum is commutative so nothing changes at runtime; this makes the call sites match the argument the doc gives.
Two defects in how
takebounds at the extremes ofi64reach SQL. Both are reachable from an ordinary query throughprqlc compile, and both are covered by new inline snapshot tests inprqlc/prqlc/src/sql/gen_expr.rs.range_of_rangesoverflowed on nestedtakes. Combining two ranges computesa + b - 1to re-base the inner range's 1-based bounds onto the outer one, with no overflow check.from a | take 9223372036854775807.. | take 2..panics withattempt to add with overflowin a debug build;[profile.release]leavesoverflow-checksoff, so a release build instead wraps and silently emitsOFFSET 9223372036854775807— I confirmed both against locally built binaries. Lowering validates each bound is>= 1but never bounds it above, so nothing upstream catches this. The shift is now checked, in a helper whose subtraction runs before the addition:1 + i64::MAX - 1is representable but overflows if evaluated left to right, so the naive form would rejecttake ..9223372036854775807. A test pins that this still compiles.An overflow is only an error where the answer is genuinely unrepresentable, which turns on the enclosing range:
i64::MAX, so the intersection on the next line would discard it in favour of the enclosing end anyway. It falls back to that end —take 2..10 | take ..9223372036854775807compiles toLIMIT 9 OFFSET 1.i64::MAXis past every representable end, so the combined range selects nothing, andrange_of_rangesalready has a representation for that:take 9223372036854775807..9223372036854775807 | take 2..compiles toLIMIT 0, the same outputtake 2..3 | take 5..reaches without overflowing.take 9223372036854775807.. | take 2..andtake 2.. | take ..9223372036854775807. Each bound carries its own span, so the caret lands on the bound that overflowed rather than on whichever bound of thattakecomes first.LIMITaboveu32::MAXemitted a strayL.expr_of_i64passednumber.leading_zeros() < 32as sqlparser's secondValue::Numberfield. That field is thelongflag — it appends anLsuffix — not a width hint, sofrom a | take 5000000000compiled toLIMIT 5000000000 L, which no dialect parses. The threshold is exactly 2^32:take 4294967295was fine. Every otherValue::Numberin the module already passesfalse, andfetch_of_i64routes throughtranslate_literal, so dialects usingFETCHwere already correct whileLIMITdialects were not.They ship together because the first fix's test compiles a query with an
i64::MAXbound, whose snapshot would otherwise pin the invalidLoutput.Verification
task prqlc:pull-requestruns the workspace suite; the only failure isqueries::results::read_csv, which fails in this sandbox because DuckDB cannot reachextensions.duckdb.orgto autoload itsjsonextension — unrelated to this change, and green in CI.cargo fmt --checkandcargo clippy -p prqlc --lib --no-default-features --features=default,lsp -- -D warningsare both clean, and CI is green on the branch.