neilconway commented on code in PR #25300:
URL: https://github.com/apache/datafusion/pull/25300#discussion_r4037979005
##########
datafusion/functions-nested/src/set_ops.rs:
##########
@@ -351,29 +351,32 @@ fn generic_set_lists<OffsetSize: OffsetSizeTrait>(
let converter = RowConverter::new(vec![SortField::new(l.value_type())])?;
- // Normalize -0.0 → +0.0 so RowConverter (which uses IEEE 754 totalOrder
- // and treats ±0 as distinct) groups them together. Use the normalized
- // arrays for both row conversion and the final output values.
- let l_values_norm = normalize_float_zero(l.values());
- let r_values_norm = normalize_float_zero(r.values());
-
- // Only convert the visible portion of the values array. For sliced
- // ListArrays, values() returns the full underlying array but only
- // elements between the first and last offset are referenced.
+ // ListArray::values() returns the full underlying array for sliced lists.
+ // Slice first so normalization only scans and, when -0.0 is present,
+ // allocates for values referenced by the logical array.
+ // Normalization keeps SQL signed-zero equality when rows are encoded.
let l_first = l.offsets()[0].as_usize();
let l_len = l.offsets()[l.len()].as_usize() - l_first;
- let l_values = l_values_norm.slice(l_first, l_len);
- let rows_l = converter.convert_columns(&[Arc::clone(&l_values)])?;
+ let l_values_norm = if l_first == 0 && l_len == l.values().len() {
+ normalize_float_zero(l.values())
+ } else {
+ normalize_float_zero(&l.values().slice(l_first, l_len))
+ };
+ let rows_l = converter.convert_columns(&[l_values_norm.slice(0, l_len)])?;
Review Comment:
What about refactoring this into a shared helper?
##########
datafusion/functions-nested/src/set_ops.rs:
##########
@@ -351,29 +351,32 @@ fn generic_set_lists<OffsetSize: OffsetSizeTrait>(
let converter = RowConverter::new(vec![SortField::new(l.value_type())])?;
- // Normalize -0.0 → +0.0 so RowConverter (which uses IEEE 754 totalOrder
- // and treats ±0 as distinct) groups them together. Use the normalized
- // arrays for both row conversion and the final output values.
- let l_values_norm = normalize_float_zero(l.values());
- let r_values_norm = normalize_float_zero(r.values());
-
- // Only convert the visible portion of the values array. For sliced
- // ListArrays, values() returns the full underlying array but only
- // elements between the first and last offset are referenced.
+ // ListArray::values() returns the full underlying array for sliced lists.
+ // Slice first so normalization only scans and, when -0.0 is present,
+ // allocates for values referenced by the logical array.
+ // Normalization keeps SQL signed-zero equality when rows are encoded.
Review Comment:
Can we keep the original comments here? I think the note about
`RowConverter` float semantics is useful. The change we want to make is just to
normalize after slicing, not before, which might not even merit a separate
comment.
--
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]
