andygrove commented on PR #4782: URL: https://github.com/apache/datafusion-comet/pull/4782#issuecomment-5180708969
Thanks for the thorough pass @mbutrovich — all items addressed in 47b0dfd. Detail is in the individual threads; summarising the one item that changed the conclusion, since it affects how you read the diff. **The `-0.0` fix is version-dependent, so it is gated rather than dropped.** Your reasoning is correct for Spark 3.4 through 4.1, and I verified each claim against the release branches. But Spark 4.2.0 reversed the behaviour: [SPARK-57329](https://issues.apache.org/jira/browse/SPARK-57329) treats the split `-0.0`/`0.0` counts as Spark's own correctness bug and normalizes the key at update time. It landed in `branch-4.2` after `v4.2.0-rc1`, so released 4.2.0 has it, and this repo already builds a `spark-4.2` profile. | Spark | `-0.0` and `0.0` | `mode` of (-0.0 x2, 0.0 x2, 5.0 x3) | | ------------ | ---------------- | ----------------------------------- | | 3.4 - 4.1 | distinct keys | `5.0` | | 4.2.0+ | one key | `0.0` | So simply dropping the `-0.0` branch would have fixed 3.4-4.1 and newly broken 4.2. Instead there is now a `normalize_neg_zero` field on the `Mode` proto message, set from `isSpark42Plus` in the serde (the same shape as `BloomFilterVersion` directly above it, and `setIsSpark4Plus` in `CometCast`), with the fold gated on it in the native accumulators. `NaN` canonicalization stayed unconditional. **The existing signed-zero coverage was vacuous, for a reason worth knowing.** You flagged that the group `b` fixture used `-0.0` and passed CI anyway. The masking was in the data: `CAST(-0.0 AS DOUBLE)` does not produce a negative zero, because an unsuffixed `-0.0` is a `DecimalType` literal and `Decimal` has no signed zero. I read `doubleToRawLongBits` back out of the Parquet table and every "negative" zero was `bits=0`. Fixtures now use `-0.0D`, and I confirmed the new `mode_signed_zero` case is not vacuous by forcing the flag to the wrong value and watching it fail. Everything else from the review is in: `size()` now counts key heap bytes, the redundant `default_value` is gone, `debug_assert!` at both grouped emit sites, a comment recording that the `ScalarValue` map is intentionally type-generic, a `timestamp_ntz` compared query, and a new `mode_within_group.sql` for the Spark 4.x ordered forms. One of my own additions there was wrong and is corrected: `mode(col, false)` should *not* fall back, because `ModeBuilder` rewrites it to the plain `Mode(child)`. Verified on Spark 4.1: 11 Rust unit tests, 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]
