cloud-fan commented on code in PR #58875:
URL: https://github.com/apache/spark/pull/58875#discussion_r4069607550
##########
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:
**Nit (P3):** A non-null key can share hash 0 with a null HashMap key; it is
the equality check, not the absence of a hash collision, that prevents a match.
Could this rationale state that distinction?
##########
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:
**Nit (P3):** ArrayBasedMapData explicitly leaves duplicate-key behavior
undefined, so this comment creates a contract the representation does not
provide. Please attribute first-match behavior only to the linear and prebuilt
lookup implementations.
##########
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:
**Nit (P3):** The corrected lookup returns NULL only when no non-null key
matches. If a real matching key follows the null slot, it now returns that
entry's value, as the added test demonstrates; could the migration note
distinguish those two outcomes?
--
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]
