Re: [PR] [SPARK-59598][SQL] GetMapValue and ElementAt must not match a null map key [spark]

["qingfureal (via GitHub)" <gi...@apache.org>]

qingfureal commented on code in PR #58875:
URL: https://github.com/apache/spark/pull/58875#discussion_r4212989174


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala:
##########
@@ -572,12 +577,18 @@ trait GetMapValueUtil extends BinaryExpression with ImplicitCastInputTypes {
     val mask = cap - 1
     var i = 0
     while (i < len) {
-      var h = hashKeyOnDriver(keys.get(i, keyType), keyType) & mask
-      // Open addressing with linear probing; duplicates take the next free slot so that the
-      // lookup (which stops at the first match) returns the first-inserted index -- matches
-      // [[buildHashIndex]] / [[ArrayBasedMapData]] first-wins semantics.
-      while (buckets(h) != -1) h = (h + 1) & mask
-      buckets(h) = i
+      // Null keys are skipped, as in [[buildHashIndex]]. This is load-bearing here: the
+      // generated probe reads a candidate key with a primitive getter, which on a null slot
+      // returns the type's zero value, so a bucket for a null key would let a lookup of 0
+      // match it.
+      if (!keys.isNullAt(i)) {
+        var h = hashKeyOnDriver(keys.get(i, keyType), keyType) & mask
+        // Open addressing with linear probing; duplicates take the next free slot so that the
+        // lookup (which stops at the first match) returns the first-inserted index -- matches
+        // [[buildHashIndex]] / [[ArrayBasedMapData]] first-wins semantics.

Review Comment:
   Done in 6b6aba9. The comment now attributes first match only to the linear scan and the prebuilt lookups, and says `ArrayBasedMapData` leaves duplicate-key behavior undefined.



##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala:
##########
@@ -547,9 +547,14 @@ trait GetMapValueUtil extends BinaryExpression with ImplicitCastInputTypes {
     val hm = new java.util.HashMap[Any, Int]((len * 1.5).toInt)
     var i = 0
     while (i < len) {
-      // putIfAbsent preserves first-match semantics for maps with duplicate keys (allowed at
-      // the physical level by [[ArrayBasedMapData]]), matching the linear scan path.
-      hm.putIfAbsent(keys.get(i, keyType), i)
+      // Null keys are skipped so this map describes the same key set as the other lookup
+      // paths. Unlike them it is a no-op rather than a fix: a non-null lookup key can never

Review Comment:
   Good catch. Reworded in 6b6aba9: a non-null key can share a bucket with the null entry, and it's the equality check that keeps them from matching.



##########
docs/sql-migration-guide.md:
##########
@@ -37,6 +37,7 @@ license: |
 - Since Spark 4.4, `spark.sql.sources.v2.bucketing.partition.filter.enabled` defaults to `true`. In a storage-partitioned join, the partition key groups pushed down to both sides may now be narrowed to those that can produce output for the join type, instead of always taking the union of the two sides' groups: an inner or semi join may keep only the groups present on both sides, a join that keeps or tests every left row (left outer, left anti, left single, existence) keeps the left side's groups, a right outer join keeps the right side's, and a full outer join is unaffected; a CROSS join carrying an equality condition is treated like an inner join. The narrowing is skipped when either side's partitioning may contain unknown partition keys, as after a shuffle on one side, so such a join keeps the full union. A group that is dropped is not scanned at all, so a query may read fewer files. To restore the previous behavior, set `spark.sql.sources.v2.bucketing.partition.filter.enabled` to
  `false`.
 - Since Spark 4.4, `spark.sql.sources.v2.bucketing.partitionKeyOrdering.enabled` defaults to `true`. A V2 scan that reports a keyed partitioning but no explicit ordering now also reports itself sorted by its partition key expressions, because every row of such a partition evaluates them to the same value. Spark can then drop a `Sort` it would otherwise place above the scan. An aggregate whose grouping is exactly the partition key expressions may also be planned as a sort aggregate rather than a hash aggregate, because `spark.sql.execution.replaceHashWithSortAgg` keys off the reported ordering. To restore the previous behavior, set `spark.sql.sources.v2.bucketing.partitionKeyOrdering.enabled` to `false`.
 - Since Spark 4.4, `spark.sql.sources.v2.bucketing.preserveKeyOrderingOnCoalesce.enabled` defaults to `true`. When `GroupPartitionsExec` merges several input partitions that share one partition key value into a single output partition, it now keeps sort orders over the partition key expressions in the ordering it reports, since those expressions are constant within the merged partition. Orders over other columns are still dropped, as the concatenation invalidates them, and a join that reduced the partition keys onto a common key space (see `spark.sql.sources.v2.bucketing.allowCompatibleTransforms.enabled`) reports no order, since the merged partitions then share only the reduced key. This can remove a `Sort` downstream of the merge. To restore the previous behavior, set `spark.sql.sources.v2.bucketing.preserveKeyOrderingOnCoalesce.enabled` to `false`.
+- Since Spark 4.4, looking a key up in a map never matches a null key, so `m[k]` and `element_at(m, k)` return NULL rather than the null key's value. Map keys cannot normally be null, but the file-format readers build maps directly and do not reject them, so a null key can reach a query from sources such as Parquet or a Hive text table. Previously a lookup of a primitive key type's zero value could match a null key and return its value -- including when the map also held a real entry for that key, whose value was then shadowed. Lookups against such a map could also fail with a `NullPointerException` for non-primitive key types; they now return NULL as well.

Review Comment:
   Done in 6b6aba9. The note now covers both outcomes: the value of a non-null entry for `k` if the map has one (previously shadowed when it followed the null key), and NULL otherwise.



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org