kumarUjjawal commented on code in PR #24916:
URL: https://github.com/apache/datafusion/pull/24916#discussion_r3921766593
##########
datafusion/optimizer/src/analyzer/type_coercion.rs:
##########
@@ -1110,7 +1115,17 @@ fn coerce_window_frame(
.map(|s| s.expr.get_type(schema))
.transpose()?;
if let Some(col_type) = current_types {
- let target_type = extract_window_frame_target_type(&col_type)?;
+ let target_type = match
extract_window_frame_target_type(&col_type) {
+ Ok(target_type) => target_type,
+ // A free range frame (`UNBOUNDED PRECEDING` / `CURRENT
ROW`
+ // on both sides, which is what `OVER (ORDER BY ...)`
+ // defaults to) has no offsets to coerce and only needs the
+ // ORDER BY values to be comparable, so an ORDER BY type
+ // without arithmetic (Duration, Interval, Struct, Map,
...)
+ // is fine there.
+ Err(_) if window_frame.free_range() => return
Ok(window_frame),
Review Comment:
This fallback now admits `Map` and `Struct` order keys, but RANGE peer
detection does not use the same comparison as sorting. `partial_cmp_map`
ignores map values it compares only entry column 0 and `partial_cmp_struct`
skips child fields when either side is NULL. Therefore, distinct keys such as
`map(['a'], [1])` and `map(['a'], [2])` can be treated as peers, making
`COUNT(*) OVER (ORDER BY x)` return `2, 2` instead of `1, 2`.
Could we either restrict this fallback to types whose peer comparison
matches the sorter, or fix those comparators and add Map and NULL-containing
Struct regression tests?
##########
datafusion/optimizer/src/analyzer/type_coercion.rs:
##########
@@ -1091,7 +1091,12 @@ fn extract_window_frame_target_type(col_type: &DataType)
-> Result<DataType> {
} 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}")
+ // Only reached for frames with a finite offset (free range frames
Review Comment:
These comment blocks repeat the same rationale, and the comment above
`plan_err!` is inaccurate: this function is also reached for free RANGE frames;
its error is created and then discarded by the caller. Could we keep one
concise explanation beside the free-range handling and remove the duplicate
block?
--
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]
