kumarUjjawal commented on code in PR #24916:
URL: https://github.com/apache/datafusion/pull/24916#discussion_r3998634975
##########
datafusion/optimizer/src/analyzer/type_coercion.rs:
##########
@@ -1114,18 +1116,56 @@ fn extract_window_frame_target_type(col_type:
&DataType) -> Result<DataType> {
| DataType::Time64(_)
)
{
- Ok(col_type.clone())
+ Some(col_type.clone())
} else if is_datetime(col_type) {
- Ok(DataType::Interval(IntervalUnit::MonthDayNano))
+ Some(DataType::Interval(IntervalUnit::MonthDayNano))
} else if let DataType::Dictionary(_, value_type) = col_type {
extract_window_frame_target_type(value_type)
} else if let DataType::RunEndEncoded(_, value_type) = col_type {
extract_window_frame_target_type(value_type.data_type())
} else {
- internal_err!("Cannot run range queries on datatype: {col_type}")
+ None
}
}
+/// Whether a free RANGE frame (all bounds `UNBOUNDED` or `CURRENT ROW`) can
+/// run over an ORDER BY column of `col_type` even though the type has no
+/// arithmetic for finite offsets.
+///
+/// Such a frame only compares rows to find peers, so the type must compare
+/// the same way in the RANGE peer check (`ScalarValue::partial_cmp`) as in
+/// the sort that produced the input order. That holds for durations and
+/// intervals; it does not for structs and maps, whose `ScalarValue`
+/// comparison differs from the sorter's, so they stay unsupported.
+fn supports_free_range_frame(col_type: &DataType) -> bool {
+ match col_type {
+ DataType::Duration(_) | DataType::Interval(_) => true,
+ DataType::Dictionary(_, value_type) =>
supports_free_range_frame(value_type),
+ DataType::RunEndEncoded(_, value_type) => {
+ supports_free_range_frame(value_type.data_type())
+ }
+ _ => false,
+ }
+}
+
+/// Errors if any ORDER BY expression has a type not supported in a free RANGE
frame.
+fn check_free_range_order_by_types(
+ expressions: &[Sort],
+ schema: &DFSchema,
+) -> Result<()> {
+ for sort in expressions {
+ let t = sort.expr.get_type(schema)?;
+ if extract_window_frame_target_type(&t).is_none()
Review Comment:
This still treats every type accepted by `extract_window_frame_target_type`
as safe for free-RANGE peer comparison. For example, `List<Struct>` returns
`Some` at line 1111, so `ORDER BY duration_col, list_of_struct_col` passes this
check. When the duration values tie, `compare_rows` reaches the list key, but
`ScalarValue::partial_cmp_list` uses Arrow's vectorized `lt`/`eq` kernels on
the Struct child arrays. Those kernels do not support nested types, so
`try_cmp` returns an internal `Uncomparable values` error. Sorting uses
`make_comparator`, which does support `List<Struct>`, leaving the sort and
RANGE peer comparison inconsistent. Could we recursively validate list element
types or reject unsupported nested elements and add a Duration plus
`List<Struct>` regression test?
##########
datafusion/optimizer/src/analyzer/type_coercion.rs:
##########
@@ -1099,7 +1099,9 @@ fn coerce_frame_bound(
}
}
-fn extract_window_frame_target_type(col_type: &DataType) -> Result<DataType> {
+/// The type that RANGE frame offsets are coerced to for an ORDER BY column of
+/// `col_type`, or `None` if the type does not support RANGE frames.
Review Comment:
`None` no longer means the type does not support RANGE frames: Duration and
Interval return `None` here but are supported for free RANGE frames. Could this
say that `None` means there is no offset-coercion target instead?
--
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]
