viirya commented on code in PR #6992:
URL: https://github.com/apache/datafusion-comet/pull/6992#discussion_r4289237080
##########
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:
Thanks, the new reason reads right. Two spots still carry the old claim. The
scaladoc at L51-52 still says the lookup runs over the keys
`ArrayBasedMapBuilder` stored "having first normalized them", and the comment
above the first `expect_fallback` in `element_at_map.sql` (L59) still says
Spark stores `-0.0` map keys as `+0.0`. Both only hold on 4.0+ without the
legacy flag. Could you qualify or drop those too?
##########
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:
With the dispatcher off here, nothing now runs the default path for
`LAST_WIN`, which after this PR is the dispatcher, with a key that actually
repeats. Could you add an `expect_dispatch(map_from_arrays)` query under
`LAST_WIN` with the dispatcher on? A `DOUBLE` key would be the most useful: two
NaNs that differ in the sign bit, plus `0.0` and `-0.0` gated on 4.0+. A
repeated key sends `from()` through `build()`, so both the kept value and the
stored key get checked. `routing_map_legacy_enabled.sql` has the same gap for
`map_from_entries`, since its data has no repeated key, so a matching query
there would cover both builders.
--
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]
