cloud-fan commented on code in PR #58545:
URL: https://github.com/apache/spark/pull/58545#discussion_r3962178699
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -310,27 +314,27 @@ class StaxXmlParser(
startElementName: String,
attributes: Array[Attribute]): Any = dt match {
case st: StructType => convertObject(parser, st)
- case MapType(StringType, vt, _) => convertMap(parser, vt, attributes)
+ case MapType(kt: StringType, vt, _) => convertMap(parser, kt, vt,
attributes)
case ArrayType(st, _) => convertField(parser, st, startElementName)
case VariantType =>
StaxXmlParser.convertVariant(parser, attributes, options)
- case _: StringType =>
+ case dt: StringType =>
convertTo(
StaxXmlParserUtils.currentStructureAsString(
parser, startElementName, options),
- StringType)
+ dt)
}
(parser.peek, dataType) match {
case (_: StartElement, dt: DataType) =>
convertComplicatedType(dt, startElementName, attributes)
- case (_: EndElement, _: StringType) =>
+ case (_: EndElement, dt: StringType) =>
StaxXmlParserUtils.skipNextEndElement(parser, startElementName,
options)
// Empty. It's null if "" is the null value
if (options.nullValue == "") {
null
} else {
- UTF8String.fromString("")
+ CharVarcharUtils.applyTextParseSemantics(UTF8String.fromString(""),
dt)
Review Comment:
**Non-blocking (P2):** Please add a focused `BasicCharVarcharTestSuite` case
for `from_xml('<ROW><a></a></ROW>', 'a CHAR(5)', map('nullValue', 'NULL'))` and
assert exactly five spaces. The existing character-content and overflow cases
do not exercise this empty-element branch or distinguish the required padded
value from null and `""`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -374,31 +378,41 @@ class StaxXmlParser(
*/
private def convertMap(
parser: XMLEventReader,
+ keyType: DataType,
valueType: DataType,
attributes: Array[Attribute]): MapData = {
val kvPairs = ArrayBuffer.empty[(UTF8String, Any)]
+ def mapKey(raw: String): UTF8String = {
+ CharVarcharUtils.applyTextParseSemantics(UTF8String.fromString(raw),
keyType)
+ }
attributes.foreach { attr =>
- kvPairs += (UTF8String.fromString(options.attributePrefix +
attr.getName.getLocalPart)
- -> convertTo(attr.getValue, valueType))
+ kvPairs += (mapKey(options.attributePrefix + attr.getName.getLocalPart)
->
+ convertTo(attr.getValue, valueType))
}
var shouldStop = false
while (!shouldStop) {
parser.nextEvent match {
case e: StartElement =>
val key = StaxXmlParserUtils.getName(e.asStartElement.getName,
options)
- kvPairs +=
- (UTF8String.fromString(key) -> convertField(parser, valueType, key))
+ kvPairs += (mapKey(key) -> convertField(parser, valueType, key))
Review Comment:
**Blocking (P1):** `mapKey(key)` can throw before `convertField` consumes
`<abc>`. In permissive mode, the enclosing `convertObject` then resumes on the
same reader and can treat `</abc>` as the end of the row, so
`from_xml('<ROW><m><abc>1</abc></m><tail>2</tail></ROW>', 'm MAP<CHAR(2), INT>,
tail INT')` loses `tail = 2`. Please consume or recover the entry value before
applying constrained-key semantics, and append the key/value pair atomically so
a rejected key leaves the reader positioned at the next sibling.
**Recommended change:** Consume or recover each XML map entry value before
applying CHAR/VARCHAR key normalization, then commit the normalized key and
converted value together.
**Why this works:** Moving key validation after value consumption preserves
the XMLEventReader position expected by convertObject's recoverable-error path
and prevents partially committed map entries.
**Scope:** StaxXmlParser.convertMap and focused XML partial-result tests for
an oversized constrained map key followed by a valid sibling field.
**Compatibility:** Successful maps, ordinary STRING-key handling, parse
modes, and duplicate-policy behavior should remain unchanged; only recovery
after an invalid constrained key is corrected.
**Risks:** Changing conversion order can alter which error wins when both a
key and its value are invalid. Incorrect event advancement could skip a nested
value or the following map entry.
**Constraints:** Invoke the value converter at most once per entry. Keep key
and value assembly atomic on every partial-result path. Preserve the configured
normalized-key duplicate policy.
**Success:** With partial results enabled, an oversized CHAR/VARCHAR XML map
key no longer hides a valid later row field, while existing successful and
duplicate-policy cases retain their behavior.
--
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]
