andygrove opened a new pull request, #6992: URL: https://github.com/apache/datafusion-comet/pull/6992
## Which issue does this PR close? Closes #6549. Part of #6385. ## Rationale for this change Native `map_from_arrays` and `map_from_entries` tell keys apart by their bits, and they report `Compatible` for every key type, so they run by default. Spark's `ArrayBasedMapBuilder` handles keys that hold a `FLOAT` or `DOUBLE` differently: - It finds a duplicate top-level key in a `HashMap` of boxed values, so NaNs with different bits are one key. For `map_from_arrays(array(d, -d), array(1, 2))` over a NaN `d`, Spark raises `DUPLICATED_MAP_KEY` and Comet returns a map with two NaN keys. - It finds a duplicate struct or array key in a `TreeMap` in Spark's SQL ordering, where `-0.0` equals `0.0` and all NaNs are equal. Comet keeps both keys there too. - From Spark 4.0 it also normalizes a top-level key before the lookup, unless `spark.sql.legacy.disableMapKeyNormalization` is set. So `0.0` and `-0.0` are one key, and `map_from_entries`, which always rebuilds the map, stores `0.0` for a `-0.0` key. Comet keeps `-0.0` and accepts both zeros. The first two also apply on Spark 3.4 and 3.5, which #6549 doesn't describe. I reproduced all three on `main` with Spark 4.1, and the first two with Spark 3.5. #6682 implements these rules in the native builders, but it is stacked on #5854. Until then, this falls back for these key types, as #6549 suggests. ## What changes are included in this PR? - `MapBuilderKeySupport` in `maps.scala` reports `Incompatible` when the key type holds a `FLOAT` or `DOUBLE` at any depth, on every Spark version, and both `CometMapFromArrays` and `CometMapFromEntries` check it. `map_from_entries` then runs Spark's code through the codegen dispatcher, as it already does for `BinaryType` keys and `LAST_WIN`, and `map_from_arrays` falls back to Spark, as it does for `LAST_WIN`. A float value type still runs natively. - Docs: a "Map construction" section in the floating-point compatibility guide, the notes for both functions in `expressions.md`, the contributor guide's rule for map construction, and the map audit, which said 4.0 left their semantics unchanged. ## How are these changes tested? - `map_builders_float_keys.sql` runs on every version. It covers a lone `-0.0` key, compared with the running Spark's answer, NaN keys that differ only in the sign bit, and struct and array keys whose floats differ only in sign, plus a float value type that stays native. `map_builders_float_keys_spark_4_0.sql` checks that Spark 4.0+ raises `DUPLICATED_MAP_KEY` for `0.0` and `-0.0`. `map_builders_float_keys_legacy.sql` checks that with `spark.sql.legacy.disableMapKeyNormalization` they stay two keys, which also shows that the dispatcher sees the session's flag. - With the new check disabled, all three fixtures fail on Spark 4.1. - All 32 `expressions/map/` fixtures pass on Spark 3.4, 3.5, 4.0, 4.1 and 4.2. `CometMapExpressionSuite` and `CometFloatSemanticsSuite` pass on 4.1 (445 tests). -- 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]
