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: [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]
