neilconway commented on code in PR #25186:
URL: https://github.com/apache/datafusion/pull/25186#discussion_r4136157174
##########
datafusion/physical-expr/src/expressions/in_list.rs:
##########
@@ -82,6 +83,20 @@ fn supports_arrow_eq(dt: &DataType) -> bool {
}
}
+fn normalize_in_list_float_zero_value(value: ColumnarValue) -> ColumnarValue {
+ match value {
+ ColumnarValue::Array(array)
+ if dictionary_value_type(array.data_type()).is_floating() =>
Review Comment:
`is_floating` seems like too shallow of a check for nested types. What if we
just did
```
ColumnarValue::Array(array) =>
ColumnarValue::Array(normalize_float_zero(&array)),
```
##########
datafusion/common/src/utils/mod.rs:
##########
@@ -1557,6 +1566,10 @@ pub fn normalize_float_zero_scalar(scalar: ScalarValue)
-> ScalarValue {
ScalarValue::Float16(Some(v)) if v.to_bits() << 1 == 0 => {
ScalarValue::Float16(Some(half::f16::from_bits(0)))
}
+ ScalarValue::Dictionary(key, mut value) => {
+ *value = normalize_float_zero_scalar(*value);
+ ScalarValue::Dictionary(key, value)
+ }
other => other,
Review Comment:
Shouldn't we handle other nested types, like REEs, unions, lists, ...?
##########
datafusion/common/src/utils/mod.rs:
##########
@@ -1455,6 +1455,15 @@ pub fn normalize_float_zero(array: &ArrayRef) ->
ArrayRef {
const NEG_ZERO_F32_BITS: u32 = (-0.0_f32).to_bits();
const NEG_ZERO_F64_BITS: u64 = (-0.0_f64).to_bits();
match array.data_type() {
+ DataType::Dictionary(_, value_type) if has_float_leaf(value_type) => {
+ let dictionary = array.as_any_dictionary();
+ let values = normalize_float_zero(dictionary.values());
+ if Arc::ptr_eq(&values, dictionary.values()) {
+ Arc::clone(array)
+ } else {
+ dictionary.with_values(values)
+ }
+ }
Review Comment:
This doesn't influence functionality, right, since we already handle
dictionaries in `has_float_leaf`? If so, is the idea here that adding this
special-case improves performance? If so, please make that clear in the PR
description and include benchmark results.
--
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]
