Skip to content

Commit 0e292dc

Browse files
edubraqdclaudealambkumarUjjawal
authored
fix: allow free RANGE window frames over Duration and Interval ORDER BY types (#24916)
## Which issue does this PR close? - Part of #24915. This PR covers `Duration` and `Interval` ORDER BY types; `Struct` and `Map` stay rejected because their RANGE peer comparison (`partial_cmp_struct` / `partial_cmp_map`) does not match the sort order, and are left for a follow-up (fix the comparators, then extend `supports_free_range_frame`). ## Rationale for this change `OVER (ORDER BY x)` defaults to a free range frame with no offsets, which only needs the ORDER BY values to be comparable. `coerce_window_frame` nevertheless required an arithmetic target type for the ORDER BY column first, so `row_number() OVER (ORDER BY x)` on a `Duration` or `Interval` column failed with an internal error asking the user to file a bug. ## What changes are included in this PR? Only require the arithmetic target type for frames with a finite offset. A free range frame whose ORDER BY expressions are all either arithmetic types or `Duration` / `Interval` (also nested in `Dictionary` / `RunEndEncoded`) is returned unchanged; every ORDER BY expression is checked, since all of them take part in the peer comparison. Frames over supported types are coerced exactly as before, so plans do not change. The failure for unsupported types is now a planning error, `RANGE window frames are not supported for ORDER BY type ...`, which fits both free and offset frames. ## Are these changes tested? Yes. `window.slt` gains `row_number` / `rank` / `count` over `Duration` and `Interval` ORDER BY keys (default and explicit free range frame, single and two-column ORDER BY), and checks that `Struct` and `Map` keys, a `Map` after a `Duration`, and a finite offset over a `Struct` are still rejected at planning time. The existing `Struct` case that pinned the internal-error text now expects the planning error. ## Are there any user-facing changes? Window functions ordered by `Duration` or `Interval` columns work with the default frame. Unsupported ORDER BY types in a RANGE frame report a planning error instead of an internal error. --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> Co-authored-by: Kumar Ujjawal <ujjawalpathak6@gmail.com>
1 parent 3b16a3d commit 0e292dc

2 files changed

Lines changed: 165 additions & 7 deletions

File tree

‎datafusion/optimizer/src/analyzer/type_coercion.rs‎

Lines changed: 85 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1091,7 +1091,11 @@ fn coerce_frame_bound(
10911091
}
10921092
}
10931093

1094-
fn extract_window_frame_target_type(col_type: &DataType) -> Result<DataType> {
1094+
/// The type that RANGE frame offsets are coerced to for an ORDER BY column of
1095+
/// `col_type`, or `None` if there is no offset type to coerce to (the column
1096+
/// type has no arithmetic). `None` does not mean the type is unusable in a
1097+
/// RANGE frame: a free frame has no offsets, see `supports_free_range_frame`.
1098+
fn extract_window_frame_target_type(col_type: &DataType) -> Option<DataType> {
10951099
if col_type.is_numeric()
10961100
|| col_type.is_string()
10971101
|| col_type.is_binary()
@@ -1106,18 +1110,80 @@ fn extract_window_frame_target_type(col_type: &DataType) -> Result<DataType> {
11061110
| DataType::Time64(_)
11071111
)
11081112
{
1109-
Ok(col_type.clone())
1113+
Some(col_type.clone())
11101114
} else if is_datetime(col_type) {
1111-
Ok(DataType::Interval(IntervalUnit::MonthDayNano))
1115+
Some(DataType::Interval(IntervalUnit::MonthDayNano))
11121116
} else if let DataType::Dictionary(_, value_type) = col_type {
11131117
extract_window_frame_target_type(value_type)
11141118
} else if let DataType::RunEndEncoded(_, value_type) = col_type {
11151119
extract_window_frame_target_type(value_type.data_type())
11161120
} else {
1117-
internal_err!("Cannot run range queries on datatype: {col_type}")
1121+
None
11181122
}
11191123
}
11201124

1125+
/// Whether a free RANGE frame (all bounds `UNBOUNDED` or `CURRENT ROW`) can
1126+
/// run over an ORDER BY column of `col_type` even though the type has no
1127+
/// arithmetic for finite offsets.
1128+
///
1129+
/// Such a frame only compares rows to find peers, so the type must compare
1130+
/// the same way in the RANGE peer check (`ScalarValue::partial_cmp`) as in
1131+
/// the sort that produced the input order. That holds for durations and
1132+
/// intervals; it does not for structs and maps, whose `ScalarValue`
1133+
/// comparison differs from the sorter's, so they stay unsupported.
1134+
fn supports_free_range_frame(col_type: &DataType) -> bool {
1135+
match col_type {
1136+
DataType::Duration(_) | DataType::Interval(_) => true,
1137+
DataType::Dictionary(_, value_type) => supports_free_range_frame(value_type),
1138+
DataType::RunEndEncoded(_, value_type) => {
1139+
supports_free_range_frame(value_type.data_type())
1140+
}
1141+
_ => false,
1142+
}
1143+
}
1144+
1145+
/// Whether `col_type` is a list, possibly behind dictionary or run-end
1146+
/// encoding.
1147+
///
1148+
/// Lists are kept out of the free-range fallback because their peer
1149+
/// comparison and the sort do not agree on element NULLs: `compare_rows`
1150+
/// applies the NULLS FIRST / NULLS LAST option to the top-level value only and
1151+
/// then calls `ScalarValue::partial_cmp`, whose `partial_cmp_list` always
1152+
/// orders a NULL element after a non-NULL one (Postgres semantics), while the
1153+
/// sorter's `make_comparator` applies the option to the elements as well. Under
1154+
/// `ORDER BY d, l NULLS FIRST` with tied `d`, the sort puts `[NULL]` before
1155+
/// `[1]` and the peer check orders them the other way round.
1156+
fn is_list_type(col_type: &DataType) -> bool {
1157+
match col_type {
1158+
DataType::List(_) | DataType::LargeList(_) | DataType::FixedSizeList(_, _) => {
1159+
true
1160+
}
1161+
DataType::Dictionary(_, value_type) => is_list_type(value_type),
1162+
DataType::RunEndEncoded(_, value_type) => is_list_type(value_type.data_type()),
1163+
_ => false,
1164+
}
1165+
}
1166+
1167+
/// Errors if any ORDER BY expression has a type not supported in a free RANGE
1168+
/// frame: a type with neither an offset target nor a sound peer comparison, or
1169+
/// a list (see `is_list_type`).
1170+
fn check_free_range_order_by_types(
1171+
expressions: &[Sort],
1172+
schema: &DFSchema,
1173+
) -> Result<()> {
1174+
for sort in expressions {
1175+
let t = sort.expr.get_type(schema)?;
1176+
let supported = supports_free_range_frame(&t)
1177+
|| (extract_window_frame_target_type(&t).is_some() && !is_list_type(&t));
1178+
if !supported {
1179+
return plan_err!(
1180+
"RANGE window frames are not supported for ORDER BY type {t}"
1181+
);
1182+
}
1183+
}
1184+
Ok(())
1185+
}
1186+
11211187
// Coerces the given `window_frame` to use appropriate natural types.
11221188
// For example, ROWS and GROUPS frames use `UInt64` during calculations.
11231189
fn coerce_window_frame(
@@ -1133,7 +1199,21 @@ fn coerce_window_frame(
11331199
.map(|s| s.expr.get_type(schema))
11341200
.transpose()?;
11351201
if let Some(col_type) = current_types {
1136-
let target_type = extract_window_frame_target_type(&col_type)?;
1202+
let target_type = match extract_window_frame_target_type(&col_type) {
1203+
Some(target_type) => target_type,
1204+
// A free range frame has no offsets to coerce, so ORDER BY
1205+
// types without arithmetic are fine as long as their peer
1206+
// comparison is sound (see `supports_free_range_frame`).
1207+
None if window_frame.free_range() => {
1208+
check_free_range_order_by_types(expressions, schema)?;
1209+
return Ok(window_frame);
1210+
}
1211+
None => {
1212+
return plan_err!(
1213+
"RANGE window frames are not supported for ORDER BY type {col_type}"
1214+
);
1215+
}
1216+
};
11371217
// A finite offset bound (e.g. `5 PRECEDING`) is computed as
11381218
// `current_value ± offset`, so it is only meaningful for target
11391219
// types that support arithmetic. Other orderable target types can

‎datafusion/sqllogictest/test_files/window.slt‎

Lines changed: 80 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7018,8 +7018,8 @@ query error DataFusion error: type_coercion\ncaused by\nError during planning: R
70187018
SELECT COUNT(*) OVER (ORDER BY x RANGE BETWEEN CURRENT ROW AND INTERVAL '1' HOUR FOLLOWING)
70197019
FROM (VALUES (arrow_cast('01:00:00', 'Time64(Microsecond)'))) t(x)
70207020

7021-
# Unsupported RANGE ORDER BY types still propagate the type-coercion error.
7022-
query error DataFusion error: type_coercion\ncaused by\nInternal error: Cannot run range queries on datatype: Struct\("c0": Int64\)
7021+
# Unsupported RANGE ORDER BY types are rejected during planning.
7022+
query error Error during planning: RANGE window frames are not supported for ORDER BY type Struct
70237023
SELECT COUNT(*) OVER (ORDER BY x)
70247024
FROM (VALUES (struct(1))) t(x)
70257025

@@ -7094,6 +7094,84 @@ SELECT last_value() OVER () FROM (VALUES (1), (2), (3)) t(x)
70947094
statement error DataFusion error: Error during planning: 'nth_value' does not support zero arguments
70957095
SELECT nth_value() OVER () FROM (VALUES (1), (2), (3)) t(x)
70967096

7097+
# The default frame of `OVER (ORDER BY ...)` is a free RANGE frame, which
7098+
# needs no offset arithmetic on the ORDER BY type. Durations and intervals
7099+
# used to fail with "Internal error: Cannot run range queries on datatype".
7100+
query ?I
7101+
select x, row_number() over (order by x) from (values (arrow_cast(2, 'Duration(Second)')), (arrow_cast(1, 'Duration(Second)'))) t(x) order by 2
7102+
----
7103+
0 days 0 hours 0 mins 1 secs 1
7104+
0 days 0 hours 0 mins 2 secs 2
7105+
7106+
query ?I
7107+
select x, rank() over (order by x) from (values (interval '2 months'), (interval '1 month')) t(x) order by 2
7108+
----
7109+
1 mons 1
7110+
2 mons 2
7111+
7112+
query ?I
7113+
select x, row_number() over (order by x range between unbounded preceding and current row) from (values (arrow_cast(2, 'Duration(Millisecond)')), (arrow_cast(1, 'Duration(Millisecond)'))) t(x) order by 2
7114+
----
7115+
0 days 0 hours 0 mins 0.001 secs 1
7116+
0 days 0 hours 0 mins 0.002 secs 2
7117+
7118+
# offset frames still need an arithmetic ORDER BY type; the map and struct
7119+
# types are not accepted in free frames either, see `supports_free_range_frame`
7120+
statement error Error during planning: RANGE window frames are not supported for ORDER BY type Struct
7121+
select count(*) over (order by x range between 1 preceding and current row) from (values (struct(1))) t(x)
7122+
7123+
statement error Error during planning: RANGE window frames are not supported for ORDER BY type Map
7124+
select count(*) over (order by x) from (values (map(['a'], [1])), (map(['a'], [2]))) t(x)
7125+
7126+
# every ORDER BY expression takes part in the peer comparison, so a map after
7127+
# a duration is rejected as well, while two durations are fine; the error names
7128+
# the offending type, not the first ORDER BY type
7129+
statement error Error during planning: RANGE window frames are not supported for ORDER BY type Map
7130+
select count(*) over (order by d, m) from (values (arrow_cast(1, 'Duration(Second)'), map(['a'], [1])), (arrow_cast(1, 'Duration(Second)'), map(['a'], [2]))) t(d, m)
7131+
7132+
query ??I
7133+
select d, e, count(*) over (order by d, e) from (values (arrow_cast(1, 'Duration(Second)'), arrow_cast(2, 'Duration(Millisecond)')), (arrow_cast(1, 'Duration(Second)'), arrow_cast(1, 'Duration(Millisecond)'))) t(d, e) order by 3
7134+
----
7135+
0 days 0 hours 0 mins 1 secs 0 days 0 hours 0 mins 0.001 secs 1
7136+
0 days 0 hours 0 mins 1 secs 0 days 0 hours 0 mins 0.002 secs 2
7137+
7138+
# List ORDER BY keys were already accepted in RANGE frames before the free-range
7139+
# fallback existed (`extract_window_frame_target_type` returns the list type
7140+
# itself), and this PR does not change that. A list of primitives works:
7141+
query I
7142+
select count(*) over (order by l) from (values ([1]), ([2])) t(l)
7143+
----
7144+
1
7145+
2
7146+
7147+
# but a list whose elements are nested (struct, list) plans and then fails at
7148+
# execution: the peer comparison in the RANGE frame goes through
7149+
# `ScalarValue::partial_cmp`, which cannot compare nested list elements, while
7150+
# the sort that produced the input uses `make_comparator` and can. Documented
7151+
# here so the fix to the scalar comparison can flip these expectations.
7152+
query error DataFusion error: Internal error: Uncomparable values: List\(\[\{c0: 1\}\]\), List\(\[\{c0: 1\}\]\)
7153+
select count(*) over (order by l) from (values ([struct(1)]), ([struct(2)])) t(l)
7154+
7155+
query error DataFusion error: Internal error: Uncomparable values: List\(\[\[1\]\]\), List\(\[\[1\]\]\)
7156+
select count(*) over (order by l) from (values ([[1]]), ([[2]])) t(l)
7157+
7158+
# after a duration key the free-range check looks at every ORDER BY type, and
7159+
# a list is rejected at planning even though it sorts fine: the RANGE peer
7160+
# check (`compare_rows`, then `ScalarValue::partial_cmp`) always orders a NULL
7161+
# list element after a non-NULL one, while the sort applies NULLS FIRST to the
7162+
# elements as well, so with tied durations `[NULL]` and `[1]` would be sorted
7163+
# one way and compared the other
7164+
query error Error during planning: RANGE window frames are not supported for ORDER BY type List
7165+
select count(*) over (order by d, l nulls first) from (values (arrow_cast(1, 'Duration(Second)'), [NULL]), (arrow_cast(1, 'Duration(Second)'), [1])) t(d, l)
7166+
7167+
# the check is on the type, not the values: a list without NULLs and a
7168+
# dictionary-encoded list are rejected the same way
7169+
query error Error during planning: RANGE window frames are not supported for ORDER BY type List
7170+
select count(*) over (order by d, l) from (values (arrow_cast(1, 'Duration(Second)'), [1]), (arrow_cast(1, 'Duration(Second)'), [2])) t(d, l)
7171+
7172+
query error Error during planning: RANGE window frames are not supported for ORDER BY type Dictionary\(Int32, List
7173+
select count(*) over (order by d, l) from (values (arrow_cast(1, 'Duration(Second)'), arrow_cast([1], 'Dictionary(Int32, List(Int64))')), (arrow_cast(1, 'Duration(Second)'), arrow_cast([2], 'Dictionary(Int32, List(Int64))'))) t(d, l)
7174+
70977175
# Window functions are not allowed in WHERE or HAVING: both are evaluated before
70987176
# window functions are computed, so they are rejected during planning
70997177
# issue: https://github.com/apache/datafusion/issues/4610

0 commit comments

Comments
 (0)