mkleen opened a new pull request, #26595: URL: https://github.com/apache/datafusion/pull/26595
## Which issue does this PR close? - Part of #26138 (step 2: bring the existing `-0.0` handling into one place). - Prepares #26091. - Depends on #<benchmark PR>, which adds the benchmark used below. Only the last two commits belong to this PR. ## Rationale for this change DataFusion treats `-0.0` and `0.0` as the same number (#22835). The code that makes this work was copied into several different places. Each copy has to stay in sync with the others: if one is forgotten when the rule changes, joins and `GROUP BY` can quietly return wrong results. #26138 proposes keeping all of this in one place, and #26091 (making all NaNs equal) will need to change the rule. This PR does the first part: it moves the existing code into one module without changing how queries behave. ## What changes are included in this PR? **One home for the rule.** A new module, `datafusion_common::float_semantics`, now decides when two floats count as the same value and how floats are ordered. Everything that used its own copy of the rule now asks this module: - hashing (used by hash joins and grouping) - `GROUP BY`, `DISTINCT` and the `array_*` set functions - comparisons like `=` and `IS DISTINCT FROM` - the join types that compare keys - `min`, `max`, `ORDER BY ... LIMIT` (TopK) and range repartitioning **No change in results.** Floats are still sorted exactly as before. Changing the sort order, so that `-0.0` and `0.0` sort as ties the way PostgreSQL does, is a separate step in #26138. After this PR that change only needs to be made in one place. **Faster in the common case.** Before comparing floats, DataFusion checks whether a column contains any `-0.0` that needs to be replaced. Usually there is none. That check used to look at one value at a time. It now looks at 64 values at once, which the CPU can do much faster. This also helps #26091: with the old check, handling NaN would have been slower than simply copying every column. The Comet project found the same (apache/datafusion-comet#6590). On my local run (Apple M1 Max, 8,192 values per batch), the common case where a column has no -0.0 is about 4x faster than main for f64 (2.65 µs → 0.69 µs) and about 7x faster for f32 (2.66 µs → 0.37 µs). When there is a -0.0 to replace, it's about 1.8x faster for f64 (4.27 µs → 2.39 µs) and 2.7x faster for f32 (3.15 µs → 1.17 µs). ## What is the testing strategy for this PR? Query results don't change, so the existing tests are the main check. They all pass without modification, including the `-0.0` tests (`negative_zero.slt`) and the tests for arrays, aggregates, grouping, joins, TopK and repartitioning. Added: - unit tests for the new module - a test that places a `-0.0` at every position in a column, to make sure the faster check never misses one The benchmark from #<benchmark PR> now calls the new function name. Its cases are unchanged, so results can be compared directly with `main`. ## Are there any user-facing changes? Query results are the same. For code that uses DataFusion as a library: `normalize_float_zero` and `normalize_float_zero_scalar` in `datafusion_common::utils` are deprecated. They still work. Use `canonicalize_float_array` and `canonicalize_float_scalar` from `datafusion_common::float_semantics` instead. -- 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]
