peterxcli commented on PR #5166: URL: https://github.com/apache/datafusion-comet/pull/5166#issuecomment-5159819008
@andygrove thanks for the review! review change is pushed, issue is opened per your review. please take another look, thanks! > 1. Gate incompatibility reasons by Spark version `getIncompatibleReasons()` now returns `Nil` on Spark 4.2+, while retaining the compatibility warning for Spark 3.4–4.1. > 2. Normalize `Complete` aggregates Floating-point normalization now applies to both `Partial` and `Complete` modes. Merge modes remain unchanged because they consume aggregate buffers rather than raw rows. > 3. Assert collected values The regression queries now compare `sort_array(collect_set(...))` results instead of only checking collection sizes. > 4. Restore scalar coverage Added float and double cases covering ordinary values, NULL, NaN, positive/negative infinity, and signed zero. Tests run with Parquet dictionary encoding both enabled and disabled. > 5. Expand nested-value coverage Added null structs, null arrays, arrays containing null elements, `array<struct<double>>`, and `struct<array<double>>`, including NaN and signed-zero values. > 6. Test global aggregation Added a repartitioned `collect_set` query without `GROUP BY` to exercise partial-buffer merging into a global aggregate. > 7. Check Spark 4.2 SQL-test diffs Confirmed that no `dev/diffs/4.2*.diff` or Spark 4.2 Spark SQL Tests job currently exists. No diff file was modified. > 8. Document both pre-4.2 divergences The compatibility messages now explain that Comet deduplicates NaNs but distinguishes `-0.0` from `0.0`, while pre-4.2 Spark does the opposite. Negative-zero fixtures now use string casts so the sign is preserved. > 9. File a Spark 4.2 tracking issue Filed https://github.com/apache/datafusion-comet/issues/5209 to enable the two SPARK-57298 `DataFrameAggregateSuite` tests once the Spark 4.2 diff and CI job are added. > 10. Keep the original compatibility wording Restored the original “Before Spark 4.2…” wording and added only the missing signed-zero explanation. > 11. Remove the mode explanation comment Removed the added explanatory comment above `normalizeFloatingNumbers`; the `Partial | Complete` behavior remains unchanged. -- 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]
