srielau commented on code in PR #59325:
URL: https://github.com/apache/spark/pull/59325#discussion_r4240302115
##########
docs/sql-migration-guide.md:
##########
@@ -28,6 +28,7 @@ license: |
## Upgrading from Spark SQL 4.3 to 4.4
+- Since Spark 4.4, when `spark.sql.charVarchar.standardSemantics.enabled` is
true, JSON object names used as `MAP<CHAR(n), _>` or `MAP<VARCHAR(n), _>` keys
in `from_json` and the JSON datasource are length-checked without padding or
trimming. A `CHAR(n)` key must already be exactly `n` characters, and a
`VARCHAR(n)` key must already be at most `n` characters. A mismatched key turns
the row into a bad record: in `PERMISSIVE` mode it is dropped (the output is
`null`), while in `FAILFAST` mode parsing fails with
`UNSUPPORTED_JSON_CHAR_VARCHAR_MAP_KEY` (SQLSTATE `0A000`) as the cause of
`MALFORMED_RECORD_IN_PARSING`. Duplicate names are kept, exactly as for STRING
keys; `spark.sql.mapKeyDedupPolicy` is not applied.
Review Comment:
Good catch, and confirmed against the read-side code.
`CharVarcharUtils.processStringForCharVarchar` rebuilds a `MapType` with
`MapFromArrays` (applying `mapKeyDedupPolicy`) and pads CHAR keys via
`charTypeWriteSideCheck` / raises `EXCEED_LIMIT_LENGTH`, so the "no pad/trim,
keep-all, `mapKeyDedupPolicy` not applied" statements only hold for the parser
path (`from_json`, and reads with a user-specified CHAR/VARCHAR reader schema)
- not for reads whose keys are handled read-side (e.g. catalog tables).
I rescoped the migration-guide bullet to `from_json`, noted that the same
parser check runs for user-specified JSON reader schemas, and spelled out that
read-side-handled reads are unchanged (keys padded, `mapKeyDedupPolicy`
applied, over-long key -> `EXCEED_LIMIT_LENGTH`).
I also fixed the `PERMISSIVE` wording in the same bullet: it no longer says
the record is "dropped". It now says the enclosing map is set to `null` while
sibling fields are preserved, and the whole result is `null` only when the map
is the top-level type (matching the nested-struct and multiLine datasource
tests). The PR description got both fixes too.
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -3072,6 +3094,229 @@ class BasicCharVarcharTestSuite extends
SharedSparkSession {
}
}
+ test("SPARK-60108: JSON CHAR/VARCHAR map keys are not padded or trimmed") {
+ withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true") {
+ // Exact CHAR and within-limit VARCHAR names are accepted as-is: the
trailing space in
+ // "ab " is preserved for CHAR(3) and not trimmed for VARCHAR(3).
+ checkAnswer(
+ sql("""SELECT from_json('{"ab ": 1}', 'MAP<CHAR(3), INT>')"""),
+ Row(Map("ab " -> 1)))
+ checkAnswer(
+ sql("""SELECT from_json('{"ab": 1}', 'MAP<VARCHAR(3), INT>')"""),
+ Row(Map("ab" -> 1)))
+ // Binary-distinct names ("ab" vs "ab ") are both kept.
+ checkAnswer(
+ sql("""SELECT map_entries(from_json('{"ab": 1, "ab ": 2}',
'MAP<VARCHAR(3), INT>'))"""),
+ Row(Seq(Row("ab", 1), Row("ab ", 2))))
+ // Duplicate names are kept, exactly as for STRING keys;
mapKeyDedupPolicy is not applied.
+ // checkAnswer alone would collapse the duplicates when it builds a
Scala Map, so pin the
+ // behavior with size and map_entries.
+ checkAnswer(
+ sql("""SELECT size(from_json('{"abc": 1, "abc": 2}', 'MAP<CHAR(3),
INT>'))"""),
+ Row(2))
+ checkAnswer(
+ sql("""SELECT map_entries(from_json('{"abc": 1, "abc": 2}',
'MAP<CHAR(3), INT>'))"""),
+ Row(Seq(Row("abc", 1), Row("abc", 2))))
+ // Length counts characters (code points), not UTF-16 units or bytes:
one non-BMP code
+ // point is exactly CHAR(1) and is kept verbatim, and the same name is
rejected by CHAR(2).
+ // scalastyle:off nonascii (the \u escape is decoded before the nonascii
check runs)
+ val nonBmp = "\uD83D\uDE00" // U+1F600, one code point stored as a
UTF-16 surrogate pair
+ // scalastyle:on nonascii
+ checkAnswer(
+ sql(s"""SELECT map_keys(from_json('{"$nonBmp": 1}', 'MAP<CHAR(1),
INT>'))[0]"""),
+ Row(nonBmp))
+ checkAnswer(sql(s"""SELECT from_json('{"$nonBmp": 1}', 'MAP<CHAR(2),
INT>')"""), Row(null))
+
+ // Length-0 boundary: CHAR(0)/VARCHAR(0) accept only the empty-string
name, and any
+ // non-empty name is rejected (the empty key is still kept as a real map
entry).
+ checkAnswer(
+ sql("""SELECT from_json('{"": 1}', 'MAP<CHAR(0), INT>')"""),
+ Row(Map("" -> 1)))
+ checkAnswer(
+ sql("""SELECT from_json('{"": 1}', 'MAP<VARCHAR(0), INT>')"""),
+ Row(Map("" -> 1)))
+ checkAnswer(sql("""SELECT from_json('{"a": 1}', 'MAP<CHAR(0),
INT>')"""), Row(null))
+ checkAnswer(sql("""SELECT from_json('{"a": 1}', 'MAP<VARCHAR(0),
INT>')"""), Row(null))
+ assertUnsupportedJsonMapKey(
+ """SELECT from_json('{"a": 1}', 'MAP<CHAR(0), INT>', map('mode',
'FAILFAST'))""",
+ key = "a",
+ dataTypeSql = "CHAR(0)")
+ assertUnsupportedJsonMapKey(
+ """SELECT from_json('{"a": 1}', 'MAP<VARCHAR(0), INT>', map('mode',
'FAILFAST'))""",
+ key = "a",
+ dataTypeSql = "VARCHAR(0)")
+
+ // A key longer than the 128-character echo cap is truncated with a
trailing "..." in the
+ // error so a huge JSON object name does not bloat the message.
+ val longKey = "a" * 200
+ assertUnsupportedJsonMapKey(
+ s"""SELECT from_json('{"$longKey": 1}', 'MAP<CHAR(3), INT>',
map('mode', 'FAILFAST'))""",
+ key = "a" * 128 + "...",
+ dataTypeSql = "CHAR(3)")
+
+ // Valid keys inside nested containers.
+ checkAnswer(
+ sql("""SELECT from_json('{"outer": {"xy ": 1}}',
+ | 'MAP<STRING, MAP<CHAR(3), INT>>')""".stripMargin),
+ Row(Map("outer" -> Map("xy " -> 1))))
+ checkAnswer(
+ sql("""SELECT from_json('[{"ab ": 1}]', 'ARRAY<MAP<CHAR(3),
INT>>')"""),
+ Row(Seq(Map("ab " -> 1))))
+
+ // A rejected key makes the row a bad record: null in PERMISSIVE (not
EXCEED_LIMIT_LENGTH),
+ // and in FAILFAST the UNSUPPORTED_JSON_CHAR_VARCHAR_MAP_KEY cause is
surfaced.
+ checkAnswer(sql("""SELECT from_json('{"a": 1}', 'MAP<CHAR(3),
INT>')"""), Row(null))
+ checkAnswer(sql("""SELECT from_json('{"abcd": 1}', 'MAP<CHAR(3),
INT>')"""), Row(null))
+ checkAnswer(sql("""SELECT from_json('{"abcd": 1}', 'MAP<VARCHAR(3),
INT>')"""), Row(null))
+ checkAnswer(sql("""SELECT from_json('{"ab ": 1}', 'MAP<VARCHAR(2),
INT>')"""), Row(null))
+ assertUnsupportedJsonMapKey(
+ """SELECT from_json('{"a": 1}', 'MAP<CHAR(3), INT>', map('mode',
'FAILFAST'))""",
+ key = "a",
+ dataTypeSql = "CHAR(3)")
+ assertUnsupportedJsonMapKey(
+ """SELECT from_json('{"abcd": 1}', 'MAP<CHAR(3), INT>', map('mode',
'FAILFAST'))""",
+ key = "abcd",
+ dataTypeSql = "CHAR(3)")
+ assertUnsupportedJsonMapKey(
+ """SELECT from_json('{"abcd": 1}', 'MAP<VARCHAR(3), INT>', map('mode',
'FAILFAST'))""",
+ key = "abcd",
+ dataTypeSql = "VARCHAR(3)")
+ // A rejected key inside a nested container is surfaced too (not
swallowed).
+ assertUnsupportedJsonMapKey(
+ """SELECT from_json('{"outer": {"a": 1}}',
+ | 'MAP<STRING, MAP<CHAR(3), INT>>', map('mode',
'FAILFAST'))""".stripMargin,
+ key = "a",
+ dataTypeSql = "CHAR(3)")
+ assertUnsupportedJsonMapKey(
+ """SELECT from_json('[{"a": 1}]',
+ | 'ARRAY<MAP<CHAR(3), INT>>', map('mode',
'FAILFAST'))""".stripMargin,
+ key = "a",
+ dataTypeSql = "CHAR(3)")
+
+ // The DataFrame functions.from_json path constructs JsonToStructs the
same way and must
+ // behave identically: accepted key kept, rejected key null in
PERMISSIVE and surfaced in
+ // FAILFAST.
+ checkAnswer(
+ spark.range(1).select(
+ functions.from_json(
+ functions.lit("""{"ab ": 1}"""), "MAP<CHAR(3), INT>",
Map.empty[String, String])),
+ Row(Map("ab " -> 1)))
+ checkAnswer(
+ spark.range(1).select(
+ functions.from_json(
+ functions.lit("""{"a": 1}"""), "MAP<CHAR(3), INT>",
Map.empty[String, String])),
+ Row(null))
+ assertUnsupportedJsonMapKeyError(
+ spark.range(1).select(
+ functions.from_json(
+ functions.lit("""{"a": 1}"""),
+ "MAP<CHAR(3), INT>",
+ Map("mode" -> "FAILFAST"))).collect(),
+ key = "a",
+ dataTypeSql = "CHAR(3)")
+
+ // SPARK-60108: a rejected key must not corrupt a sibling field of an
enclosing struct.
+ // The map becomes null but the following `tail` field is still parsed,
and an inner name
+ // ("tail") is not mistaken for the outer field. This holds in both
partial-results modes.
+ Seq(true, false).foreach { partial =>
Review Comment:
Added. There is now a sub-block (run in both partial-results modes) with
rejected keys whose values are an object (`MAP<CHAR(3), STRUCT<x: INT>>`) and
an array (`MAP<CHAR(3), ARRAY<INT>>`): it asserts the map is nulled while the
sibling `tail` field survives, and that FAILFAST still surfaces the key. That
exercises the `parser.nextToken()` + `parser.skipChildren()` step over a nested
value, which the previous scalar-only cases did not cover.
--
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]
