HyukjinKwon commented on code in PR #59404: URL: https://github.com/apache/spark/pull/59404#discussion_r4273345884
########## sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlInferSchema.scala: ########## @@ -52,8 +52,9 @@ class XmlInferSchema(private val options: XmlOptions, private val caseSensitive: import org.apache.spark.sql.catalyst.xml.XmlInferSchema._ private val decimalParser = ExprUtils.getDecimalParser(options.locale) + private val legacyTimeParser = SQLConf.get.legacyTimeParserPolicy == LegacyBehaviorPolicy.LEGACY - private val timestampFormatter = TimestampFormatter( + @transient private lazy val timestampFormatter = TimestampFormatter( Review Comment: **Non-blocking (P2):** Making this a `lazy val` moves `TimestampFormatter`'s pattern validation out of the `XmlInferSchema` constructor and into the first `tryParseTimestamp` call, where per-record error handling hides it. This happens under every policy, including the default CORRECTED. For example, with `timestampFormat` set to `YYYY-MM-dd HH:mm:ss` (week-based year), inference used to fail right away with `INCONSISTENT_BEHAVIOR_CROSS_VERSION.DATETIME_PATTERN_RECOGNITION`, which tells the user to fix the pattern or switch to LEGACY. Now: - The `SparkUpgradeException` escapes `tryParseTimestamp`'s `IllegalArgumentException` catch and reaches the `NonFatal` handler in `infer()`. - In PERMISSIVE, every record with a string field infers as `_corrupt_record: string`. FAILFAST reports the generic malformed-records error instead. - Patterns rejected with a `SparkIllegalArgumentException`, such as an unsupported letter (`INVALID_DATETIME_PATTERN.ILLEGAL_CHARACTER`), are caught as "not a timestamp". The column then silently infers as `StringType` with no error. `schema_of_xml` behaves the same way. Keeping this a plain `private val`, as before this PR and as in `JsonInferSchema`/`CSVInferSchema`, restores the construction-time error. **Verification:** - **Regression:** XML schema inference with an invalid or legacy-only timestampFormat raises the pattern or upgrade error under CORRECTED, rather than returning a corrupt-record or StringType schema. - **Compatibility:** Existing LEGACY and CORRECTED XML timestamp inference expectations, including the LEGACY nanosecond-type error, still hold. ########## sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/xml/XmlLegacyTimestampSuite.scala: ########## @@ -0,0 +1,227 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql.execution.datasources.xml + +import java.sql.Timestamp +import java.time.{Instant, LocalDateTime, ZoneId} + +import scala.jdk.CollectionConverters._ + +import org.apache.spark.{SparkConf, SparkException} +import org.apache.spark.sql.{QueryTest, Row} +import org.apache.spark.sql.catalyst.util.DateTimeTestUtils.withDefaultTimeZone +import org.apache.spark.sql.catalyst.util.DateTimeUtils +import org.apache.spark.sql.functions.{from_xml, struct, to_xml} +import org.apache.spark.sql.internal.SQLConf +import org.apache.spark.sql.test.SharedSparkSession +import org.apache.spark.sql.types._ + +class XmlLegacyTimestampSuite extends QueryTest with SharedSparkSession { + import testImplicits._ + + override protected def sparkConf: SparkConf = super.sparkConf + .set(SQLConf.TIMESTAMP_NANOS_TYPES_ENABLED, false) + + private val schema = new StructType().add("stamp", TimestampType) + + test("SPARK-60199: LEGACY XML preserves spark-xml timestamps in files and inference") { + val timestamps = Seq( + "2024-02-29T12:34:56Z" -> "2024-02-29T12:34:56Z", + "2024-02-29T12:34:56.1Z" -> "2024-02-29T12:34:56.100Z", + "2024-02-29T12:34:56.123Z" -> "2024-02-29T12:34:56.123Z", + "2024-02-29T12:34:56.123456Z" -> "2024-02-29T12:34:56.123456Z", + "2024-02-29T12:34:56.000001Z" -> "2024-02-29T12:34:56.000001Z", + "2024-02-29T12:34:56.123456789Z" -> "2024-02-29T12:34:56.123456Z", + "2024-02-29T12:34:56.123456+02:00" -> "2024-02-29T10:34:56.123456Z", + "2024-02-29T12:34:56.123456" -> "2024-02-29T12:34:56.123456Z", + "2024-02-29 12:34:56.123456" -> "2024-02-29T12:34:56.123456Z", + "1500-01-01T12:34:56.123456Z" -> "1500-01-01T12:34:56.123456Z") + + withSQLConf( + SQLConf.LEGACY_TIME_PARSER_POLICY.key -> "LEGACY", + SQLConf.SESSION_LOCAL_TIMEZONE.key -> "America/Los_Angeles") { + Seq(false, true).foreach { multiLine => + withTempPath { path => + val rows = timestamps.map { case (input, _) => s"<ROW><stamp>$input</stamp></ROW>" } + val records = if (multiLine) Seq(rows.mkString("<ROWS>\n", "\n", "\n</ROWS>")) else rows + records.toDS().coalesce(1).write.text(path.getAbsolutePath) + for { + mode <- Seq("PERMISSIVE", "FAILFAST") + inferSchema <- Seq(false, true) + legacyXmlParser <- Seq(false, true) + } { + withClue( + s"$mode, infer=$inferSchema, multiLine=$multiLine, legacy=$legacyXmlParser") { + val reader = spark.read + .option("rowTag", "ROW") + .option("mode", mode) + .option("multiLine", multiLine) + .option("useLegacyXMLParser", legacyXmlParser) + if (!inferSchema) { + reader.schema(schema) + } + val result = reader.xml(path.getAbsolutePath) + assert(result.schema == schema) + checkAnswer( + result.selectExpr("unix_micros(stamp)"), + timestamps.map { case (_, expected) => + Row(DateTimeUtils.instantToMicros(Instant.parse(expected))) + }) + } + } + } + } + } + } + + test("SPARK-60199: LEGACY XML tries ISO timestamps before custom formats") { + val cases = Seq( + ("dd/MM/yyyy HH:mm:ss.SSSSSS", "29/02/2024 12:34:56.123456", "2024-02-29T20:34:56.123456Z"), + ( + "dd/MM/yyyy HH:mm:ss.SSSSSS XXX", + "29/02/2024 12:34:56.123456 +02:00", + "2024-02-29T10:34:56.123456Z"), + ( + "yyyy-MM-dd'T'HH:mm:ss.SSSXXX", + "2024-02-29T12:34:56.123456Z", + "2024-02-29T12:34:56.123456Z")) + withSQLConf(SQLConf.LEGACY_TIME_PARSER_POLICY.key -> "LEGACY") { + cases.foreach { case (format, input, expected) => + val xml = Seq(s"<ROW><stamp>$input</stamp></ROW>").toDS() + val reader = spark.read + .option("rowTag", "ROW") + .option("mode", "FAILFAST") + .option("timestampFormat", format) + .option("timeZone", "America/Los_Angeles") + checkAnswer(reader.xml(xml), Row(Timestamp.from(Instant.parse(expected)))) + checkAnswer(reader.schema(schema).xml(xml), Row(Timestamp.from(Instant.parse(expected)))) + } + } + } + + test("SPARK-60199: LEGACY XML rejects invalid ISO timestamps instead of normalizing them") { + withSQLConf(SQLConf.LEGACY_TIME_PARSER_POLICY.key -> "LEGACY") { + Seq("2024-02-30T12:34:56.123Z", "2024-02-29T25:34:56.123Z", "2024-02-29T12:34:56.123Zjunk") + .foreach { value => + val xml = Seq(s"<ROW><stamp>$value</stamp></ROW>").toDS() + val reader = spark.read.option("rowTag", "ROW").schema(schema) Review Comment: **Nit (P3):** This malformed-value test reads with an explicit schema, so it never runs inference. Two declared LEGACY branches have no assertion that would fail if they regressed: - **No-format inference guard.** Inference without `timestampFormat` must not fall back to the lenient legacy default pattern. If it did, `2024-02-30T12:34:56.123Z` or a trailing-text value would infer as TimestampType and then read as null. - **UTC fallback after a format miss.** A zone-less ISO value that does not match an explicit `timestampFormat`, for example `2024-02-29T12:34:56` under `dd/MM/yyyy HH:mm:ss`, should still read as UTC. None of the explicit-format cases uses such a value. Inference cannot cover this either, because `tryParseTimestampNTZ` claims zone-less values first under LEGACY. An inference case for the first and a mixed-format read for the second would cover both. **Verification:** - **Behavior:** LEGACY XML inference without timestampFormat reports STRING for impossible-date and trailing-text ISO-like values. - **Behavior:** LEGACY XML reads with a non-matching explicit timestampFormat return the UTC instant for a zone-less ISO value in PERMISSIVE and FAILFAST. ########## docs/sql-migration-guide.md: ########## @@ -35,6 +35,7 @@ license: | - Since Spark 4.4, when an already-analyzed Data Source V2 query is refreshed after a compatible schema change, connectors can return more data columns from the current table schema in `Scan.readSchema()` than requested by `SupportsPushDownRequiredColumns.pruneColumns`. Previously, this partial pruning could fail planning because the scan reported columns absent from the analyzed relation output. - Since Spark 4.4, when an already-analyzed Data Source V2 query is refreshed, if rebinding an expanded struct used as a map key causes distinct current keys to collide under the analyzed schema, the query fails with `DUPLICATED_MAP_KEY` by default instead of returning duplicate keys. With `spark.sql.mapKeyDedupPolicy=LAST_WIN`, the last value is kept. - Since Spark 4.4, the Oracle JDBC datasource maps bare `NUMBER` columns (declared without explicit precision or scale) to `DecimalType(38, 18)` instead of `DecimalType(38, 10)`. This uses `DecimalType.DEFAULT_SCALE` and preserves up to 18 fractional digits instead of 10, reducing silent precision loss. However, the integer range shrinks from 28 to 20 digits; bare `NUMBER` values with more than 20 integer digits that previously read correctly will raise `NUMERIC_VALUE_OUT_OF_RANGE`. The same change applies to Oracle `FLOAT` columns, which JDBC reports as `NUMERIC` with `scale = -127`. Columns with explicit `NUMBER(p,s)` are not affected. To restore the previous `DecimalType(38, 10)` mapping, set `spark.sql.legacy.oracle.numberMapping.enabled` to `true`. +- Since Spark 4.4, XML `TIMESTAMP` reads under `spark.sql.legacy.timeParserPolicy=LEGACY` preserve fractional seconds in ISO timestamps carrying an offset. Explicit `timestampFormat` values retain legacy pattern, locale, and time-zone handling; otherwise zone-less ISO timestamps are interpreted as UTC for spark-xml compatibility. `CORRECTED`, `EXCEPTION`, `TIMESTAMP_NTZ`, and nanosecond timestamp parsing are unchanged. Review Comment: **Nit (P3):** This entry says explicit `timestampFormat` values "retain legacy pattern, locale, and time-zone handling", and that zone-less ISO values are UTC "otherwise". Under LEGACY, `parseLegacyTimestamp` actually tries offset-bearing ISO values before the explicit format, and tries the UTC zone-less formatters after an explicit format that does not match. For upgrading LEGACY users, that means: - **Shifted instants.** With `timestampFormat` set to `yyyy-MM-dd'T'HH:mm:ss'Z'` and session zone `America/Los_Angeles`, `2024-02-29T12:34:56Z` used to read as `2024-02-29T20:34:56Z` and now reads as `2024-02-29T12:34:56Z`. - **Newly parsed values.** With `timestampFormat` set to `dd/MM/yyyy HH:mm:ss`, the same value used to be null, or StringType in inference, and now parses. - **Newly rejected values.** Without `timestampFormat`, values the old lenient parser normalized or partially consumed, such as `2024-02-30T12:34:56.123Z` or a value with trailing text, now become null or a FAILFAST error, and may infer as STRING. The option table in `sql-data-sources-xml.md` already describes the order correctly. Aligning this entry with it and listing the newly rejected inputs would fix this. **Verification:** - **Inspection:** The migration-guide entry describes the same precedence and UTC fallback as the timestampFormat option documentation and parseLegacyTimestamp, and names the newly rejected input classes. -- 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]
