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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
