sunchao commented on code in PR #26067:
URL: https://github.com/apache/datafusion/pull/26067#discussion_r4349937670
##########
datafusion/expr/src/window_state.rs:
##########
@@ -440,6 +498,28 @@ impl WindowFrameStateRange {
delta: Option<&ScalarValue>,
length: usize,
) -> Result<usize> {
+ if delta.is_none()
+ && range_columns
+ .iter()
+ .any(|column| column.data_type().is_nested())
+ {
+ // CURRENT ROW needs peer equality. Scalar ordering does not
support
+ // all nested keys; Arrow's sort comparator also handles
dictionary nulls.
+ let mut boundary = if SIDE {
+ last_range.start
+ } else {
+ last_range.end.max(idx)
+ };
+ let comparator = self.peer_comparator(range_columns)?;
+ while boundary < length {
+ let is_peer = comparator.compare(boundary, idx).is_eq();
+ if if SIDE { is_peer } else { !is_peer } {
Review Comment:
Changed this to `if is_peer == SIDE { break; }`.
Updated in
[`9910027`](https://github.com/apache/datafusion/pull/26067/commits/99100274dd139574854e40393474caac5c97e16b).
##########
datafusion/expr/src/window_state.rs:
##########
@@ -70,7 +71,10 @@ impl WindowAggState {
match self.window_frame_ctx.as_mut() {
// Rows have no state do nothing
Some(WindowFrameContext::Rows(_)) => {}
- Some(WindowFrameContext::Range { .. }) => {}
+ Some(WindowFrameContext::Range { state, .. }) => {
+ // The comparator refers to the old, unpruned ORDER BY arrays.
+ state.peer_comparator = None;
Review Comment:
Updated the comment to describe early release of the old ORDER BY arrays.
Replacement arrays already miss the cache by identity. The ownership assertion
checks release of the old array object; retained slices may still share its
backing buffers.
Updated in
[`9910027`](https://github.com/apache/datafusion/pull/26067/commits/99100274dd139574854e40393474caac5c97e16b).
##########
datafusion/expr/src/window_state.rs:
##########
@@ -729,6 +809,73 @@ mod tests {
use arrow::array::{Float64Array, UInt64Array};
use arrow::datatypes::{Field, Schema};
+ #[test]
+ fn range_peer_comparator_reuses_arrays_and_invalidates_after_pruning() ->
Result<()> {
Review Comment:
Removed the redundant assertions about the old comparator being released.
Kept one check that the comparator is reused and one that pruning releases the
old array, alongside the result check: repeated setup and retained arrays would
otherwise escape the value assertions. Renamed the test to describe those
behaviors.
Updated in
[`9910027`](https://github.com/apache/datafusion/pull/26067/commits/99100274dd139574854e40393474caac5c97e16b).
##########
datafusion/optimizer/src/analyzer/type_coercion.rs:
##########
@@ -1325,11 +1325,8 @@ fn extract_window_frame_target_type(col_type: &DataType)
-> Option<DataType> {
/// 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.
+/// This fallback admits durations and intervals, including encoded values.
+/// Structs and maps remain outside its supported types.
Review Comment:
Updated these comments to say that SQL admission remains conservative and
can be relaxed separately after validating the additional types. The
documentation now distinguishes the admission restrictions from Arrow's nested
peer comparison support.
Updated in
[`9910027`](https://github.com/apache/datafusion/pull/26067/commits/99100274dd139574854e40393474caac5c97e16b).
##########
datafusion/sqllogictest/test_files/window.slt:
##########
@@ -7207,23 +7206,21 @@ select count(*) over (order by l) from (values ([1]),
([2])) t(l)
1
2
-# but a list whose elements are nested (struct, list) plans and then fails at
-# execution: the peer comparison in the RANGE frame goes through
-# `ScalarValue::partial_cmp`, which cannot compare nested list elements, while
-# the sort that produced the input uses `make_comparator` and can. Documented
-# here so the fix to the scalar comparison can flip these expectations.
-query error DataFusion error: Internal error: Uncomparable values:
List\(\[\{c0: 1\}\]\), List\(\[\{c0: 1\}\]\)
+# Lists containing structs or lists also form RANGE peer groups.
Review Comment:
Added this plain-list case to window.slt with counts 2,2,3 and included the
wrong-result fix in the PR rationale. Restoring the old peer-search behavior in
a control reproduces 3,3,3.
Updated in
[`9910027`](https://github.com/apache/datafusion/pull/26067/commits/99100274dd139574854e40393474caac5c97e16b).
--
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]
