HyukjinKwon commented on code in PR #59404:
URL: https://github.com/apache/spark/pull/59404#discussion_r4269543975
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -1172,6 +1189,48 @@ class XmlTokenizer(
}
object StaxXmlParser {
+ private val legacyTimestampFormatters = Seq(
+ new DateTimeFormatterBuilder()
+ .parseCaseInsensitive()
+ .append(DateTimeFormatter.ISO_LOCAL_DATE)
+ .appendLiteral(' ')
+ .append(DateTimeFormatter.ISO_LOCAL_TIME)
+ .toFormatter()
+ .withZone(ZoneOffset.UTC),
+ DateTimeFormatter.ISO_LOCAL_DATE_TIME.withZone(ZoneOffset.UTC),
+ DateTimeFormatter.ISO_OFFSET_DATE_TIME,
+ DateTimeFormatter.ISO_INSTANT)
+
+ private[xml] def customLegacyTimestampFormatter(
+ options: XmlOptions): Option[DateTimeFormatter] = {
+ options.parameters.get(XmlOptions.TIMESTAMP_FORMAT).map { pattern =>
+ val format = DateTimeFormatter.ofPattern(pattern)
+ if (pattern.exists("VzOXxZ".contains(_))) {
+ format
+ } else {
+ format.withZone(options.timezone.map(ZoneId.of).orNull)
Review Comment:
**Blocking (P1):** `options.timezone` is the raw `timeZone` option, so a
zone-less custom `timestampFormat` gets `withZone(null)` unless the user sets
`timeZone` explicitly. `Instant.from` then always fails, and the `Try` turns
that into `None`.
Example under LEGACY: `.option("timestampFormat", "dd/MM/yyyy HH:mm:ss")`
with `29/02/2024 12:34:56` and no `timeZone`. Before this PR the value was
parsed in the session time zone (`options.zoneId`). Now it gives null in
PERMISSIVE, `_LEGACY_ERROR_TEMP_3260` in FAILFAST, and StringType in inference.
`ZoneId.of` also rejects IDs that `DateTimeUtils.getZoneId`, which
`options.zoneId` uses, accepts, such as `PST` or `+8:00`. The resulting
`ZoneRulesException` or `DateTimeException` is thrown outside the `Try` and is
not an `IllegalArgumentException`. During inference, a non-digit string field
skips the ISO formatters and forces this lazy formatter, so every record
containing one becomes `_corrupt_record` in PERMISSIVE, or the read fails in
FAILFAST.
The `VzOXxZ` check also matches quoted literals such as `'Z'`, which leaves
those patterns without a zone. None of the new tests use a custom format
without an explicit region `timeZone`.
See **Shared repair plan 1** in the review body.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -1172,6 +1189,48 @@ class XmlTokenizer(
}
object StaxXmlParser {
+ private val legacyTimestampFormatters = Seq(
+ new DateTimeFormatterBuilder()
+ .parseCaseInsensitive()
+ .append(DateTimeFormatter.ISO_LOCAL_DATE)
+ .appendLiteral(' ')
+ .append(DateTimeFormatter.ISO_LOCAL_TIME)
+ .toFormatter()
+ .withZone(ZoneOffset.UTC),
+ DateTimeFormatter.ISO_LOCAL_DATE_TIME.withZone(ZoneOffset.UTC),
+ DateTimeFormatter.ISO_OFFSET_DATE_TIME,
+ DateTimeFormatter.ISO_INSTANT)
+
+ private[xml] def customLegacyTimestampFormatter(
+ options: XmlOptions): Option[DateTimeFormatter] = {
+ options.parameters.get(XmlOptions.TIMESTAMP_FORMAT).map { pattern =>
+ val format = DateTimeFormatter.ofPattern(pattern)
+ if (pattern.exists("VzOXxZ".contains(_))) {
+ format
+ } else {
+ format.withZone(options.timezone.map(ZoneId.of).orNull)
+ }
+ }
+ }
+
+ private[xml] def parseLegacyTimestamp(
+ value: String,
+ customFormatter: => Option[DateTimeFormatter]): Option[Long] = {
+ def parseWithFormatter(format: DateTimeFormatter): Option[Long] = {
+ // Preserve spark-xml's java.sql.Timestamp conversion, including
historical calendar rebasing.
+ Try(
+
DateTimeUtils.fromJavaTimestamp(Timestamp.from(Instant.from(format.parse(value))))).toOption
+ }
+
+ val isoTimestamp = if (value.nonEmpty && Character.isDigit(value.head)) {
+ legacyTimestampFormatters.iterator.map(parseWithFormatter)
+ .collectFirst { case Some(timestamp) => timestamp }
+ } else {
+ None
+ }
+ isoTimestamp.orElse(customFormatter.flatMap(parseWithFormatter))
Review Comment:
**Blocking (P1):** Zone-less ISO-shaped values are tried against the
UTC-pinned `ISO_LOCAL_DATE_TIME` and space-separated formatters before the
user's `timestampFormat`. An overlapping custom format, the `timeZone` option,
and the session zone are therefore all silently ignored for those values.
Example under LEGACY, with the session zone (or `timeZone`) set to
`America/Los_Angeles` and `timestampFormat` set to `yyyy-MM-dd HH:mm:ss`:
`2024-02-29 12:34:56` was read as `2024-02-29T20:34:56Z` before this PR and is
now read as `2024-02-29T12:34:56Z`. Inference still reports TimestampType, so
nothing signals the shift. Formats that reorder fields, such as `yyyy-dd-MM
HH:mm:ss`, are also misread.
The `tries ISO timestamps before custom formats` test doesn't catch this.
Its only overlapping case carries an explicit offset, and its `SSSXXX` pattern
cannot parse the six-digit input at all, so the test passes in either order.
See **Shared repair plan 1** in the review body.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -1172,6 +1189,48 @@ class XmlTokenizer(
}
object StaxXmlParser {
+ private val legacyTimestampFormatters = Seq(
+ new DateTimeFormatterBuilder()
+ .parseCaseInsensitive()
+ .append(DateTimeFormatter.ISO_LOCAL_DATE)
+ .appendLiteral(' ')
+ .append(DateTimeFormatter.ISO_LOCAL_TIME)
+ .toFormatter()
+ .withZone(ZoneOffset.UTC),
+ DateTimeFormatter.ISO_LOCAL_DATE_TIME.withZone(ZoneOffset.UTC),
+ DateTimeFormatter.ISO_OFFSET_DATE_TIME,
+ DateTimeFormatter.ISO_INSTANT)
+
+ private[xml] def customLegacyTimestampFormatter(
+ options: XmlOptions): Option[DateTimeFormatter] = {
+ options.parameters.get(XmlOptions.TIMESTAMP_FORMAT).map { pattern =>
+ val format = DateTimeFormatter.ofPattern(pattern)
+ if (pattern.exists("VzOXxZ".contains(_))) {
+ format
+ } else {
+ format.withZone(options.timezone.map(ZoneId.of).orNull)
+ }
+ }
+ }
+
+ private[xml] def parseLegacyTimestamp(
+ value: String,
+ customFormatter: => Option[DateTimeFormatter]): Option[Long] = {
+ def parseWithFormatter(format: DateTimeFormatter): Option[Long] = {
+ // Preserve spark-xml's java.sql.Timestamp conversion, including
historical calendar rebasing.
+ Try(
+
DateTimeUtils.fromJavaTimestamp(Timestamp.from(Instant.from(format.parse(value))))).toOption
Review Comment:
**Nit (P3):** `Try(... format.parse(value) ...)` builds and throws a
`DateTimeParseException`, with a stack trace, for every formatter that doesn't
match. Under LEGACY that is:
- four exceptions for every digit-leading string that is not a timestamp
during schema inference (for example `2020-01-01` or `123 Main St`), or five
with a `timestampFormat`;
- two exceptions for every offset-bearing value on the read path.
The previous LEGACY path used the non-throwing `parseOptional`; SPARK-39281
made legacy-format inference exception-free for exactly this cost.
Pre-screening with `format.parseUnresolved(value, new ParsePosition(0))` and
checking that the whole input was consumed avoids the throw without changing
results.
**Verification:**
- **Behavior:** The existing LEGACY accept and reject expectations,
including impossible dates and trailing text, still hold.
- **Inspection:** The probing path for a non-matching formatter performs no
throwing parse.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -1172,6 +1189,48 @@ class XmlTokenizer(
}
object StaxXmlParser {
+ private val legacyTimestampFormatters = Seq(
+ new DateTimeFormatterBuilder()
+ .parseCaseInsensitive()
+ .append(DateTimeFormatter.ISO_LOCAL_DATE)
+ .appendLiteral(' ')
+ .append(DateTimeFormatter.ISO_LOCAL_TIME)
+ .toFormatter()
+ .withZone(ZoneOffset.UTC),
+ DateTimeFormatter.ISO_LOCAL_DATE_TIME.withZone(ZoneOffset.UTC),
+ DateTimeFormatter.ISO_OFFSET_DATE_TIME,
+ DateTimeFormatter.ISO_INSTANT)
+
+ private[xml] def customLegacyTimestampFormatter(
+ options: XmlOptions): Option[DateTimeFormatter] = {
+ options.parameters.get(XmlOptions.TIMESTAMP_FORMAT).map { pattern =>
+ val format = DateTimeFormatter.ofPattern(pattern)
Review Comment:
**Non-blocking (P2):** Under `spark.sql.legacy.timeParserPolicy=LEGACY`, an
explicit `timestampFormat` used to go through
`TimestampFormatter.getLegacyFormatter`, i.e.
`LegacyFastTimestampFormatter(pattern, options.zoneId, options.locale)`:
SimpleDateFormat semantics with the `locale` option, which defaults to en-US.
`DateTimeFormatter.ofPattern(pattern)` uses java.time letter semantics and the
JVM default locale instead. As a result:
- `locale=fr-FR` with month names, or English month names on a non-English
JVM, no longer parse. The values become null, a FAILFAST error, or StringType
in inference.
- Letters whose meaning differs between the two APIs change behavior. For
example, `dd/MM/yy HH:mm:ss` now reads `01/01/70 00:00:00` as 2070-01-01
instead of 1970-01-01 (checked with jshell).
This contradicts the documented LEGACY policy (SimpleDateFormat,
locale-sensitive) and the XML `locale` option, which is "used while parsing
dates and timestamps". CORRECTED and EXCEPTION are not affected.
See **Shared repair plan 1** in the review body.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -1172,6 +1189,48 @@ class XmlTokenizer(
}
object StaxXmlParser {
+ private val legacyTimestampFormatters = Seq(
+ new DateTimeFormatterBuilder()
+ .parseCaseInsensitive()
+ .append(DateTimeFormatter.ISO_LOCAL_DATE)
+ .appendLiteral(' ')
+ .append(DateTimeFormatter.ISO_LOCAL_TIME)
+ .toFormatter()
+ .withZone(ZoneOffset.UTC),
+ DateTimeFormatter.ISO_LOCAL_DATE_TIME.withZone(ZoneOffset.UTC),
+ DateTimeFormatter.ISO_OFFSET_DATE_TIME,
+ DateTimeFormatter.ISO_INSTANT)
+
+ private[xml] def customLegacyTimestampFormatter(
+ options: XmlOptions): Option[DateTimeFormatter] = {
+ options.parameters.get(XmlOptions.TIMESTAMP_FORMAT).map { pattern =>
+ val format = DateTimeFormatter.ofPattern(pattern)
+ if (pattern.exists("VzOXxZ".contains(_))) {
+ format
+ } else {
+ format.withZone(options.timezone.map(ZoneId.of).orNull)
+ }
+ }
+ }
+
+ private[xml] def parseLegacyTimestamp(
+ value: String,
+ customFormatter: => Option[DateTimeFormatter]): Option[Long] = {
+ def parseWithFormatter(format: DateTimeFormatter): Option[Long] = {
+ // Preserve spark-xml's java.sql.Timestamp conversion, including
historical calendar rebasing.
+ Try(
+
DateTimeUtils.fromJavaTimestamp(Timestamp.from(Instant.from(format.parse(value))))).toOption
Review Comment:
**Non-blocking (P2):** `Instant.from(...)` is already a proleptic-Gregorian
instant. `Timestamp.from` plus `DateTimeUtils.fromJavaTimestamp` then applies
`rebaseJulianToGregorianMicros` in the executor's JVM default time zone.
For `1500-01-01T12:34:56.123Z`, the stored value becomes
`1499-12-23T12:34:56.123Z` on a UTC JVM
(`Timestamp.from(Instant.parse("1500-01-01T12:34:56Z"))` renders as `1499-12-23
12:34:56.0`), and it is off by a few more minutes on, for example, an
`America/Los_Angeles` JVM. CAST to string, `show()`, and Parquet/ORC writes all
expose the shifted date. CORRECTED and the previous LEGACY reader both keep
1500-01-01, and pre-1582 values written by the LEGACY XML writer no longer
round-trip.
The new 1500-01-01 test case cannot catch this, because `collect()` applies
the inverse rebase in `toJavaTimestamp`. Converting with
`DateTimeUtils.instantToMicros`, and asserting on the stored or string-cast
value, would fix both.
**Verification:**
- **Regression:** A pre-1582 LEGACY XML timestamp is checked on its stored
or string-cast value, not through java.sql.Timestamp collection, and equals the
CORRECTED result.
- **Compatibility:** Post-1582 LEGACY ISO results remain unchanged.
--
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]
