andygrove commented on PR #6866: URL: https://github.com/apache/datafusion-comet/pull/6866#issuecomment-6125083662
@viirya you're right, thanks for catching it. `Divide`'s guard goes through the comparison builder, but `Remainder`'s non-ANSI guard is built natively in `null_if_zero_primitive` as a plain `BinaryExpr`, and `x % -0.0` is NULL there only because DataFusion 55's `apply_cmp` folds the zeros before Arrow's `eq`. Until now the divisor normalization hid that in Projects and Filters. Since this PR is what takes that cover away, I fixed it here in e7d77574f, on the native side rather than in the serde. `null_if_zero_primitive` now builds its check with `spark_comparison(..., FloatOperands::Normalize)`, which is also what @rich7420 suggested, so every `Remainder` plan gets it without a second `IF` from the serde, and integer and decimal divisors keep the same `BinaryExpr`. `test_modulo_divide_by_negative_zero_float_non_ansi` covers Float32 and Float64 with column and literal `-0.0` divisors. Those values also come out NULL with the old guard because of the folding, so `test_non_ansi_float_divisor_guard_compares_in_spark_ordering` checks that the guard compares with a `SparkComparison`, and it fails against the old `BinaryExpr`. `arithmetic.sql` also gets `1.0D % (-d)` over a column holding `0.0`. I updated the rationale in the description to match. -- 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]
