srielau commented on code in PR #59325:
URL: https://github.com/apache/spark/pull/59325#discussion_r4238472699
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala:
##########
@@ -619,25 +622,59 @@ class JacksonParser(
/**
* Parse an object as a Map, preserving all fields.
+ *
+ * JSON object names used as CHAR/VARCHAR keys are length-checked without
rewriting:
+ * CHAR keys must already be exactly n characters, and VARCHAR keys must
already be
+ * at most n characters. Padding, trimming, and mapKeyDedupPolicy are not
applied.
+ * Repeated names that are binary-equal last-win for CHAR/VARCHAR keys
(collation is
+ * not consulted, matching how STRING keys are compared here); STRING keys
keep every
+ * pair. This differs from XML, which pads keys and then applies
mapKeyDedupPolicy.
*/
private def convertMap(
parser: JsonParser,
- fieldConverter: ValueConverter): MapData = {
+ fieldConverter: ValueConverter,
+ keyType: DataType): MapData = {
val keys = ArrayBuffer.empty[UTF8String]
val values = ArrayBuffer.empty[Any]
var badRecordException: Option[Throwable] = None
+ // CHAR/VARCHAR keys last-win on exact (binary) duplicate names. This maps
a key to
+ // the index of its value in `values` for O(1) overwrite; it stays null
(and the
+ // buffers keep every pair, as STRING maps always have) when last-win does
not apply.
+ // It indexes `values` (not `keys`) so it stays valid even after the
NonFatal arm
+ // appends a dangling key, leaving the two buffers unbalanced.
+ val lastWinIndex: mutable.HashMap[UTF8String, Int] =
+ if (isLengthCheckedKeyType(keyType)) mutable.HashMap.empty[UTF8String,
Int] else null
while (nextUntil(parser, JsonToken.END_OBJECT)) {
- keys += UTF8String.fromString(parser.currentName)
+ val key = convertJsonMapKey(parser.currentName, keyType)
Review Comment:
Good catch. The key check now runs inside the per-entry loop and is handled
exactly like a failing value converter: on rejection we advance past the value
and skip its children, leaving the key unpaired. The loop still drains to
`END_OBJECT`, so siblings parse normally and streaming row counts are
unaffected. The unbalanced key/value buffers then trigger the existing
`keys.length != values.length` rethrow of the real cause, independent of
`enablePartialResults`. Added tests for a bad key followed by a sibling (both
partial-result modes) and for the multiLine top-level-array row count.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala:
##########
@@ -107,6 +107,9 @@ class JacksonParser(
private val enablePartialResults = SQLConf.get.jsonEnablePartialResults
+ // CHAR/VARCHAR JSON map keys are length-checked without pad or trim under
this flag.
+ private val charVarcharStandardSemantics =
SQLConf.get.charVarcharStandardSemantics
Review Comment:
Fixed. The flag is now captured at analysis time in `JsonToStructs` (default
arg off `SQLConf.get`), threaded through `JsonToStructsEvaluator`, and passed
into `JacksonParser` via a new `charVarcharStandardSemanticsOverride`
constructor arg. The file-based datasource path leaves the override as `None`
and keeps reading the live conf, which is correct there. A view created with
the flag on now keeps standard semantics when queried with it off (test added).
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala:
##########
@@ -619,25 +622,59 @@ class JacksonParser(
/**
* Parse an object as a Map, preserving all fields.
+ *
+ * JSON object names used as CHAR/VARCHAR keys are length-checked without
rewriting:
+ * CHAR keys must already be exactly n characters, and VARCHAR keys must
already be
+ * at most n characters. Padding, trimming, and mapKeyDedupPolicy are not
applied.
+ * Repeated names that are binary-equal last-win for CHAR/VARCHAR keys
(collation is
+ * not consulted, matching how STRING keys are compared here); STRING keys
keep every
+ * pair. This differs from XML, which pads keys and then applies
mapKeyDedupPolicy.
Review Comment:
Agreed - switched to keep-all, identical to STRING keys. Dropped the
last-win dedup and the HashMap entirely; `spark.sql.mapKeyDedupPolicy` is not
consulted. Updated the scaladoc and migration guide, and added tests that pin
duplicate retention via `size` and `map_entries`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala:
##########
@@ -619,25 +622,59 @@ class JacksonParser(
/**
* Parse an object as a Map, preserving all fields.
+ *
+ * JSON object names used as CHAR/VARCHAR keys are length-checked without
rewriting:
+ * CHAR keys must already be exactly n characters, and VARCHAR keys must
already be
+ * at most n characters. Padding, trimming, and mapKeyDedupPolicy are not
applied.
+ * Repeated names that are binary-equal last-win for CHAR/VARCHAR keys
(collation is
+ * not consulted, matching how STRING keys are compared here); STRING keys
keep every
+ * pair. This differs from XML, which pads keys and then applies
mapKeyDedupPolicy.
*/
private def convertMap(
parser: JsonParser,
- fieldConverter: ValueConverter): MapData = {
+ fieldConverter: ValueConverter,
+ keyType: DataType): MapData = {
val keys = ArrayBuffer.empty[UTF8String]
val values = ArrayBuffer.empty[Any]
var badRecordException: Option[Throwable] = None
+ // CHAR/VARCHAR keys last-win on exact (binary) duplicate names. This maps
a key to
+ // the index of its value in `values` for O(1) overwrite; it stays null
(and the
+ // buffers keep every pair, as STRING maps always have) when last-win does
not apply.
+ // It indexes `values` (not `keys`) so it stays valid even after the
NonFatal arm
+ // appends a dangling key, leaving the two buffers unbalanced.
+ val lastWinIndex: mutable.HashMap[UTF8String, Int] =
+ if (isLengthCheckedKeyType(keyType)) mutable.HashMap.empty[UTF8String,
Int] else null
Review Comment:
Moot now - the HashMap is gone with the switch to keep-all (see above), so
there is no longer any key-hashing of untrusted input.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala:
##########
@@ -659,6 +696,33 @@ class JacksonParser(
}
}
+ /**
+ * A CHAR/VARCHAR map key under standard semantics is length-checked (not
padded or
+ * trimmed) and deduplicated last-win on exact names. This is the single
predicate both
+ * `convertMap` and `convertJsonMapKey` consult so the two cannot drift
apart.
+ */
+ private def isLengthCheckedKeyType(keyType: DataType): Boolean =
+ charVarcharStandardSemantics && (keyType match {
+ case _: CharType | _: VarcharType => true
+ case _ => false
+ })
+
+ private def convertJsonMapKey(rawName: String, keyType: DataType):
UTF8String = {
+ val key = UTF8String.fromString(rawName)
+ if (!isLengthCheckedKeyType(keyType)) {
+ key
+ } else {
+ keyType match {
+ case c: CharType =>
+ CharVarcharCodegenUtils.charTypeJsonMapKeyCheck(key, c.length)
+ case v: VarcharType =>
+ CharVarcharCodegenUtils.varcharTypeJsonMapKeyCheck(key, v.length)
+ case _ =>
+ key
Review Comment:
Fixed. The key checker is now built once up front via `makeMapKeyChecker`
(returns `Option[UTF8String => Unit]`), so there is no per-key re-matching and
the unreachable branch is gone.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/util/CharVarcharCodegenUtils.java:
##########
@@ -111,4 +113,28 @@ public static UTF8String charTypeReadSideCheck(UTF8String
inputStr, int limit) {
public static UTF8String varcharTypeReadSideCheck(UTF8String inputStr, int
limit) {
return varcharTypeWriteSideCheck(inputStr, limit);
}
+
+ /**
+ * JSON object names used as MAP<CHAR(n), _> keys cannot be padded or
trimmed.
+ * The name must already be exactly n characters.
+ */
+ public static UTF8String charTypeJsonMapKeyCheck(UTF8String inputStr, int
limit) {
+ if (inputStr.numChars() != limit) {
+ throw QueryExecutionErrors.unsupportedJsonCharVarcharMapKey(
+ inputStr, CharType.apply(limit));
Review Comment:
Fixed. The helpers moved out of `CharVarcharCodegenUtils` into
`JacksonParser` and now receive the actual `CharType`/`VarcharType` key type,
so collation is preserved. Added a test asserting the error message shows
`VARCHAR(2) COLLATE UTF8_LCASE` and that collation is not consulted for the
length check.
--
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]
