MaxGekk commented on code in PR #56919:
URL: https://github.com/apache/spark/pull/56919#discussion_r3508983824
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlInferSchema.scala:
##########
@@ -312,41 +338,72 @@ class XmlInferSchema(private val options: XmlOptions,
private val caseSensitive:
}
private def inferFrom(datum: String): DataType = {
+ // Start type inference from scratch, without any prior knowledge of the
field's type.
+ inferFrom(datum, NullType)
+ }
+
+ /**
+ * Infer the type of a primitive value, refining a type already inferred for
the same field
+ * from previous values.
+ *
+ * Rather than probing every candidate type from scratch on every value (the
historical
+ * approach), this dispatches on `typeSoFar` -- the type accumulated for
this field so far --
+ * and only attempts to widen it. Given a value already known to be
`Double`, for example,
+ * there is no point checking whether it is a `Long`, as the merged type can
only be `Double`
+ * or wider. This mirrors the schema inference used by the CSV datasource
+ * (`CSVInferSchema.inferField`) and keeps the type lattice monotonic: each
value can only
+ * hold the type where it is or widen it, never narrow it. `compatibleType`
reconciles the
+ * newly inferred type with `typeSoFar`, falling back to `StringType` (the
top type) when they
+ * are incompatible.
+ */
+ private[xml] def inferFrom(datum: String, typeSoFar: DataType): DataType = {
val value = if (datum != null && options.ignoreSurroundingSpaces) {
datum.trim()
} else {
datum
}
- if (options.inferSchema) {
- lazy val decimalTry = tryParseDecimal(value)
- lazy val timestampNTZTry = tryParseTimestampNTZ(value)
- value match {
- case null => NullType
- case v if v.isEmpty => NullType
- case v if isLong(v) => LongType
- case v if options.prefersDecimal && decimalTry.isDefined =>
decimalTry.get
- case v if isDouble(v) => DoubleType
- case v if isBoolean(v) => BooleanType
- case v if isTimeTypeEnabled && isTime(v) =>
TimeType(TimeType.DEFAULT_PRECISION)
- case v if isDate(v) => DateType
- case v if timestampNTZTry.isDefined => timestampNTZTry.get
- case v if isTimestamp(v) => TimestampType
- case _ => StringType
- }
- } else {
- StringType
- }
+ if (!options.inferSchema) {
+ return StringType
+ }
+ if (value == null || value.isEmpty) {
+ return typeSoFar
+ }
+
+ val inferredType = typeSoFar match {
+ // For a numeric type inferred so far, re-enter the cascade at its top
(`tryParseLong`)
+ // rather than at the narrower numeric parser. Entering at
`tryParseDecimal` would infer an
+ // integer value directly as a narrow `Decimal` (e.g. "5" ->
Decimal(1,0)) instead of the
+ // `Long` that from-scratch inference produces; `compatibleType` then
merges those to
+ // different Decimal precisions (Decimal(5,2) vs Decimal(22,2)), which
would make the
+ // inferred type differ from the legacy path and depend on row order.
Re-entering at the top
+ // yields the same representative as from-scratch inference, so the
merged type matches.
+ case NullType | LongType | DoubleType | _: DecimalType =>
tryParseLong(value)
+ case BooleanType => tryParseBoolean(value)
+ case _: TimeType => tryParseTime(value)
+ case DateType => tryParseDate(value)
+ case TimestampNTZType | _: TimestampNTZNanosType =>
tryParseTimestampNTZ(value)
+ case TimestampType => tryParseTimestamp(value)
Review Comment:
The numeric fix rerouted numeric `typeSoFar` to the cascade top
(`tryParseLong`), but the temporal arms still enter mid-cascade:
`TimestampType` → `tryParseTimestamp`, `TimestampNTZType` →
`tryParseTimestampNTZ` (line 385), both skipping `tryParseDate`. That
reintroduces the same order-dependent divergence in the temporal family.
Field values `<timestamp>` then a date-only value, with `timestampFormat =
"yyyy-MM-dd'T'HH:mm:ss"`:
- **Batch (flag off):** date-only → `DateType` (the suite's own
`inferFrom("2024-01-15", NullType) == DateType`), and
`compatibleType(TimestampType, DateType) = TimestampType` —
`findWiderDateTimeType` widens `Date` into the timestamp family.
- **Incremental (flag on):** `tryParseTimestamp("2024-01-15")` can't parse a
bare date → falls through to `String`, and `compatibleType(TimestampType,
StringType) = StringType` (a `String` isn't a `DatetimeType`, so it hits the
`(_, _) => StringType` fallback, not the date-widening path).
So the field infers `String` incrementally vs `Timestamp` in batch —
order-dependent, and under the default-on flag it's a silent change to the
inferred schema, contradicting the "no change to inferred schemas" invariant.
The "cross-family values merge to String either way" reasoning holds for
Boolean and numeric↔temporal, but not *within* the temporal family, where
`Date` and `Timestamp`/`NTZ` widen together. (CSV — the cited analogue — needs
an explicit `canParseDateAsTimestamp` in its `compatibleType`,
`CSVInferSchema.scala:276-278`, for exactly this; XML ports the cascade but not
that reconciliation.)
Fix, symmetric with the numeric one: route temporal `typeSoFar`
(Time/Date/NTZ/Timestamp) to the top of the temporal sub-cascade —
`tryParseTime` (or at minimum NTZ/Timestamp → `tryParseDate`) — so a date-only
value infers as `Date` exactly as from-scratch and then widens identically. I
checked this keeps the existing NTZ→Timestamp widening test passing. Worth
adding the reverse-order temporal case (timestamp/NTZ first, then a date-only
value, with an explicit `timestampFormat`) to the single-partition equivalence
test — it currently covers only date-then-timestamp (the safe direction),
whereas the decimal case tests both orders.
--
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]
