diff --git a/CHANGELOG.md b/CHANGELOG.md index 35eb182c006a..199cad6e65bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,13 @@ **Fixes**: +- `take` bounds at the extremes of `i64` no longer produce a panic or invalid + SQL. Combining nested ranges (`take 9223372036854775807.. | take 2..`) + overflowed while re-basing the inner bounds, panicking in a debug build and + silently emitting a wrapped `OFFSET` in a release one; it now reports a + compile error. Separately, a `LIMIT` at or above 2^32 was rendered with + sqlparser's `long` flag, so `take 5000000000` compiled to + `LIMIT 5000000000 L`, which no dialect parses. (@prql-bot, #6347) - `date.to_text` errors now name the format specifier the target dialect rejected — `format specifier %P is not supported for Postgres` rather than `PRQL doesn't support this format specifier`, whose span covers the whole diff --git a/prqlc/prqlc/src/sql/gen_expr.rs b/prqlc/prqlc/src/sql/gen_expr.rs index 0d956f374737..f9d9e791562c 100644 --- a/prqlc/prqlc/src/sql/gen_expr.rs +++ b/prqlc/prqlc/src/sql/gen_expr.rs @@ -761,11 +761,49 @@ pub(super) fn translate_sstring( pub(super) fn range_of_ranges(ranges: Vec>) -> Result> { let mut current = Range::default(); for range in ranges { - let mut range = try_range_into_int(range)?; + // Kept before the conversion below, which consumes the bounds, so an + // overflow can be reported against the bound that caused it. + let start_span = range.start.as_ref().and_then(|bound| bound.span); + let end_span = range.end.as_ref().and_then(|bound| bound.span); + let range = try_range_into_int(range)?; // b = b + a.start -1 (take care of 1-based index!) - range.start = range.start.or_map(current.start, |a, b| a + b - 1); - range.end = range.end.map(|b| current.start.unwrap_or(1) + b - 1); + // + // Both bounds are checked: a `take` nested inside another one shifts + // its bounds by the outer range's start, and two bounds near + // `i64::MAX` sum past it. Overflowing here panics in a debug build and, + // because `[profile.release]` leaves `overflow-checks` off, silently + // wraps to a nonsense `LIMIT`/`OFFSET` in a release one — so an + // overflow that the intersection and emptiness check below cannot + // discard is reported as a compile error instead. + let start = match (range.start, current.start) { + (Some(a), Some(b)) => match shift_bound(a, b, start_span) { + Ok(start) => Some(start), + // A start past `i64::MAX` is past every representable end, so + // a bounded enclosing range selects nothing — the same result + // the emptiness check below reaches for `take 2..3 | take 5..`. + // Only an unbounded enclosing end leaves no representable + // answer. + Err(err) if current.end.is_none() => return Err(err), + Err(_) => return Ok(empty_range()), + }, + (a, None) => a, + (None, b) => b, + }; + let end = match range.end { + Some(b) => match shift_bound(b, current.start.unwrap_or(1), end_span) { + Ok(end) => Some(end), + // The intersection below clamps the end to the enclosing one, + // which is necessarily the smaller of the two once the shifted + // bound has run past `i64::MAX`. So an overflow the + // intersection would discard is not an error; only an + // unbounded enclosing range leaves it with no representable + // answer. + Err(err) => Some(current.end.ok_or(err)?), + }, + None => None, + }; + let mut range = Range { start, end }; // b.end = min(a.end, b.end) range.end = current.end.or_map(range.end, i64::min); @@ -774,15 +812,38 @@ pub(super) fn range_of_ranges(ranges: Vec>) -> Result if let Some((s, e)) = current.start.zip(current.end) { if e < s { - return Ok(Range { - start: None, - end: Some(0), - }); + return Ok(empty_range()); } } Ok(current) } +/// The range that selects no rows. +fn empty_range() -> Range { + Range { + start: None, + end: Some(0), + } +} + +/// Shifts a 1-based range bound by the start of the range it is nested in, +/// i.e. `bound + enclosing_start - 1`. +/// +/// Subtracting before adding keeps the intermediate in range: lowering has +/// already rejected bounds below 1, so `bound - 1` cannot underflow, and the +/// sum then overflows only when the result genuinely exceeds `i64::MAX`. +/// Adding first would reject `take ..9223372036854775807`, whose result is +/// representable. +fn shift_bound(bound: i64, enclosing_start: i64, span: Option) -> Result { + bound + .checked_sub(1) + .and_then(|shifted| shifted.checked_add(enclosing_start)) + .ok_or_else(|| { + Error::new_simple("`take` bounds are too large to combine with the enclosing `take`") + .with_span(span) + }) +} + fn unpack_as_int_literal(bound: rq::Expr) -> Result { Some(bound.kind) .and_then(|x| x.into_literal().ok()) @@ -798,7 +859,11 @@ fn try_range_into_int(range: Range) -> Result> { } pub(super) fn expr_of_i64(number: i64) -> sql_ast::Expr { - sql_ast::Expr::Value(Value::Number(number.to_string(), number.leading_zeros() < 32).into()) + // The second field is sqlparser's `long` flag, which renders an `L` suffix + // — not a width hint. Every other number this module emits passes `false`, + // and `fetch_of_i64` renders the same value through `translate_literal`, + // so a dialect using FETCH already got it right where LIMIT did not. + sql_ast::Expr::Value(Value::Number(number.to_string(), false).into()) } pub(super) fn fetch_of_i64(take: i64, ctx: &mut Context) -> Fetch { @@ -1249,7 +1314,7 @@ impl From for ExprOrSource { #[cfg(test)] mod test { - use insta::assert_yaml_snapshot; + use insta::{assert_snapshot, assert_yaml_snapshot}; use super::*; @@ -1329,6 +1394,114 @@ mod test { Ok(()) } + /// The end bound is shifted by the enclosing range's start, so it + /// overflows on its own inputs — covered separately from the start bound. + #[test] + fn test_range_of_ranges_overflow_end() { + let query = "from a | take 2.. | take ..9223372036854775807"; + assert_snapshot!(crate::tests::compile(query).unwrap_err(), @" + Error: + ╭─[ :1:28 ] + │ + 1 │ from a | take 2.. | take ..9223372036854775807 + │ ─────────┬───────── + │ ╰─────────── `take` bounds are too large to combine with the enclosing `take` + ───╯ + "); + } + + /// An end bound that overflows while being shifted is still bounded by the + /// enclosing range's end, which the intersection picks — so this is an + /// ordinary query, not an overflow. + #[test] + fn test_range_of_ranges_overflowing_end_is_clamped_by_the_enclosing_end() { + let query = "from a | take 2..10 | take ..9223372036854775807"; + assert_snapshot!(crate::tests::compile(query).unwrap(), @" + SELECT + * + FROM + a + LIMIT + 9 OFFSET 1 + "); + } + + /// The error points at the bound that overflowed, not at whichever bound + /// of the same `take` happens to come first. + #[test] + fn test_range_of_ranges_overflow_points_at_the_offending_bound() { + let query = "from a | take 2.. | take 3..9223372036854775807"; + assert_snapshot!(crate::tests::compile(query).unwrap_err(), @" + Error: + ╭─[ :1:29 ] + │ + 1 │ from a | take 2.. | take 3..9223372036854775807 + │ ─────────┬───────── + │ ╰─────────── `take` bounds are too large to combine with the enclosing `take` + ───╯ + "); + } + + /// A `LIMIT` above `u32::MAX` must render as a plain integer; sqlparser's + /// `long` flag would append an `L` that no dialect parses. + #[test] + fn test_large_limit_has_no_long_suffix() { + let query = "from a | take 5000000000"; + assert_snapshot!(crate::tests::compile(query).unwrap(), @" + SELECT + * + FROM + a + LIMIT + 5000000000 + "); + } + + /// An end bound at `i64::MAX` in the outermost `take` is representable — + /// only the intermediate of a naive `a + b - 1` would overflow. + #[test] + fn test_range_of_ranges_max_end_is_not_an_overflow() { + let query = "from a | take ..9223372036854775807"; + assert_snapshot!(crate::tests::compile(query).unwrap(), @" + SELECT + * + FROM + a + LIMIT + 9223372036854775807 + "); + } + + /// A start bound that overflows while being shifted has run past every + /// representable end, so a bounded enclosing range selects nothing — the + /// same result `take 2..3 | take 5..` reaches without overflowing. + #[test] + fn test_range_of_ranges_overflowing_start_is_empty_when_enclosing_end_is_bounded() { + let query = "from a | take 9223372036854775807..9223372036854775807 | take 2.."; + assert_snapshot!(crate::tests::compile(query).unwrap(), @" + SELECT + * + FROM + a + LIMIT + 0 + "); + } + + #[test] + fn test_range_of_ranges_overflow() { + let query = "from a | take 9223372036854775807.. | take 2.."; + assert_snapshot!(crate::tests::compile(query).unwrap_err(), @" + Error: + ╭─[ :1:44 ] + │ + 1 │ from a | take 9223372036854775807.. | take 2.. + │ ┬ + │ ╰── `take` bounds are too large to combine with the enclosing `take` + ───╯ + "); + } + #[test] fn test_between_operands_are_parenthesized() { let query = "from t | filter x >= (b || c) && x <= (y && z)";