Re: [PR] fix: normalize signed zero in nested float array comparisons [datafusion-comet]

["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