Skip to content

Commit c585f81

Browse files
committed
[SPARK-58133][SQL] XML schema inference must not skip empty values to avoid data loss
### What changes were proposed in this pull request? In `XmlInferSchema.inferFrom`, stop skipping empty values during schema inference. Previously the guard was: ```scala if (value == null || value.isEmpty || value == options.nullValue) { return typeSoFar } ``` This PR drops the `value.isEmpty` term: ```scala if (value == null || value == options.nullValue) { return typeSoFar } ``` so an empty value now falls through the type cascade to `StringType`. As a result, a field mixing empty and numeric values widens to `StringType` instead of being inferred as the numeric type. ### Why are the changes needed? XML schema inference was copied from `CSVInferSchema.inferField`, which does early-return on `field.isEmpty`. But the two datasources have a crucial difference at parse time: - CSV defaults `nullValue` to `""`, so an empty field is read as null by the parser (`UnivocityParser.nullSafeDatum`). Its inference can safely skip empty values because they never reach a numeric converter. - XML defaults `nullValue` to `null`, so an empty value is **not** read as null. If a column mixing empty and numeric values inferred `LongType`, `StaxXmlParser` would call `convertTo("", LongType)`, which throws `NumberFormatException`. In `PERMISSIVE` mode (the default), the malformed-record error recovery can then silently consume sibling XML events, **dropping records** — a silent data-loss bug. Concretely, for input ```xml <ROW><entry Code="001">first</entry><entry Code="002">second</entry></ROW> <ROW><entry Code="">third</entry><entry Code="003">fourth</entry></ROW> <ROW><entry Code="004">fifth</entry><entry Code="">sixth</entry></ROW> ``` `entry._Code` was inferred as `LongType` (from the numeric `Code` values, empty skipped), and reading then dropped records when it hit the empty `Code=""`. After this change `_Code` infers as `StringType` and all rows are read. By letting empty values fall through to `StringType`, inference stays consistent with how the XML parser actually reads the data. ### Does this PR introduce _any_ user-facing change? Yes. With the default `nullValue` (unset), a field that mixes empty and typed (e.g. numeric) values is now inferred as `StringType` instead of the typed type. This prevents the silent record-dropping described above. Fields that are entirely typed, or that use a non-default `nullValue` covering the empty token, are unaffected. Empty *elements* (`<c/>`) are still read as `NullType` by the parser and are unaffected. ### How was this patch tested? Added two tests to `XmlInferSchemaSuite`: - `SPARK-58133: empty values are not skipped during inference (no data loss)` — asserts that an attribute mixing empty and numeric values infers `StringType` and that all rows/entries survive (no PERMISSIVE-mode record dropping). - `SPARK-58133: empty and numeric element values still infer via NullType` — documents that empty elements keep inferring `LongType` (via the parser's `NullType` path), pinning the element/attribute asymmetry. Existing XML inference suites pass. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Anthropic Claude Opus) Closes #57265 from cloud-fan/cloud-fan/xml-infer-empty-dataloss. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
1 parent 6b719b3 commit c585f81

3 files changed

Lines changed: 57 additions & 6 deletions

File tree

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlInferSchema.scala

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -371,7 +371,17 @@ class XmlInferSchema(private val options: XmlOptions, private val caseSensitive:
371371
// such values as null (see `StaxXmlParser`), so inference must skip them too; otherwise a
372372
// column that is entirely `nullValue`s (or mixes them with a typed value) would infer the
373373
// string content of the token instead of ignoring it.
374-
if (value == null || value.isEmpty || value == options.nullValue) {
374+
//
375+
// Unlike `CSVInferSchema.inferField`, an empty value is intentionally NOT skipped here. The
376+
// CSV datasource defaults `nullValue` to `""`, so an empty field is already read as null by
377+
// the parser (`UnivocityParser.nullSafeDatum`) and its inference can safely skip it. XML's
378+
// `nullValue` defaults to `null`, so an empty value is NOT read as null: `StaxXmlParser`
379+
// would call `convertTo("", <numericType>)`, which throws `NumberFormatException`, and in
380+
// PERMISSIVE mode the error recovery can silently drop sibling records. To keep inference
381+
// consistent with the parser, an empty value falls through the cascade to `StringType`, so a
382+
// field mixing empty and numeric values widens to `StringType` rather than to the numeric
383+
// type. See SPARK-58133.
384+
if (value == null || value == options.nullValue) {
375385
return typeSoFar
376386
}
377387

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/xml/XmlInferSchemaTypeCastingSuite.scala

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,9 @@ class XmlInferSchemaTypeCastingSuite extends SparkFunSuite with SQLHelper {
3737
test("String field types are inferred correctly from null types") {
3838
val inferSchema = newInferSchema(Map("timestampFormat" -> "yyyy-MM-dd HH:mm:ss"))
3939

40-
assert(inferSchema.inferFrom("", NullType) == NullType)
40+
// An empty value carries no numeric/temporal content, so it falls through the cascade to
41+
// StringType (see `inferFrom`); only a genuinely null value preserves `typeSoFar`.
42+
assert(inferSchema.inferFrom("", NullType) == StringType)
4143
assert(inferSchema.inferFrom(null, NullType) == NullType)
4244
assert(inferSchema.inferFrom("100000000000", NullType) == LongType)
4345
// XML infers integral values as LongType (there is no IntegerType narrowing).
@@ -100,14 +102,17 @@ class XmlInferSchemaTypeCastingSuite extends SparkFunSuite with SQLHelper {
100102
assert(inferSchema.inferFrom("TRUEe", DoubleType) == StringType)
101103
}
102104

103-
test("Empty and null values keep the type inferred so far") {
105+
test("Null values keep the type inferred so far; empty values widen to String") {
104106
val inferSchema = newInferSchema(Map.empty[String, String])
105107

106-
// A genuinely empty/null value carries no type information, so `typeSoFar` is preserved.
107-
assert(inferSchema.inferFrom("", NullType) == NullType)
108-
assert(inferSchema.inferFrom("", LongType) == LongType)
108+
// A genuinely null value carries no type information, so `typeSoFar` is preserved.
109109
assert(inferSchema.inferFrom(null, DoubleType) == DoubleType)
110110
assert(inferSchema.inferFrom(null, TimestampType) == TimestampType)
111+
// Unlike CSV, XML inference does NOT skip empty values (see `inferFrom` and SPARK-58133): the
112+
// parser does not read "" as null under the default `nullValue`, so an empty value falls
113+
// through the cascade to StringType, widening any prior numeric/temporal type to String.
114+
assert(inferSchema.inferFrom("", NullType) == StringType)
115+
assert(inferSchema.inferFrom("", LongType) == StringType)
111116
}
112117

113118
test("A value matching the nullValue option keeps the type inferred so far") {

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/xml/XmlInferSchemaSuite.scala

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -652,6 +652,42 @@ class XmlInferSchemaSuite
652652
checkAnswer(xmlDF, expectedAns)
653653
}
654654

655+
test("SPARK-58133: empty values are not skipped during inference (no data loss)") {
656+
// An attribute that mixes empty ("") and numeric values across rows must infer as StringType,
657+
// not LongType. With the default nullValue (null), the parser does NOT read "" as null, so if
658+
// the column inferred LongType, convertTo("", LongType) would throw NumberFormatException and
659+
// PERMISSIVE-mode error recovery could silently drop sibling records. Inference therefore lets
660+
// an empty value fall through the cascade to StringType, keeping it consistent with the parser.
661+
val rows = Seq(
662+
"""<ROW><entry Code="001">first</entry><entry Code="002">second</entry></ROW>""",
663+
"""<ROW><entry Code="">third</entry><entry Code="003">fourth</entry></ROW>""",
664+
"""<ROW><entry Code="004">fifth</entry><entry Code="">sixth</entry></ROW>""")
665+
val df = readData(rows)
666+
val entryStruct = df.schema("entry").dataType match {
667+
case ArrayType(s: StructType, _) => s
668+
case s: StructType => s
669+
case other => fail(s"Expected ArrayType(StructType) or StructType for `entry`, got $other")
670+
}
671+
assert(entryStruct("_Code").dataType === StringType)
672+
// All three rows (six entries) must survive -- the empty attribute must not trigger the
673+
// PERMISSIVE error-recovery path that drops sibling records.
674+
assert(df.count() === 3)
675+
assert(df.selectExpr("sum(size(entry))").head().getLong(0) === 6)
676+
}
677+
678+
test("SPARK-58133: empty and numeric element values still infer via NullType") {
679+
// Empty *elements* (<c/>) are read as NullType by the parser's EndElement branch, so a column
680+
// mixing empty and numeric elements infers LongType and reads the empty element as null. This
681+
// documents the element/attribute asymmetry (only attribute empties fall through to String).
682+
val rows = Seq(
683+
"""<ROW><c>1</c></ROW>""",
684+
"""<ROW><c/></ROW>""",
685+
"""<ROW><c>3</c></ROW>""")
686+
val df = readData(rows)
687+
assert(df.schema("c").dataType === LongType)
688+
checkAnswer(df, Seq(Row(1L), Row(null), Row(3L)))
689+
}
690+
655691
test("TIME type inference") {
656692
val xmlString = Seq("""<ROW><t>13:31:24.123456</t></ROW>""")
657693
val df = readData(xmlString)

0 commit comments

Comments
 (0)