cloud-fan commented on code in PR #56919:
URL: https://github.com/apache/spark/pull/56919#discussion_r3510583454
##########
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:
Good catch, and confirmed empirically -- you're right. Fixed symmetrically
with the numeric case: all temporal `typeSoFar`
(`Time`/`Date`/`NTZ`/`Timestamp`) now re-enter at the top of the temporal
sub-cascade (`tryParseTime`, flowing Time -> Date -> TimestampNTZ ->
Timestamp), which is exactly the tail from-scratch inference reaches via
`tryParseDouble`. So a date-only value infers as `Date` rather than falling
through to `String`, and `findWiderDateTimeType` widens `Date` +
`Timestamp`/`NTZ` identically on both paths. Skipping the numeric parsers on
this branch stays safe (numeric<->temporal merges to `String` either way).
Also added:
- a direct unit assertion `inferFrom("2024-01-15", TimestampType) ==
TimestampType` (under `timestampFormat = "yyyy-MM-dd'T'HH:mm:ss"`) in
`XmlInferSchemaTypeCastingSuite` -- `String` before the fix, `Timestamp` after
-- plus the NTZ case (`inferFrom("2024-01-15", TimestampNTZType) ==
TimestampNTZType`, since `DATE` adopts the other side's family);
- the reverse-order temporal case (timestamp-first and date-first, both with
an explicit time-requiring `timestampFormat`) to the single-partition
equivalence test.
Pushed in 27a071c951e.
--
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]
