andygrove commented on PR #4817: URL: https://github.com/apache/datafusion-comet/pull/4817#issuecomment-5181010316
Thanks @mbutrovich — all three addressed in d7bc497. Detail in the threads; two things worth pulling up here. **The `-0.0` fix needed no version gate, unlike the `mode` one.** Since you cross-linked these, I checked: `SQLOrderingUtil.compareDoubles` is byte-identical on `branch-3.4` through `master`, so the tie is stable across the matrix. That is the opposite of #4782, where Spark 4.2.0 reversed `mode`'s behaviour in SPARK-57329 and the fold had to become version-dependent. Both sites now carry a comment naming the governing Spark path, as you asked, and each says explicitly not to "fix" it to match the other. **The same bug applies to `NaN`, which the review did not cover.** `Double.compare` goes through `doubleToLongBits`, so a sign-bit-set `NaN` is the same value as a positive one and still sorts above `+Infinity` — but Arrow's raw-bit encoding places it below `-Infinity`. So `max_by` picked the wrong row for `-NaN` orderings as well. The canonicalization folds `NaN` alongside the zeros, which makes the row bytes agree with `compareDoubles` on both counts. The pre-existing `max_by_nan_is_largest` used a positive `NaN` and passed either way. **On the SQL fixtures: I could not write a meaningful one, and I think that is the right answer rather than a gap.** The divergence only shows up when the two zeros tie with different values attached, and which tied row then wins depends on inter-partition merge order — these tables span 5 partitions, so that is precisely the case Spark documents as non-deterministic. I built your fixture and measured it with the canonicalization disabled: Spark and the broken native path both returned `g1->2, g2->4`. It would have been vacuous *and* pinning unspecified behaviour. So the added fixtures give the tied rows equal values, which is deterministic under any merge order and still exercises the canonicalization end to end, with a comment explaining the shape. The order-dependent guarantee is in the Rust tests, where row order is explicit: both directions for `max_by` and `min_by`, across-batch, grouped, and `Float32`. All six fail with the canonicalization reverted to a no-op and pass with it — I checked rather than assuming, which is how I caught that my first `Float32` test was vacuous (its row order happened to agree with the broken ranking). Also worth knowing for any future float fixture: `CAST(-0.0 AS DOUBLE)` stores `+0.0`, because an unsuffixed `-0.0` is a `DecimalType` literal and `Decimal` has no signed zero. `-0.0D` is required. Same trap that made the `mode` fixture vacuous in #4782. Verified on Spark 4.1: 20 Rust tests in `max_min_by`, and all 23 files in `expressions/aggregate/`. -- 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]
