Re: [PR] fix: Reject negative RANGE window frame offsets [datafusion]

["kosiew (via GitHub)" <gi...@apache.org>]

kosiew commented on code in PR #25816:
URL: https://github.com/apache/datafusion/pull/25816#discussion_r4323452201


##########
datafusion/optimizer/src/analyzer/type_coercion.rs:
##########
@@ -1441,9 +1489,20 @@ fn coerce_window_frame(
         }
         WindowFrameUnits::Rows | WindowFrameUnits::Groups => DataType::UInt64,
     };
-    window_frame.start_bound =
-        coerce_frame_bound(&target_type, window_frame.start_bound)?;
-    window_frame.end_bound = coerce_frame_bound(&target_type, window_frame.end_bound)?;
+    let original_start = window_frame.start_bound.clone();
+    let original_end = window_frame.end_bound.clone();
+    window_frame.start_bound = coerce_frame_bound(&target_type, original_start.clone())?;

Review Comment:
   Could we pass `window_frame.start_bound` and `window_frame.end_bound` directly to `coerce_frame_bound()` instead of cloning the saved bounds again? We can still keep `original_start` and `original_end` for validation, while avoiding two unnecessary clones.



##########
datafusion/optimizer/src/analyzer/type_coercion.rs:
##########
@@ -1290,6 +1290,54 @@ fn coerce_frame_bound(
     }
 }
 
+/// Reject negative RANGE offsets. Numeric strings are checked only after a
+/// successful numeric coercion, before rounding can hide their original sign.
+/// Interval ordering alone is insufficient here:
+/// a positive month can mask a negative day in a lexicographic comparison.
+fn validate_range_frame_bound(bound: &WindowFrameBound) -> Result<()> {
+    let offset = match bound {
+        WindowFrameBound::Preceding(offset) | WindowFrameBound::Following(offset)
+            if !offset.is_null() =>
+        {
+            offset
+        }
+        _ => return Ok(()),
+    };
+
+    let negative = match offset {
+        ScalarValue::Utf8(Some(value)) => value
+            .trim_start()
+            .strip_prefix('-')
+            .is_some_and(|magnitude| {
+                // Ignore exponent digits: -0e10 is zero, but -1e-1000 is
+                // negative even if floating-point coercion underflows to -0.
+                magnitude
+                    .split(['e', 'E'])
+                    .next()
+                    .unwrap_or_default()
+                    .bytes()
+                    .any(|digit| matches!(digit, b'1'..=b'9'))
+            }),
+        ScalarValue::IntervalYearMonth(Some(months)) => *months < 0,
+        ScalarValue::IntervalDayTime(Some(interval)) => {
+            interval.days < 0 || interval.milliseconds < 0
+        }
+        ScalarValue::IntervalMonthDayNano(Some(interval)) => {
+            interval.months < 0 || interval.days < 0 || interval.nanoseconds < 0
+        }
+        ScalarValue::Float16(Some(value)) => value.to_f32() < 0.0,
+        ScalarValue::Float32(Some(value)) => *value < 0.0,
+        ScalarValue::Float64(Some(value)) => *value < 0.0,
+        _ => offset < &ScalarValue::new_zero(&offset.data_type())?,
+    };
+
+    if negative {

Review Comment:
   Could we include the offending `bound` in the error message? Showing the offset and whether it is `PRECEDING` or `FOLLOWING` would make invalid frames easier to diagnose, especially for mixed-sign intervals. A small test assertion for these details would also be useful.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org