cloud-fan commented on code in PR #58545:
URL: https://github.com/apache/spark/pull/58545#discussion_r3944096858
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala:
##########
@@ -636,9 +640,8 @@ class JacksonParser(
}
}
- // The JSON map will never have null or duplicated map keys, it's safe to
create a
- // ArrayBasedMapData directly here.
- val mapData = ArrayBasedMapData(keys.toArray, values.toArray)
+ val mapData = new ArrayBasedMapBuilder(keyType, valueType).from(
Review Comment:
**Blocking (P1):** The new top-level bypass only works while
`DUPLICATED_MAP_KEY` remains the root cause. Here an outer `convertMap` can
catch an inner duplicate as generic `NonFatal`, omit its value, and then
replace it with the key/value-length error from `from`; `convertObject` can
likewise retain an earlier field error and discard a later duplicate. In both
cases PERMISSIVE mode can bypass `mapKeyDedupPolicy=EXCEPTION`. Could we
rethrow or prioritize this condition in the inner partial-result paths and
cover both a map nested as a map value and a duplicate after an unrelated bad
struct field?
**Recommended change:** Make DUPLICATED_MAP_KEY terminal at every
JacksonParser partial-result boundary while leaving other conversion failures
under the existing permissive partial-result policy.
**Why this works:** Detect the duplicate-key condition before generic
NonFatal handling in convertMap and before first-error accumulation can hide it
in convertObject, using one consistent condition/cause check across the parser.
**Scope:** JacksonParser convertMap and convertObject error handling plus
BasicCharVarcharTestSuite regressions for nested-map and earlier-bad-field
ordering cases.
**Compatibility:** EXCEPTION reliably fails on normalized duplicates
regardless of nesting or field order; LAST_WIN and partial results for
non-duplicate conversion errors remain unchanged.
**Risks:** An overly broad cause-chain rethrow could disable permissive
handling for unrelated SparkRuntimeException conditions. Changing accumulator
precedence must not discard valid partial results for errors other than
duplicate map keys.
**Constraints:** Match only the established DUPLICATED_MAP_KEY condition.
Preserve JSON partial-result behavior and parse-mode handling for all other
failures.
**Success:** A CHAR-key collision nested under another map and the same
collision after an unrelated malformed struct field both raise
DUPLICATED_MAP_KEY under EXCEPTION, without changing non-duplicate permissive
results.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala:
##########
@@ -636,9 +640,8 @@ class JacksonParser(
}
}
- // The JSON map will never have null or duplicated map keys, it's safe to
create a
- // ArrayBasedMapData directly here.
- val mapData = ArrayBasedMapData(keys.toArray, values.toArray)
+ val mapData = new ArrayBasedMapBuilder(keyType, valueType).from(
Review Comment:
**Blocking (P1):** `ArrayBasedMapBuilder` is now used for every
`StringType`-keyed map, not only CHAR/VARCHAR keys. That changes existing
default-mode behavior: duplicate JSON object names (and repeated XML map
element names) now raise `DUPLICATED_MAP_KEY`, while the old JSON path
constructed `MapData` directly and the old XML path used last-wins `toMap`.
This is reachable with the standard-semantics flag still off, e.g.
`from_json('{"a":1,"a":2}', 'MAP<STRING, INT>')`. Could we preserve the old
construction path for ordinary STRING keys and select policy-aware construction
only where CHAR/VARCHAR normalization can introduce collisions?
**Recommended change:** Keep the previous JSON/XML map construction behavior
for every non-CHAR/VARCHAR string key type, and use ArrayBasedMapBuilder plus
duplicate-key escape handling only for CHAR/VARCHAR keys whose new
normalization can create collisions.
**Why this works:** Classify the declared map key type at the parser
conversion boundary instead of treating every StringType subtype as part of the
new deduplication path; gate the corresponding duplicate-error bypass by the
same classification.
**Scope:** JacksonParser.convertMap, StaxXmlParser.convertMap, their
duplicate-error propagation hooks, and compatibility tests for ordinary STRING
maps alongside the CHAR/VARCHAR collision tests.
**Compatibility:** Ordinary MAP<STRING, ...> inputs retain their pre-PR
duplicate behavior with standard semantics both disabled and enabled, while
normalized CHAR/VARCHAR collisions continue to honor EXCEPTION and LAST_WIN.
**Risks:** The type gate must distinguish first-class CHAR/VARCHAR keys
without changing behavior for other StringType variants or collations. The JSON
and XML construction branches must remain aligned with their respective pre-PR
duplicate semantics.
**Constraints:** Do not broaden spark.sql.mapKeyDedupPolicy beyond the
normalized CHAR/VARCHAR collision path introduced by this PR. Preserve existing
parse-mode and partial-result behavior for ordinary STRING maps.
**Success:** The ordinary JSON and XML duplicate examples return their
pre-PR last value under default settings, and CHAR/VARCHAR normalization
collisions still throw under EXCEPTION and retain the last value under LAST_WIN.
--
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]
