andygrove commented on code in PR #6992:
URL: https://github.com/apache/datafusion-comet/pull/6992#discussion_r4298473812
##########
spark/src/test/resources/sql-tests/expressions/map/map_from_arrays_dedup_policy.sql:
##########
@@ -17,10 +17,14 @@
-- Verifies that `map_from_arrays` falls back to Spark when
`spark.sql.mapKeyDedupPolicy` is set
-- to `LAST_WIN`. Spark's ArrayBasedMapBuilder keeps the last occurrence of
each duplicate key;
--- Comet's native `map` scalar has no LAST_WIN path, so it must fall back. The
default `EXCEPTION`
--- mode agrees with Comet and is covered by `map_from_arrays.sql`.
+-- Comet's native `map` scalar has no LAST_WIN path. `CometMapFromArrays`
mixes in
+-- `CodegenDispatchFallback`, so its native `Incompatible` normally routes
through the JVM codegen
+-- dispatcher; we disable the dispatcher here so the incompat branch surfaces
as a genuine Spark
+-- fallback rather than in-pipeline codegen. The default `EXCEPTION` mode
agrees with Comet and is
+-- covered by `map_from_arrays.sql`.
-- Config: spark.sql.mapKeyDedupPolicy=LAST_WIN
+-- Config: spark.comet.exec.scalaUDF.codegen.enabled=false
Review Comment:
Added `map_builders_float_key_last_win.sql` in 2c29974642. It runs
`map_from_arrays` and `map_from_entries` with the dispatcher on under
`LAST_WIN` and a key that really repeats: a string key, a `DOUBLE` and a
`FLOAT` pair of NaNs that differ only in the sign bit, and `0.0` with `-0.0` in
both orders so the stored key is checked too. The zero queries run on every
version rather than only 4.0+, since the dispatcher runs Spark's builder either
way and the result has to match what the running Spark does. I put it in its
own file instead of `routing_map_legacy_enabled.sql` so one fixture covers both
builders, and the two dedup-policy fixtures now point at it. It fails on 4.1
when `CodegenDispatchFallback` is dropped from either serde, and the `map`
fixtures pass on 3.4, 3.5, 4.0, 4.1 and 4.2.
##########
spark/src/main/scala/org/apache/comet/serde/maps.scala:
##########
@@ -132,6 +132,28 @@ object CometMapExtract extends
CometExpressionSerde[GetMapValue] {
}
}
+/**
+ * Shared gate for the native map builders, `map_from_arrays` and
`map_from_entries`, which tell
+ * keys apart by their bits. Spark's `ArrayBasedMapBuilder` finds a duplicate
`FLOAT` or `DOUBLE`
+ * key in a `HashMap` of boxed values, where every NaN is one key, and a
duplicate struct or array
+ * key in a `TreeMap` in Spark's SQL ordering, where `-0.0` equals `0.0` too.
From Spark 4.0 it
+ * also normalizes a `FLOAT` or `DOUBLE` key before the lookup, unless
+ * `spark.sql.legacy.disableMapKeyNormalization` is set, so the two zeros are
one key and
+ * `map_from_entries` stores `0.0`. A key type that holds a float at any depth
is therefore
+ * `Incompatible` on every version.
+ */
+private object MapBuilderKeySupport {
+ val floatingPointReason: String =
+ "Spark finds duplicate map keys that hold a `FLOAT` or `DOUBLE` by value,
so NaNs with " +
+ "different bits are one key, and so are `-0.0` and `0.0` inside a struct
or array key. " +
+ "From Spark 4.0, a `-0.0` key is also normalized to `0.0`, unless " +
+ "`spark.sql.legacy.disableMapKeyNormalization` is set. Comet's native
map construction " +
+ "compares the keys' bits
([#6549](https://github.com/apache/datafusion-comet/issues/6549))."
+
+ def hasFloatingPointKey(dataType: MapType): Boolean =
Review Comment:
Fixed in 2c29974642. The `MapKeySupport` scaladoc now says Spark looks a key
up with the interpreted ordering over the map's stored keys, without the
normalization claim. The `element_at_map.sql` comment, and the same sentence in
`get_map_value.sql` and the `CometMapExpressionSuite` comment, now say only
that Spark compares with `nanSafeCompareDoubles`, so a `-0.0` lookup finds the
`+0.0` key.
--
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]
