edubraqd commented on code in PR #24916:
URL: https://github.com/apache/datafusion/pull/24916#discussion_r4000602527
##########
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:
Reworded in 2ee8910: `None` now reads as "no offset type to coerce to", with
a pointer to `supports_free_range_frame` for the free-frame case.
##########
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:
Verified: `partial_cmp_list` goes through the arrow `lt`/`eq` kernels and
bails with `Uncomparable values` on nested element types, while the sorter uses
`make_comparator`. So the inconsistency is real.
It is not introduced here, though: `main` already accepts `List<Struct>` in
`extract_window_frame_target_type` (the `List(_)` arm predates this PR), so
`ORDER BY list_of_struct_col RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW`
plans today and hits the same runtime error on a tie. This PR does not widen
what `List` types are accepted; it only adds `Duration`/`Interval` and checks
every ORDER BY key instead of the first.
I would rather fix the comparator than reject at planning time, since the
latter would make queries that run on `main` fail. #24938 did exactly that
(`partial_cmp_list` via `make_comparator`, with `List<Struct>` and `List<List>`
regression tests) and was closed with the batch; I can reopen it as the
follow-up once this one lands, and add the Duration + `List<Struct>` window
case there. If you prefer the planning-time rejection inside this PR instead,
say so and I will add it.
--
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]
