cloud-fan commented on code in PR #55661:
URL: https://github.com/apache/spark/pull/55661#discussion_r3196656473
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala:
##########
@@ -122,6 +122,8 @@ class JacksonParser(
}
private val variantAllowDuplicateKeys =
SQLConf.get.getConf(SQLConf.VARIANT_ALLOW_DUPLICATE_KEYS)
+ private val variantvalidateUnicodeInJsonParsing =
Review Comment:
Naming typo — `variantvalidateUnicodeInJsonParsing` has a lowercase `v` in
the middle. The line directly above uses `variantAllowDuplicateKeys`; the new
one should follow the same camelCase convention.
```suggestion
private val variantValidateUnicodeInJsonParsing =
SQLConf.get.getConf(SQLConf.VARIANT_VALIDATE_UNICODE_IN_JSON_PARSING)
```
(And update the use on line 137 to match.)
##########
sql/core/src/test/scala/org/apache/spark/sql/VariantEndToEndSuite.scala:
##########
@@ -185,6 +185,28 @@ class VariantEndToEndSuite extends SharedSparkSession {
checkAnswer(variantDF, Seq(Row(expected)))
}
+ test("SPARK-56654: parse_json/from_json reject unpaired UTF-16 surrogates by
default") {
+ val invalidJson = "\"\\uD835\""
+ val df = Seq(invalidJson).toDF("j")
+ checkAnswer(df.selectExpr("try_parse_json(j)"), Seq(Row(null)))
+ checkAnswer(df.selectExpr("from_json(j, 'variant')"), Seq(Row(null)))
+
+ val parseJsonError = intercept[SparkException] {
+ df.selectExpr("parse_json(j)").collect()
+ }
+ val cause = Option(parseJsonError.getCause).getOrElse(parseJsonError)
+ assert(cause.getMessage.contains("MALFORMED_RECORD_IN_PARSING"),
+ s"Unexpected error message: ${cause.getMessage}")
Review Comment:
The same suite uses `checkError(...)` for typed assertions on this exact
error class (see line 414). `getMessage.contains(...)` is weaker (no parameter
check) and inconsistent with the rest of the file. Suggest:
```scala
val parseJsonError = intercept[SparkException] {
df.selectExpr("parse_json(j)").collect()
}
checkError(
exception = parseJsonError,
condition = "MALFORMED_RECORD_IN_PARSING.WITHOUT_SUGGESTION",
parameters = Map("badRecord" -> invalidJson, "failFastMode" -> "FAILFAST")
)
```
Also, the PR description claims `from_json` throws in FAILFAST mode, but
only the PERMISSIVE-returns-null path is tested — consider adding a
`from_json(j, 'variant', map('mode', 'FAILFAST'))` check using the template at
line 411.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -6050,6 +6050,19 @@ object SQLConf {
.booleanConf
.createWithDefault(false)
+ val VARIANT_VALIDATE_UNICODE_IN_JSON_PARSING =
+ buildConf("spark.sql.variant.validateUnicodeInJsonParsing")
+ .internal()
+ .doc("When true, parsing variant from JSON rejects strings that contain
unpaired UTF-16 " +
+ "surrogate code units (such as a lone high surrogate like \\uD835),
which are invalid " +
+ "per RFC 8259 section 7. When false, restores the legacy permissive
behavior in which " +
+ "the unpaired surrogate is silently replaced by the Unicode
replacement character " +
+ "during UTF-8 encoding, causing data corruption that diverges from
strict JSON parsers.")
+ .version("4.2.0")
Review Comment:
This PR will be merged to `branch-4.x`, so the conf version should be
`4.3.0`.
```suggestion
.version("4.3.0")
```
##########
common/variant/src/main/java/org/apache/spark/types/variant/VariantBuilder.java:
##########
@@ -557,6 +593,30 @@ private void parseFloatingPoint(JsonParser parser) throws
IOException {
}
}
+ // Reject JSON strings that contain unpaired UTF-16 surrogate code units.
Java strings can
+ // hold lone surrogates, but RFC 8259 section 7 requires JSON string
contents to be well-formed
+ // Unicode. Stricter parsers such as simdjson reject these inputs, while
Jackson's
+ // `ReaderBasedJsonParser` accepts them and silently drops the invalid
character to U+FFFD
Review Comment:
Minor wording: "silently drops the invalid character to U+FFFD" reads
awkwardly ("drops...to" mixes idioms). The Javadoc on the new `parseJson`
overload (line 69) already uses cleaner phrasing — let's match it:
```suggestion
// `ReaderBasedJsonParser` accepts them and silently replaces the invalid
character with
// U+FFFD when the result is encoded as UTF-8. That silent replacement
causes data
```
--
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]
