cloud-fan commented on code in PR #58545:
URL: https://github.com/apache/spark/pull/58545#discussion_r3960161108
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/CharVarcharUtils.scala:
##########
@@ -156,6 +157,25 @@ object CharVarcharUtils extends Logging with
SparkCharVarcharUtils {
StructType(fields)
}
+ /**
+ * Applies CHAR padding and VARCHAR length checks when parsing text into a
typed schema.
+ * Null stays null. Unbounded STRING is unchanged. This is assignment
semantics
+ * (overflow raises EXCEED_LIMIT_LENGTH), not explicit CAST truncation.
+ */
+ def applyTextParseSemantics(value: UTF8String, dt: DataType): UTF8String = {
+ if (value == null) {
+ null
+ } else {
+ dt match {
+ case c: CharType =>
+ CharVarcharCodegenUtils.charTypeWriteSideCheck(value, c.length)
Review Comment:
**Non-blocking (P2):** Please add a non-space overflow case for this CHAR
branch on each new parser path. For example, parse `abcdef` into `CHAR(5)`
through JSON, CSV, and XML and assert a null field in `PERMISSIVE` mode plus
`EXCEED_LIMIT_LENGTH` in `FAILFAST`. The current CHAR cases are all under the
limit, while every overflow case uses VARCHAR, so they would not catch this
branch being accidentally wired to CAST-style truncation (`abcde`).
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala:
##########
@@ -617,16 +629,20 @@ class JacksonParser(
*/
private def convertMap(
parser: JsonParser,
- fieldConverter: ValueConverter): MapData = {
+ fieldConverter: ValueConverter,
+ keyType: DataType,
+ valueType: DataType): MapData = {
val keys = ArrayBuffer.empty[UTF8String]
val values = ArrayBuffer.empty[Any]
var badRecordException: Option[Throwable] = None
while (nextUntil(parser, JsonToken.END_OBJECT)) {
- keys += UTF8String.fromString(parser.currentName)
+ keys += CharVarcharUtils.applyTextParseSemantics(
Review Comment:
**Blocking (P1):** Please consume or recover the value before committing its
normalized key, and append the key/value pair atomically. As written, an
over-limit key throws while Jackson is still on `FIELD_NAME`, so the outer
partial-result recovery can stop at the nested map's `END_OBJECT` and lose
later sibling fields. A value failure with no partial result also leaves `keys`
longer than `values`, so the builder reports unequal array lengths before a
later normalized collision can raise `DUPLICATED_MAP_KEY`. For example, cover
`{'m':{'abc':1},'tail':2}` with `m MAP<CHAR(2), INT>` and `{'a':'bad','a ':2}`
with `MAP<CHAR(2), INT>` under partial results.
**Recommended change:** Make JSON map-entry conversion atomic: consume the
value first, then normalize and append the key together with a successful or
partial value; append neither side when no partial value exists.
**Why this works:** Capture the raw field name, advance through
fieldConverter and partial-result recovery, and only after the value token is
consumed apply CHAR/VARCHAR key semantics and commit both arrays. A key
validation error will then propagate with the parser positioned after that
entry, and ArrayBasedMapBuilder will always receive arrays of equal length.
**Scope:** JacksonParser.convertMap plus focused JSON partial-result tests
for an invalid constrained key followed by a sibling field and for a malformed
value followed by a normalized duplicate key.
**Compatibility:** Keep successful JSON maps, ordinary STRING-key maps,
parse modes, partial-result behavior, and mapKeyDedupPolicy outcomes unchanged
except for restoring later-field recovery and the intended duplicate-key error
precedence.
**Risks:** Changing key/value validation order can change which error is
surfaced when both parts of one entry are invalid. Incorrect token advancement
could skip a nested value or consume the next map entry.
**Constraints:** Do not invoke fieldConverter more than once per entry. Keep
keys and values the same length on every recovery path. Preserve
DuplicateMapKeyException propagation and the first recorded bad-record cause.
**Success:** With partial results enabled, a bad constrained map key does
not discard a valid later struct field, and a malformed value followed by a
normalized duplicate reaches the configured duplicate-key policy rather than
failing the array-length precondition.
--
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]
