From 21df1853e02f8f54b42d35c5134a6fd939b3c744 Mon Sep 17 00:00:00 2001 From: Qing Fu Date: Wed, 16 Sep 2026 16:16:33 -0700 Subject: [PATCH] [SPARK-59598][SQL] GetMapValue and ElementAt must not match a null map key GetMapValue (m[k]) and ElementAt (element_at(m, k)) share their key lookup in GetMapValueUtil. When the map's key array contains a null, the lookup matches that null key against the key type's zero value and returns its value instead of NULL. Two independent mechanisms cause this, so it reproduces with codegen both on and off: 1. The interpreted linear scan compares with the key type's natural ordering, which unboxes its arguments for a primitive key type; Scala's BoxesRunTime.unboxToInt(null) is 0, so equiv(null, 0) is true. 2. The generated code reads the candidate key with a primitive getter, which is not null-aware and returns 0 for a null slot. This affects the generated linear scan, and also the hash probe added by SPARK-55959, whose bucket table hashes the unboxed null into the same bucket as 0. Null map keys are reachable because the file-format readers construct ArrayBasedMapData directly rather than through ArrayBasedMapBuilder; ParquetRowConverter documents the case as undefined behavior in the tree. The lookup key is never null, since both expressions are null-safe on the ordinal, so a null map key can never be the key being looked up. Skip null keys on all four paths: the interpreted and generated linear scans, and the driver-side hash bucket table and hash index. --- .../expressions/complexTypeExtractors.scala | 40 +++++++++---- .../expressions/ComplexTypeSuite.scala | 57 +++++++++++++++++++ 2 files changed, 87 insertions(+), 10 deletions(-) diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala index 2f9c1e2dd704b..5966f25c3e34b 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala @@ -547,9 +547,13 @@ 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: the lookup key is never null (both eval and codegen are + // null-safe on the ordinal), so a null key can never match. See [[LinearExecutor]]. + if (!keys.isNullAt(i)) { + // 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) + } i += 1 } hm @@ -572,12 +576,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. + while (buckets(h) != -1) h = (h + 1) & mask + buckets(h) = i + } i += 1 } (buckets, mask) @@ -651,7 +661,12 @@ trait GetMapValueUtil extends BinaryExpression with ImplicitCastInputTypes { var i = 0 var found = false while (i < length && !found) { - if (ordering.equiv(keys.get(i, keyType), ordinal)) { + // Skip null keys. `ordinal` is never null here (the caller is null-safe on both + // inputs), so a null key can never be the one being looked up. The guard is required + // rather than cosmetic: `ordering` is the key type's natural ordering, and for a + // primitive key type it unboxes its arguments, turning a null key into the type's + // zero value -- which would make a lookup of 0 match it. + if (!keys.isNullAt(i) && ordering.equiv(keys.get(i, keyType), ordinal)) { found = true } else { i += 1 @@ -693,6 +708,11 @@ trait GetMapValueUtil extends BinaryExpression with ImplicitCastInputTypes { |int $index = -1; | |for (int $i = 0; $i < $length; $i++) { + | // Skip null keys: the getter below is not null-aware, so a null key would read + | // back as the java type's default value and could match the lookup key. + | if ($keys.isNullAt($i)) { + | continue; + | } | $keyJavaType $loopKey = ${CodeGenerator.getValue(keys, keyType, i)}; | if (${ctx.genEqual(keyType, loopKey, eval2)}) { | $index = $i; diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ComplexTypeSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ComplexTypeSuite.scala index abc28ff203f8d..3730fe1b1ca84 100644 --- a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ComplexTypeSuite.scala +++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ComplexTypeSuite.scala @@ -399,6 +399,63 @@ class ComplexTypeSuite extends SparkFunSuite with ExpressionEvalHelper { } } + test("SPARK-59598: map lookup must not match a null key") { + // A map whose key array contains a null. `ArrayBasedMapBuilder` rejects null keys, but the + // file-format readers build `ArrayBasedMapData` directly and do not: see the "the parquet + // map may contains null or duplicated map keys" note in `ParquetRowConverter`. A lookup + // must never match such a key, whichever executor and code path is used. + def mapWithNullKey(keys: Array[Any], values: Array[Any]): ArrayBasedMapData = + new ArrayBasedMapData(new GenericArrayData(keys), new GenericArrayData(values)) + + // Primitive keys are the dangerous case: a null slot read with a primitive getter (codegen) + // or unboxed by the natural ordering (interpreted) yields 0, so `m[0]` used to return the + // null key's value instead of null. + val intKeyMap = mapWithNullKey(Array(null, 1), Array(10, 20)) + val intMapType = MapType(IntegerType, IntegerType) + + // Non-foldable input -> LinearExecutor, regardless of threshold. + withSQLConf(SQLConf.MAP_LOOKUP_HASH_THRESHOLD.key -> "0") { + val mapRef = BoundReference(0, intMapType, nullable = true) + val row = create_row(intKeyMap) + assert(!GetMapValue(mapRef, Literal(0)).usesFoldableHashLookup) + + checkEvaluation(GetMapValue(mapRef, Literal(0)), null, row) + checkEvaluation(ElementAt(mapRef, Literal(0)), null, row) + // Non-null keys in the same map still resolve, i.e. the scan skips the null slot rather + // than stopping at it. + checkEvaluation(GetMapValue(mapRef, Literal(1)), 20, row) + checkEvaluation(ElementAt(mapRef, Literal(1)), 20, row) + } + + // Foldable input above the threshold -> PrebuiltHashExecutor. The generated probe reads a + // candidate key with a primitive getter, so a bucket built for a null key would also match + // a lookup of 0. + withSQLConf(SQLConf.MAP_LOOKUP_HASH_THRESHOLD.key -> "0") { + val foldable = Literal.create(intKeyMap, intMapType) + assert(GetMapValue(foldable, Literal(0)).usesFoldableHashLookup) + + checkEvaluation(GetMapValue(foldable, Literal(0)), null) + checkEvaluation(ElementAt(foldable, Literal(0)), null) + checkEvaluation(GetMapValue(foldable, Literal(1)), 20) + checkEvaluation(ElementAt(foldable, Literal(1)), 20) + } + + // Non-primitive keys: the interpreted ordering would compare against null and the generated + // code would call `equals` on a value read from a null slot. + val stringKeyMap = mapWithNullKey( + Array(null, UTF8String.fromString("a")), + Array(UTF8String.fromString("x"), UTF8String.fromString("y"))) + val stringMapType = MapType(StringType, StringType) + withSQLConf(SQLConf.MAP_LOOKUP_HASH_THRESHOLD.key -> "0") { + val mapRef = BoundReference(0, stringMapType, nullable = true) + val row = create_row(stringKeyMap) + checkEvaluation(GetMapValue(mapRef, Literal("a")), "y", row) + checkEvaluation(GetMapValue(mapRef, Literal("missing")), null, row) + checkEvaluation(ElementAt(mapRef, Literal("a")), "y", row) + checkEvaluation(ElementAt(mapRef, Literal("missing")), null, row) + } + } + test("GetMapValue - strategy choice for foldable maps") { // Build a foldable map literal large enough to clear the default threshold. The // strategy assertions here pair with the non-foldable test above: together they lock in