sunchao commented on code in PR #26067:
URL: https://github.com/apache/datafusion/pull/26067#discussion_r4349935636
##########
datafusion/expr/src/window_state.rs:
##########
@@ -343,12 +347,66 @@ impl PartitionBatchState {
#[derive(Debug, Default, Clone)]
pub struct WindowFrameStateRange {
sort_options: Vec<SortOptions>,
+ peer_comparator: Option<RangePeerComparator>,
+}
+
+/// Cache comparators because dictionary logical-null masks can scan entire
arrays.
Review Comment:
I kept Arrow comparison because scalar equality can give wrong results for
lists containing dictionary values. Two rows can both represent `[NULL]`, with
one using a null dictionary key and the other a key pointing to a null value.
ScalarValue equality distinguishes them; Arrow treats them as peers. Added a
regression and explained this beside the scan. The cache comment now covers
comparator setup generally, including deriving logical nulls across dictionary
arrays.
Updated in
[`9910027`](https://github.com/apache/datafusion/pull/26067/commits/99100274dd139574854e40393474caac5c97e16b).
##########
datafusion/expr/src/window_state.rs:
##########
@@ -343,12 +347,66 @@ impl PartitionBatchState {
#[derive(Debug, Default, Clone)]
pub struct WindowFrameStateRange {
sort_options: Vec<SortOptions>,
+ peer_comparator: Option<RangePeerComparator>,
+}
+
+/// Cache comparators because dictionary logical-null masks can scan entire
arrays.
+#[derive(Clone)]
+struct RangePeerComparator {
+ columns: Vec<ArrayRef>,
+ comparator: Arc<LexicographicalComparator>,
+}
+
+// The cached Arrow arrays and comparison closures are immutable. A comparison
+// that unwinds cannot leave this cache partially updated.
+impl std::panic::UnwindSafe for RangePeerComparator {}
Review Comment:
Yes, the public type affected is WindowFrameStateRange. Compile controls
confirm that it supports both borrowed and owned catch_unwind usage before this
PR and with the two marker implementations, and loses that compatibility
without them. I kept the two implementations on the private immutable
comparator wrapper and updated the comment to name the public type precisely.
The guarantee does not extend to WindowAggState or the Groups variant.
Updated in
[`9910027`](https://github.com/apache/datafusion/pull/26067/commits/99100274dd139574854e40393474caac5c97e16b).
##########
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.
Review Comment:
Clarified the exact case: dictionary encoding inside a nested list, where a
null key and a key referencing a null value represent the same logical list
element. The accompanying regression covers this distinction.
Updated in
[`9910027`](https://github.com/apache/datafusion/pull/26067/commits/99100274dd139574854e40393474caac5c97e16b).
##########
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)
Review Comment:
Added a comment explaining that an equality scan cannot skip preceding
non-peers when evaluation starts partway through a partition.
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]
