viirya commented on code in PR #6992:
URL: https://github.com/apache/datafusion-comet/pull/6992#discussion_r4276506042
##########
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:
`MapKeySupport` at the top of this file already gates float keys with the
same `containsType` check. Its reason at L36-38 and its scaladoc at L54 say
Spark normalizes floating-point map keys and that `ArrayBasedMapBuilder`
rewrites `-0.0` to `+0.0`, without saying which versions. Your research in this
PR shows that only holds on 4.0+ when
`spark.sql.legacy.disableMapKeyNormalization` is unset. So two objects in the
same file now describe the same builder differently. Could you correct that
description here? Or fold `hasFloatingPointKey` and its reason into
`MapKeySupport`, so there is one place that explains how Spark treats float map
keys?
##########
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))."
Review Comment:
This PR says `Closes #6549`, and so does #6682. If this one merges first,
#6549 closes while the native builders still compare the keys' bits. This
reason string, which ends up on the generated compat page, and the docs would
then point users at a closed issue for a difference that is still there. Could
the description here say `Part of #6549` instead, so the issue stays open until
#6682 fixes it natively?
##########
spark/src/main/scala/org/apache/comet/serde/maps.scala:
##########
@@ -132,6 +132,28 @@ object CometMapExtract extends
CometExpressionSerde[GetMapValue] {
}
}
+/**
Review Comment:
This scaladoc repeats the reason string below almost word for word. The same
explanation also appears in both audit entries, both floating-point guides,
`expressions.md`, and the headers of the three SQL fixtures. When #6682 lifts
this gate, all of them have to change together, and any one that gets missed
goes stale. Could the scaladoc just say why the gate lives here and point at
`floatingPointReason` for the details? The new "Map construction" section in
the user-guide `floating-point.md` could also be trimmed to the user-facing
outcome, since the reason already feeds the generated compat page for these
expressions.
##########
spark/src/test/resources/sql-tests/expressions/map/map_builders_float_keys.sql:
##########
@@ -0,0 +1,94 @@
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements. See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership. The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied. See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+
+-- Map keys that hold a FLOAT or DOUBLE (#6549). Spark finds a duplicate
top-level key in a HashMap
Review Comment:
#6682 also adds `map_builders_float_keys.sql` and
`map_builders_float_keys_legacy.sql` in this directory, with different
contents. Whichever lands second will collide with the other. Could you rename
these, or agree with #6682 on which PR owns the fixtures, so the rebase doesn't
silently drop cases from either 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]
