You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "divyankshah (via GitHub)" <gi...@apache.org> on 2026/08/03 18:53:49 UTC
[PR] fix: normalize signed zero in nested float array comparisons [datafusion-comet]
divyankshah opened a new pull request, #5235:
URL: https://github.com/apache/datafusion-comet/pull/5235
## Which issue does this PR close?
Closes #5191.
## Rationale for this change
Arrow's nested comparator (`make_comparator`, used by `arrays_overlap`'s nested path and `array_position`'s nested fallback) orders floats by total order, where `-0.0` and `0.0` are distinct. Spark's `ordering.equiv` (used for structural equality of nested elements) checks numeric equality first, so `-0.0 == 0.0` there, while `NaN == NaN` still holds in both.
## What changes are included in this PR?
- Add `nested_float_normalize.rs`: recursively rebuilds nested (List/LargeList/FixedSizeList/Struct) arrays with `-0.0` normalized to `0.0` in Float32/Float64 leaves, leaving NaN untouched.
- `arrays_overlap.rs`: normalize both sides before building the nested comparator; update `test_nested_float_total_order` to assert `-0.0` and `0.0` now overlap; add `test_struct_float_field_signed_zero_overlap` covering a struct field.
- `array_position.rs`: normalize both sides before building the fallback comparator; update `test_nested_float_and_null_position` (result changes from `[2, 2, 1]` to `[2, 1, 1]` since row 1's `-0.0` vs `0.0` now matches at position 1); add `test_struct_float_field_signed_zero_position` covering a struct field.
Note: #5194 is a separate issue (#5101, comparator-hoisting for perf) but touches the same comparator-construction code path. Happy to rebase on top of whichever lands first.
## How are these changes tested?
- `cargo test -p datafusion-comet-spark-expr` — all 600+ tests pass, including the new/updated ones above.
- `cargo clippy -p datafusion-comet-spark-expr --lib -- -D warnings` — clean.
- `cargo fmt -p datafusion-comet-spark-expr -- --check` — clean.
- `cargo check --workspace` — clean.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: normalize signed zero in nested float array comparisons [datafusion-comet]
Posted by "divyankshah (via GitHub)" <gi...@apache.org>.
divyankshah commented on PR #5235:
URL: https://github.com/apache/datafusion-comet/pull/5235#issuecomment-5590122373
Rebased onto main again to clear a fresh conflict, so this is mergeable once more.
The conflict was a small one in `array_funcs/mod.rs`. #5614 added `mod sequence;` right next to the `mod nested_float_normalize;` line added here, so the two additions landed on adjacent lines of the same list. Both are kept, in alphabetical order, along with the `pub use sequence::spark_sequence;` re-export from main. Nothing else needed changing, and none of the expression code in this PR was affected.
Re-verified after the rebase:
- `cargo test -p datafusion-comet-spark-expr`: 723 passed, 0 failed
- `cargo clippy -p datafusion-comet-spark-expr --lib --all-targets -- -D warnings`: clean
- `cargo fmt -p datafusion-comet-spark-expr -- --check`: clean
For what it is worth, the previous push had a full green CI run, including the array SQL file tests, so the rebase is the only thing that has changed since then.
@andygrove, @peterxcli, whenever either of you has a spare moment, this one is ready for another look. No rush at all, I know review bandwidth is tight. If it would be easier to review as a single squashed commit, or if you would prefer the `normalize_nan.rs` cleanup folded in first, just say the word and I will sort it out.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: normalize signed zero in nested float array comparisons [datafusion-comet]
Posted by "divyankshah (via GitHub)" <gi...@apache.org>.
divyankshah commented on PR #5235:
URL: https://github.com/apache/datafusion-comet/pull/5235#issuecomment-5195038287
Hi @andygrove,
thanks for letting me know the -0.0 literal trap, good to know for future fixtures too. addressed in the latest commit:
1) fixed arrays_overlap.sql:120 to use double('0.0')/double('-0.0') instead of bare literals
2) added test_overlap_nested_dbl and test_overlap_struct_dbl to arrays_overlap.sql
3) added test_ap_nested_dbl to array_position.sql, kept the trailing NULL inner list row
4) added a comment on arrays_overlap_list_generic noting the flat path stays untouched since Spark's flat and nested paths disagree on signed zero
5) added a comment on test_struct_float_field_signed_zero_position noting it's not reachable from SQL yet
Please let me know if anything needs to be adjusted.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: normalize signed zero in nested float array comparisons [datafusion-comet]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #5235:
URL: https://github.com/apache/datafusion-comet/pull/5235#discussion_r3715308708
##########
native/spark-expr/src/array_funcs/nested_float_normalize.rs:
##########
@@ -0,0 +1,208 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::math_funcs::internal::normalize_float;
+use arrow::array::{
+ Array, ArrayRef, AsArray, FixedSizeListArray, Float32Array, Float64Array, LargeListArray,
+ ListArray, StructArray,
+};
+use arrow::datatypes::{DataType, Float32Type, Float64Type};
+use std::sync::Arc;
+
+pub(super) fn has_float_leaf(dt: &DataType) -> bool {
+ match dt {
+ DataType::Float32 | DataType::Float64 => true,
+ DataType::List(field) | DataType::LargeList(field) | DataType::FixedSizeList(field, _) => {
+ has_float_leaf(field.data_type())
+ }
+ DataType::Struct(fields) => fields.iter().any(|f| has_float_leaf(f.data_type())),
+ _ => false,
+ }
+}
+
+/// Recursively rebuilds nested arrays with `-0.0` normalized to `0.0` and NaN canonicalized
+/// in any Float32/Float64 leaves.
+pub(super) fn normalize_nested_floats(array: &ArrayRef) -> ArrayRef {
Review Comment:
found there is a similar helper in hll agg:
https://github.com/apache/datafusion-comet/blob/397b5bbf754e0c40c6ae9472fa7530037bf00889/native/spark-expr/src/agg_funcs/hll_plus_plus.rs#L123-L140
considering extending it to cover more data type so we dont need another file, or at least we can have one centralized helper for similar usage.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: normalize signed zero in nested float array comparisons [datafusion-comet]
Posted by "divyankshah (via GitHub)" <gi...@apache.org>.
divyankshah commented on PR #5235:
URL: https://github.com/apache/datafusion-comet/pull/5235#issuecomment-5721332768
Thanks @sunchao, that is a good catch and the diagnosis is exactly right. Fixed in 90dc9a36.
The guard was only applied at the call sites in `arrays_overlap_list_generic`, so once the top level check passed, every child was rebuilt whether or not it held float data. An empty struct child then reached `StructArray::new`, which cannot infer a length from zero columns. Your `_partition` example is a real reachable path for that, and I had not considered a zero column struct at all.
The fix is the recursive guard you suggested. `has_float_leaf` now runs inside `normalize_nested_floats` rather than only at the call sites, so any subtree without a float leaf is returned unchanged. Empty structs fall out of that naturally since they have no float leaf, and as a side benefit the rebuild is now proportional to the float data instead of the whole element.
Three regression tests added, all verified to fail without the guard and pass with it:
- `test_struct_with_empty_struct_field_overlap` in `arrays_overlap.rs`, which mirrors your query shape with two rows of ordinary doubles and an empty struct field. Without the guard it panics in `struct_array.rs`.
- `test_normalize_struct_with_empty_struct_sibling`, the same case at the helper level.
- `test_normalize_leaves_float_free_subtree_untouched`, which pins the no rebuild behaviour using a nested float free struct child.
Also rebased onto current main while I was in here. Local results after the fix:
- `cargo test -p datafusion-comet-spark-expr`: 862 passed, 0 failed
- `cargo clippy -p datafusion-comet-spark-expr --lib --all-targets -- -D warnings`: clean
- `cargo fmt -p datafusion-comet-spark-expr -- --check`: clean
I have not been able to run the Iceberg case end to end locally, so if you have that setup handy I would appreciate a confirmation on the original repro.
@andygrove, thanks for the approval and for digging back into the `typeWithProperEquals` split rather than taking the earlier round on trust. Since the head has moved, this will need another look before it goes in.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: normalize signed zero in nested float array comparisons [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5235:
URL: https://github.com/apache/datafusion-comet/pull/5235#issuecomment-5209910268
Third round, and this all checks out. I built the branch and ran the Rust suite (602 pass), clippy, `cargo fmt`, and all 60 array SQL file tests, everything green. I also re-ran the two new fixtures against the merge-base and confirmed they fail there, so they are genuinely load bearing rather than passing vacuously. The `double('-0.0')` spellings all survived the transcription, including the fix to the existing row.
One small thing left, on naming. `test_nested_float_total_order` at `arrays_overlap.rs:737` was originally pinning Arrow's total-order behavior, and it now asserts that `-0.0` and `0.0` overlap, which is the opposite of total order. Same story for the new `test_nested_float_signed_nan_total_order` at `:754`. Could you rename both, maybe to `test_nested_float_spark_equality` and `test_nested_float_signed_nan_spark_equality`? My worry is an arrow-rs upgrade down the line producing a failure in a test called `total_order` and someone restoring the old behavior in the wrong direction.
Happy to approve once that is in and CI comes back green.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: normalize signed zero in nested float array comparisons [datafusion-comet]
Posted by "divyankshah (via GitHub)" <gi...@apache.org>.
divyankshah commented on PR #5235:
URL: https://github.com/apache/datafusion-comet/pull/5235#issuecomment-5209988069
Hi @andygrove, thanks for suggestion renamed both as suggested in the latest commit. Waiting on CI now.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: normalize signed zero in nested float array comparisons [datafusion-comet]
Posted by "divyankshah (via GitHub)" <gi...@apache.org>.
divyankshah commented on code in PR #5235:
URL: https://github.com/apache/datafusion-comet/pull/5235#discussion_r3715481331
##########
native/spark-expr/src/array_funcs/nested_float_normalize.rs:
##########
@@ -0,0 +1,208 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::math_funcs::internal::normalize_float;
+use arrow::array::{
+ Array, ArrayRef, AsArray, FixedSizeListArray, Float32Array, Float64Array, LargeListArray,
+ ListArray, StructArray,
+};
+use arrow::datatypes::{DataType, Float32Type, Float64Type};
+use std::sync::Arc;
+
+pub(super) fn has_float_leaf(dt: &DataType) -> bool {
+ match dt {
+ DataType::Float32 | DataType::Float64 => true,
+ DataType::List(field) | DataType::LargeList(field) | DataType::FixedSizeList(field, _) => {
+ has_float_leaf(field.data_type())
+ }
+ DataType::Struct(fields) => fields.iter().any(|f| has_float_leaf(f.data_type())),
+ _ => false,
+ }
+}
+
+/// Recursively rebuilds nested arrays with `-0.0` normalized to `0.0` and NaN canonicalized
+/// in any Float32/Float64 leaves.
+pub(super) fn normalize_nested_floats(array: &ArrayRef) -> ArrayRef {
Review Comment:
Hi @peterxcli,
thanks for pointing that out. the normalization rule itself isn't duplicated, we already call the same normalize_float function you linked. there's a bit of repeated dispatch code around it (the Float32/Float64 match arms), but the actual behavior comes from one place. hll's version skips nested types (struct/list) right now, and I'm not sure if making it handle those too would change hll's behavior in ways that need separate testing. But I can follow up on it if it's worth doing. Please let me know if I should implement it in that way.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: normalize signed zero in nested float array comparisons [datafusion-comet]
Posted by "divyankshah (via GitHub)" <gi...@apache.org>.
divyankshah commented on PR #5235:
URL: https://github.com/apache/datafusion-comet/pull/5235#issuecomment-5488245449
Hi @andygrove, @peterxcli,
Sorry for the delay on this one. The branch had picked up a merge conflict, so I have rebased it onto current main and it is mergeable again.
The only conflict was in `arrays_overlap.sql`, on the double array fixture row. #5393 changed that row to `array(0.0)` against `array(double('-0.0'))`, and this branch had already changed it to `double('0.0')` against `double('-0.0')`. The two spell the same value since the column is `array<double>`, so I kept the explicit form for consistency with the rest of the file.
The Rust side merged without conflicts. #5376 added NaN canonicalization to the flat path, which sits next to the nested path this PR touches rather than overlapping with it, so both behaviours are preserved. The comment on `arrays_overlap_list_generic` explaining why the normalization cannot move into the flat fast path still applies.
Re-verified on the rebased branch:
- `cargo test -p datafusion-comet-spark-expr`: 666 passed, 0 failed. That includes the tests added here and the flat float tests from #5376, so the two fixes coexist.
- `cargo clippy -p datafusion-comet-spark-expr --lib --all-targets -- -D warnings`: clean
- `cargo fmt -p datafusion-comet-spark-expr -- --check`: clean
One thing worth flagging: @sunchao opened #5507 for ordering and rank semantics on floats nested in arrays and structs, noting that #5469's type gate does not recurse into arrays or structs. That is the same nested gap this PR addresses, just reached from the sort and window side rather than the array function side. The normalization helper added here (`nested_float_normalize.rs`) is written to be reusable, so it may be a useful building block there too.
@peterxcli, on your `normalize_nan.rs` suggestion from the earlier round, happy to fold that in if you would still like it in this PR, or to leave it as a follow-up if you would rather keep this one focused. Either works for me.
Thanks both for the thorough reviews earlier, and sorry again for letting this go stale.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: normalize signed zero in nested float array comparisons [datafusion-comet]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #5235:
URL: https://github.com/apache/datafusion-comet/pull/5235#discussion_r3741131088
##########
native/spark-expr/src/array_funcs/nested_float_normalize.rs:
##########
@@ -0,0 +1,208 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::math_funcs::internal::normalize_float;
+use arrow::array::{
+ Array, ArrayRef, AsArray, FixedSizeListArray, Float32Array, Float64Array, LargeListArray,
+ ListArray, StructArray,
+};
+use arrow::datatypes::{DataType, Float32Type, Float64Type};
+use std::sync::Arc;
+
+pub(super) fn has_float_leaf(dt: &DataType) -> bool {
+ match dt {
+ DataType::Float32 | DataType::Float64 => true,
+ DataType::List(field) | DataType::LargeList(field) | DataType::FixedSizeList(field, _) => {
+ has_float_leaf(field.data_type())
+ }
+ DataType::Struct(fields) => fields.iter().any(|f| has_float_leaf(f.data_type())),
+ _ => false,
+ }
+}
+
+/// Recursively rebuilds nested arrays with `-0.0` normalized to `0.0` and NaN canonicalized
+/// in any Float32/Float64 leaves.
+pub(super) fn normalize_nested_floats(array: &ArrayRef) -> ArrayRef {
Review Comment:
And I think, with the way, we could also resolve the problem mention in https://github.com/apache/datafusion-comet/pull/5166#discussion_r3732293876
> And would you file an issue to teach the native NormalizeNaNAndZero in native/spark-expr/src/math_funcs/internal/normalize_nan.rs to recurse into List and Struct, so the serde can emit a single native node for the nested shapes?
##########
native/spark-expr/src/array_funcs/nested_float_normalize.rs:
##########
@@ -0,0 +1,208 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::math_funcs::internal::normalize_float;
+use arrow::array::{
+ Array, ArrayRef, AsArray, FixedSizeListArray, Float32Array, Float64Array, LargeListArray,
+ ListArray, StructArray,
+};
+use arrow::datatypes::{DataType, Float32Type, Float64Type};
+use std::sync::Arc;
+
+pub(super) fn has_float_leaf(dt: &DataType) -> bool {
+ match dt {
+ DataType::Float32 | DataType::Float64 => true,
+ DataType::List(field) | DataType::LargeList(field) | DataType::FixedSizeList(field, _) => {
+ has_float_leaf(field.data_type())
+ }
+ DataType::Struct(fields) => fields.iter().any(|f| has_float_leaf(f.data_type())),
+ _ => false,
+ }
+}
+
+/// Recursively rebuilds nested arrays with `-0.0` normalized to `0.0` and NaN canonicalized
+/// in any Float32/Float64 leaves.
+pub(super) fn normalize_nested_floats(array: &ArrayRef) -> ArrayRef {
Review Comment:
1. Move `has_float_leaf` + `normalize_nested_floats` out of `array_funcs/` into a neutral, `pub(crate)` location (co-locating with `normalize_float` in `math_funcs/internal/` is defensible).
2. Have `NormalizeNaNAndZero::evaluate` and `hll_plus_plus::normalize_floats` delegate to it, keeping HLL’s pass-through and the flat fast path intact.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: normalize signed zero in nested float array comparisons [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on PR #5235:
URL: https://github.com/apache/datafusion-comet/pull/5235#issuecomment-5721037435
Found **one P2 regression** at head `ca51e94b`. I recommend fixing it before merging.
**P2 — Preserve empty struct children during normalization** — [nested_float_normalize.rs:85](https://github.com/apache/datafusion-comet/blob/ca51e94b124b1864ada8551e2740c90359d993fc/native/spark-expr/src/array_funcs/nested_float_normalize.rs#L85).
Once an enclosing struct contains a float, the helper recursively rebuilds every child. An empty struct sibling reaches `StructArray::new`, which rejects zero fields and panics.
This is reachable through `_partition` on an unpartitioned Iceberg table:
```sql
SELECT arrays_overlap(
array(named_struct('x', x, 'p', _partition)),
array(named_struct('x', x, 'p', _partition)))
FROM t;
```
With ordinary double values `1.0` and `2.0`, Spark returns `[true, true]`. Comet fails with `CometNativeException`. I reproduced this end to end and verified both the Iceberg scan and projection executed natively. The native expression pipeline succeeds with the merge-base implementation and fails with this PR.
Return subtrees without float leaves unchanged **at each recursive call**, or explicitly preserve empty structs. I verified the recursive guard fixes this case while retaining signed-zero and NaN normalization.
Validation: **723 exact-head Rust tests passed**, along with additional slicing, null, and comparison checks. Existing CI is green. The failing Iceberg test used the verified CI merge build, whose affected code matches the reviewed head.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: normalize signed zero in nested float array comparisons [datafusion-comet]
Posted by "divyankshah (via GitHub)" <gi...@apache.org>.
divyankshah commented on PR #5235:
URL: https://github.com/apache/datafusion-comet/pull/5235#issuecomment-5176253897
Reopening, this got closed by accident.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: normalize signed zero in nested float array comparisons [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao merged PR #5235:
URL: https://github.com/apache/datafusion-comet/pull/5235
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: normalize signed zero in nested float array comparisons [datafusion-comet]
Posted by "divyankshah (via GitHub)" <gi...@apache.org>.
divyankshah commented on PR #5235:
URL: https://github.com/apache/datafusion-comet/pull/5235#issuecomment-5176230997
> First pass, focused on the two items that need to change. I have not gone through test coverage or the docs yet, so expect a second round.
>
> Nice find on the root cause, and the diagnosis matches what I see in Spark's `SQLOrderingUtil`.
>
> **1. The normalization is inside the per-row loop, and it is quadratic.**
>
> In `arrays_overlap.rs:431` the comparator is built per row, so `normalize_negative_zero` runs once per row per side. `probe` is `left.value(i)`, and arrow-rs's `GenericListArray::slice` only narrows the offsets and null buffer, it leaves `values` pointing at the entire child buffer. So `list.values()` in the `DataType::List` branch hands back every float in the column, and each row copies all of them.
>
> I checked out the branch and added a nested benchmark to compare against `apache/main`:
>
> benchmark main this PR
> `array<array<double>>` 1024 rows x 4 inner x 8 floats 192.8 us 223.8 ms
> `array<array<double>>` 4096 rows x 4 inner x 8 floats 760.9 us 3.574 s
> `array<array<int>>` 4096 rows x 4 inner x 8 ints 753.0 us 980.3 us
> 4x the rows gives 16x the time, which confirms the shape. The int32 row also regresses 30% despite having no float leaves at all, from the unconditional `ListArray::new` rebuild.
>
> Could you hoist the normalization above the row loop, normalizing `left` and `right` once and slicing per row from the normalized arrays? A cheap recursive `DataType` check to skip types with no float leaf would take care of the int32 case. `array_position`'s `position_fallback` only normalizes once per call so it is fine on the loop question, but it would still benefit from the type gate. Since this touches the same lines as the comparator hoisting in #5194, rebasing on that first as you suggested is probably the easier path.
>
> **2. `normalize_float` already exists, and using it also fixes NaN.**
>
> There is a `normalize_float` at `native/spark-expr/src/math_funcs/internal/normalize_nan.rs:110`, and `hll_plus_plus.rs:126` applies it to Float32/Float64 leaves with `unary()`, which is close to what the leaf arms here do. Reusing it drops the duplicate logic and lets you use `unary()`, which works on the values buffer and preserves the null buffer.
>
> It also closes a second mismatch in the same code path. Spark's `SQLOrderingUtil.compareDoubles` is `if (x == y) 0 else java.lang.Double.compare(x, y)`, and `Double.compare` goes through `doubleToLongBits`, which collapses every NaN payload including the sign bit. Arrow's comparator uses `total_cmp`, which sorts `-NaN` below `-Infinity`. On this branch:
>
> ```
> [[-NaN]] overlaps [[NaN]] => false (Spark returns true)
> ```
>
> `normalize_float` canonicalizes NaN as well as signed zero, so it fixes this for free. Worth noting because the new comment on `test_nested_float_total_order` says NaN matches itself and matches Spark, which currently only holds for canonical positive NaN.
>
> One process note: `gh pr checks` reports no checks on this branch yet, so nothing has been validated by CI. I will get the workflow approved.
Hi @andygrove,
Thanks for the thorough review and feedback, this was really helpful.
Both points addressed in the latest commit:
1) Hoisted normalization above the row loop in arrays_overlap.rs, so it runs once per column instead of rebuilding the whole float buffer on every row. Also gated it behind a has_float_leaf check so non-float types (like your int32 case) skip it entirely. Re-ran your benchmark shape locally: 1024 rows went from 223.8ms to ~650µs, 4096 rows from 3.574s to 1.9ms.
2) Switched to the existing normalize_float (from normalize_nan.rs, already used in hll_plus_plus.rs) instead of the hand-rolled version, so NaN payloads get canonicalized too. Added a test for [[-NaN]] vs [[NaN]].
The array_position's fallback also got the type gate, though it didn't have the per-row issue since it already normalized once per call.
Please let me know if anything needs to be adjusted.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: normalize signed zero in nested float array comparisons [datafusion-comet]
Posted by "divyankshah (via GitHub)" <gi...@apache.org>.
divyankshah closed pull request #5235: fix: normalize signed zero in nested float array comparisons
URL: https://github.com/apache/datafusion-comet/pull/5235
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: normalize signed zero in nested float array comparisons [datafusion-comet]
Posted by "divyankshah (via GitHub)" <gi...@apache.org>.
divyankshah commented on code in PR #5235:
URL: https://github.com/apache/datafusion-comet/pull/5235#discussion_r3761775980
##########
native/spark-expr/src/array_funcs/nested_float_normalize.rs:
##########
@@ -0,0 +1,208 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::math_funcs::internal::normalize_float;
+use arrow::array::{
+ Array, ArrayRef, AsArray, FixedSizeListArray, Float32Array, Float64Array, LargeListArray,
+ ListArray, StructArray,
+};
+use arrow::datatypes::{DataType, Float32Type, Float64Type};
+use std::sync::Arc;
+
+pub(super) fn has_float_leaf(dt: &DataType) -> bool {
+ match dt {
+ DataType::Float32 | DataType::Float64 => true,
+ DataType::List(field) | DataType::LargeList(field) | DataType::FixedSizeList(field, _) => {
+ has_float_leaf(field.data_type())
+ }
+ DataType::Struct(fields) => fields.iter().any(|f| has_float_leaf(f.data_type())),
+ _ => false,
+ }
+}
+
+/// Recursively rebuilds nested arrays with `-0.0` normalized to `0.0` and NaN canonicalized
+/// in any Float32/Float64 leaves.
+pub(super) fn normalize_nested_floats(array: &ArrayRef) -> ArrayRef {
Review Comment:
Hi @peterxcli, thanks for digging into this and for finding the #5166 thread, that's genuinely useful context.
I think the two pieces you're describing are different sizes though. Moving the helper is a small refactor, but making NormalizeNaNAndZero actually recurse into List/Struct doesn't do much on its own, since nothing on the JVM side constructs it with a nested data_type today. The part that actually delivers the perf win needs contraintExpressions.scala to detect the ArrayTransform(arr, x -> NormalizeNaNAndZero(x)) shape and collapse it, and that's real serde work with its own tests, on a node Spark inserts broadly for joins, grouping and sort, not just collect_set or arrays_overlap.
Does that split sound reasonable? I'd rather keep it out of this PR and open it as its own issue instead, referencing this thread and the one andygrove raised on #5166. I'll also add all the deatils to it.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org