Skip to content

Commit e2db766

Browse files
authored
fix nth_value window function negates i64::MIN (#22304)
## Which issue does this PR close? - Closes #22222. ## Rationale for this change `nth_value` could panic at execution time when called with `i64::MIN` as the `n` argument. The implementation supports negative `n` for reverse indexing and negated the value internally, but negating `i64::MIN` overflows. This caused a runtime panic instead of returning a normal DataFusion error for invalid input. ## What changes are included in this PR? This PR adds validation for the second argument of `nth_value` so that `i64::MIN` is rejected before any negation occurs. Instead of panicking during execution, the function now returns a regular execution error. This PR also adds regression coverage at two levels: - a unit test for the `nth_value` window function implementation - a sqllogictest case covering the SQL query shape that previously triggered the panic ## Are these changes tested? Yes. The change is covered by: 1. a focused unit test in the window function implementation 2. a SQL logic test in `window.slt` Validated with: 1. `cargo test -p datafusion-functions-window nth_value --lib` 2. `cargo test -p datafusion-sqllogictest --test sqllogictests window` ## Are there any user-facing changes? Yes. Queries that previously panicked when calling `nth_value(..., -9223372036854775808)` now return a proper execution error instead.
1 parent a786471 commit e2db766

2 files changed

Lines changed: 33 additions & 0 deletions

File tree

datafusion/functions-window/src/nth_value.rs

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,14 @@ impl NthValue {
125125
}
126126
}
127127

128+
fn validate_nth_value_n(n: i64) -> Result<i64> {
129+
if n == i64::MIN {
130+
return exec_err!("The second argument of nth_value must not be i64::MIN");
131+
}
132+
133+
Ok(n)
134+
}
135+
128136
static FIRST_VALUE_DOCUMENTATION: LazyLock<Documentation> = LazyLock::new(|| {
129137
Documentation::builder(
130138
DOC_SECTION_ANALYTICAL,
@@ -287,6 +295,7 @@ impl WindowUDFImpl for NthValue {
287295
.map(|v| get_signed_integer(&v))
288296
{
289297
Some(Ok(n)) => {
298+
let n = validate_nth_value_n(n)?;
290299
if partition_evaluator_args.is_reversed() {
291300
-n
292301
} else {
@@ -660,4 +669,24 @@ mod tests {
660669
)?;
661670
Ok(())
662671
}
672+
673+
#[test]
674+
fn nth_value_i64_min_returns_error() {
675+
let expr = Arc::new(Column::new("c3", 0)) as Arc<dyn PhysicalExpr>;
676+
let n_value = Arc::new(Literal::new(ScalarValue::Int64(Some(i64::MIN))))
677+
as Arc<dyn PhysicalExpr>;
678+
679+
let err = NthValue::nth()
680+
.partition_evaluator(PartitionEvaluatorArgs::new(
681+
&[expr, n_value],
682+
&[Field::new("f", DataType::Int32, true).into()],
683+
false,
684+
false,
685+
))
686+
.unwrap_err();
687+
688+
assert!(err.to_string().starts_with(
689+
"Execution error: The second argument of nth_value must not be i64::MIN"
690+
));
691+
}
663692
}

datafusion/sqllogictest/test_files/window.slt

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1215,6 +1215,10 @@ NULL 3917
12151215
-1114 -1114
12161216
15673 15673
12171217

1218+
statement error Execution error: The second argument of nth_value must not be i64::MIN
1219+
SELECT nth_value(x, -9223372036854775808) OVER (ORDER BY x)
1220+
FROM (VALUES (1)) AS t(x);
1221+
12181222

12191223

12201224

0 commit comments

Comments
 (0)