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