andygrove opened a new pull request, #6400: URL: https://github.com/apache/datafusion-comet/pull/6400
## Which issue does this PR close? Part of #6385. This is the "consolidate the native helpers into the shared module (no behavior change)" step. ## Rationale for this change Spark's `-0.0` and NaN rules were re-implemented in each native expression that needs one: three copies of `NormalizeNaNAndZero`, three of `compareDoubles`, two nested comparators, and six inline `-0.0` checks in the hash macros. Putting each rule in one place means the later steps in #6385 each change one function, not every copy. Those steps are canonicalizing NaN in `hash`/`xxhash64`, normalizing comparisons, fixing `min`/`max` and friends, and nested ordering. This PR does not change behavior. The fixes come in the follow-up PRs. ## What changes are included in this PR? A new `native/spark-expr/src/float_semantics/` module. Its module docs list which Spark rule each helper implements. | Helper | Spark rule | Replaces | | --- | --- | --- | | `normalize_float` | `NormalizeNaNAndZero` | `math_funcs/internal/normalize_nan.rs`, the closures in `canonicalize_float_ordering` (`max_min_by.rs`), the `mode.rs` macro | | `canonicalize_nan` | `java.lang.Double.equals` | `OverlapKey` for `f32`/`f64` (`arrays_overlap.rs`), `mode` before Spark 4.2 | | `compare_floats` | `SQLOrderingUtil.compareDoubles` | `spark_double_cmp` (`percentile.rs`), the inline checks in `float_extrema` (`array_extrema.rs`) and `position_float` (`array_position.rs`) | | `hash_input` | `Murmur3Hash` / `XxHash64` input | the six inline `-0.0` checks in `hash_funcs/utils.rs` | | `normalize_floats` | `NormalizeNaNAndZero` on a flat array | `NormalizeNaNAndZero::normalize_array`, `normalize_floats` (`hll_plus_plus.rs`), `canonicalize_float_ordering` (`max_min_by.rs`) | | `normalize_nested_floats`, `has_float_leaf`, `NormalizeNestedFloats` | `NormalizeNaNAndZero` at any depth | moved from `array_funcs/nested_float_normalize.rs` | | `NormalizeNaNAndZero::wrap_if_needed` | | the check in `create_normalized_key_expr` (`planner.rs`) | | `spark_comparator` | `compareDoubles` inside lists and structs | `nested_equality` (`nested_comparison.rs`) and `spark_comparator` (`array_extrema.rs`) | Notes for review: - `spark_comparator` takes a left and a right array, which the nested ordering work (#6157, #5507) needs. Nulls sort first at every level, as `array_min`/`array_max` require, and nested `=`/`IN` only check for `Equal`, so both keep their results. - `hash_input` keeps today's behavior: it folds `-0.0` but does not canonicalize NaN. Its doc comment says so and points at #6385, since the hash fix is the next step and now lands in this one function. - `percentile.rs`, `array_position.rs` and `float_extrema` were not in the issue's list, but each held a copy of `compareDoubles`. - Left out because they change behavior: `Map` support in the nested normalization, and the hash NaN fix. - `NormalizeNaNAndZero` and `NormalizeNestedFloats` are still re-exported from the crate root. `hash_array_primitive_float!` drops its two type parameters, which only fed the inline `-0.0` check. - One difference is unreachable from Spark, which only produces `List`: the old `array_min`/`array_max` comparator fell back to Arrow's float order inside a `LargeList` or `FixedSizeList`, and the shared comparator uses Spark's order there too. The type-mismatch internal error, which is also unreachable, now reads "Spark comparison requires matching types". ## How are these changes tested? The existing tests pass unchanged. These were run on macOS aarch64 with the default Spark 4.1 profile: - `datafusion-comet-spark-expr` unit tests: 1024 passed. `datafusion-comet` unit tests: 551 passed, 5 ignored. - `CometSqlFileTestSuite`: 578 passed. This includes the float fixtures for `array_min`/`array_max`, `arrays_overlap`, `array_position`, `mode`, `max_by`/`min_by`, `approx_count_distinct`, `percentile`, `hash` and nested `IN`. - `CometArrayExpressionSuite` and `CometHashExpressionSuite`: 109 passed. `CometWindowExecSuite` and `CometNativeShuffleSuite`: 124 passed. These cover window partition keys and range partition bounds. New unit tests: - Each rule against Spark's order, including sign-bit and payload NaNs. - The comparator at every nesting depth, with left and right arrays of different lengths and different values hidden under null list slots. - `normalize_floats` leaving nested arrays unchanged. - `wrap_if_needed` not wrapping a key twice. - A `percentile` NaN-ordering test, replacing the test of the removed `spark_double_cmp`. I also checked equivalence with a throwaway differential test, not committed, that runs the old helpers verbatim against the new ones: - The rules on edge values and 2,000 random bit patterns, for `f32` and `f64`. - The normalizers on random nested arrays: 17 shapes, sliced and unsliced, with byte-identical buffers. - `spark_comparator` against the old `nested_equality` on 2.04M pairs, and against the old `array_extrema` comparator on 1.10M pairs. I also planted six mutations in the new code, and the committed tests catch each one. -- 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]
