qingfureal opened a new pull request, #58875:
URL: https://github.com/apache/spark/pull/58875
### What changes were proposed in this pull request?
`GetMapValue` (`m[k]`) and `ElementAt` (`element_at(m, k)`) share their key
lookup in
`GetMapValueUtil`. This PR makes that lookup skip null entries in the map's
key array, on
every path it has:
- `LinearExecutor.eval` — the interpreted linear scan.
- `LinearExecutor.genCode` — the generated linear scan.
- `buildHashBuckets` — the driver-side bucket table backing the generated
hash probe.
- `buildHashIndex` — the driver-side index backing the interpreted hash
lookup (defensive;
a null `HashMap` key could not be matched anyway, but the two structures
should describe
the same set of keys).
The lookup key is never null — both expressions are null-safe on the ordinal
— so a null map
key can never be the key being looked up, and skipping it is the correct
semantics.
### Why are the changes needed?
A map whose key array contains a null currently returns that null key's
value for a lookup of
the key type's zero value (`0`, `0L`, `false`, ...). For a map `{null: 10,
1: 20}`:
```
m[0] ==> 10 -- should be NULL
m[1] ==> 20 -- correct
```
Two independent mechanisms produce this, so it reproduces with codegen both
on and off:
1. **Interpreted linear scan.** `ordering.equiv(keys.get(i, keyType),
ordinal)` compares using
the key type's natural ordering. `keys.get` is null-aware and correctly
returns `null` for a
null slot, but for a primitive key type the ordering unboxes its
arguments, and Scala's
`BoxesRunTime.unboxToInt(null)` is `0`. So `equiv(null, 0)` is `true`.
2. **Generated code.** The candidate key is read with a primitive getter
(`CodeGenerator.getValue` emits `keys.getInt(i)`), which is not
null-aware and returns `0`
for a null slot. This affects the linear scan, and also the hash probe
added by SPARK-55959:
`buildHashBuckets` hashes the unboxed null to the same bucket as `0`, so
the probe finds the
null key and then compares `0 == 0`.
Maps with null keys are reachable. `ArrayBasedMapBuilder` rejects them, but
the file-format
readers construct `ArrayBasedMapData` directly and do not.
`ParquetRowConverter` says so in the
tree today:
```scala
override def end(): Unit = {
// The parquet map may contains null or duplicated map keys. When it
happens, the behavior is
// undefined.
// TODO (SPARK-26174): disallow it with a config.
updater.set(
new ArrayBasedMapData(
new GenericArrayData(currentKeys.toArray),
new GenericArrayData(currentValues.toArray)))
}
```
`OrcDeserializer` and `AvroDeserializer` likewise build the key array
without a runtime null
check (each carries a comment asserting its format cannot produce one), and
the Hive
`MapObjectInspector` unwrapper in `HiveInspectors` applies the key unwrapper
with no null check
at all. So a null key can reach the lookup from Parquet, ORC, Avro or Hive
data.
"Undefined behavior" for such a map is one thing; silently returning a
wrong, non-null value
for an unrelated lookup key is another, and it is not detectable by the
user. Skipping null
keys makes the result well-defined and consistent across all four paths,
without changing
anything for maps that have no null keys.
Note on cost: the linear scan now performs one `isNullAt` per candidate key.
That is a bit test
for `UnsafeArrayData` and a reference compare for `GenericArrayData`. It
cannot be hoisted or
gated, because `MapType` has no `keyContainsNull` flag to gate on (unlike
`valueContainsNull`).
The hash paths take no new runtime cost — the filtering happens once, on the
driver, at
construction time.
### Does this PR introduce _any_ user-facing change?
Yes, for maps that contain a null key, which previously had undefined
behavior here.
Given a map `{null: 10, 1: 20}` read from Parquet/ORC/Avro/Hive, `m[0]` and
`element_at(m, 0)` returned `10`; they now return `NULL`. Lookups of keys
that are actually
present are unaffected (`m[1]` returns `20` before and after), as are all
maps without null
keys.
### How was this patch tested?
New test `map lookup must not match a null key` in `ComplexTypeSuite`,
covering both
`GetMapValue` and `ElementAt`, both executors (`LinearExecutor` via a
non-foldable map,
`PrebuiltHashExecutor` via a foldable one, with the strategy asserted rather
than assumed), and
both a primitive (`IntegerType`) and a non-primitive (`StringType`) key
type. It also asserts
that a non-null key in the same map still resolves, i.e. the scan skips the
null slot rather
than stopping at it. `checkEvaluation` exercises the interpreted, codegen
and unsafe-projection
paths.
The test fails on `master` and passes with this change. Each hunk was
confirmed to be
load-bearing by reverting it individually:
| Reverted hunk | Failure |
|---|---|
| all | `Incorrect evaluation (codegen off): input[0, map<int,int>,
true][0], actual: 10, expected: null` |
| `LinearExecutor.genCode` only | `Incorrect evaluation (fallback mode =
CODEGEN_ONLY): input[0, map<int,int>, true][0], actual: 10, expected: null` |
| `buildHashBuckets` only | `Incorrect evaluation (fallback mode =
CODEGEN_ONLY): map(keys: [null,1], values: [10,20])[0], actual: 10, expected:
null` |
Also ran `ComplexTypeSuite`, `CollectionExpressionsSuite`, `MapDataSuite`
and the optimizer's
`ComplexTypesSuite` (122 tests, 7 suites) plus `scalastyle` on
`sql/catalyst`: all pass.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)
--
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]
