HyukjinKwon commented on code in PR #59406:
URL: https://github.com/apache/spark/pull/59406#discussion_r4274637665
##########
docs/sql-data-sources-xml.md:
##########
@@ -184,7 +184,14 @@ Data source options of XML can be set via:
<tr>
<td><code>dateFormat</code></td>
<td><code>yyyy-MM-dd</code></td>
- <td>Sets the string that indicates a date format. Custom date formats
follow the formats at <a
href="https://spark.apache.org/docs/latest/sql-ref-datetime-pattern.html";>
datetime pattern</a>. This applies to date type.</td>
+ <td>Sets the string that indicates a date format. Custom date formats
follow the formats at <a
href="https://spark.apache.org/docs/latest/sql-ref-datetime-pattern.html";>
datetime pattern</a>. This applies to date type.
+ With <code>spark.sql.legacy.timeParserPolicy=LEGACY</code>, an explicit
format and a Gregorian
+ <code>locale</code>, XML date reads try ISO dates before this pattern for
spark-xml compatibility.
+ Non-Gregorian calendars continue to use the configured pattern. Writes
still use this pattern,
+ so ISO-ambiguous formats such as <code>yyyy-dd-MM</code> do not
necessarily round-trip under
+ <code>LEGACY</code>. <code>CORRECTED</code> and <code>EXCEPTION</code>
keep configured-format
+ precedence; <code>EXCEPTION</code> does not report differences when both
interpretations
Review Comment:
**Nit (P3):** This caveat, and the matching sentence in
`docs/sql-migration-guide.md:35`, covers only the case where both
interpretations parse. Under `LEGACY`, the ISO pre-pass also accepts ISO-shaped
values that the configured pattern rejects, and `EXCEPTION` doesn't report
those either.
For example, take `dateFormat=dd/MM/yyyy` and `<d>2024-03-04</d>`:
- `LEGACY` returns 2024-03-04 from `parseIsoDate`.
- Under `EXCEPTION`, the java.time parser fails.
`DateTimeFormatterHelper.checkParsedDiff` then retries only the
configured-pattern `FastDateFormat`, which also rejects the value; I checked
`dd/MM/yyyy`, `yyyyMMdd` and `dd.MM.yyyy` with commons-lang3 3.17.0. The
original parse error is rethrown, so the value becomes NULL or a corrupt record
in `PERMISSIVE` and an error in `FAILFAST`, with no `SparkUpgradeException`.
A user who switches from `LEGACY` to `EXCEPTION` to find changed results
would get silent NULLs for these values. The code matches the existing CSV/JSON
LEGACY fallback, which has the same `EXCEPTION` blind spot, so only the wording
needs to change. For example, in both docs: "`EXCEPTION` does not report XML
ISO-first differences, including ISO dates that the configured pattern
rejects." A test with a non-ISO `dateFormat` and an ISO-shaped value across the
three policies would pin this boundary.
**Verification:**
- **Inspection:** The XML dateFormat option text and the migration note
describe the EXCEPTION boundary as covering every LEGACY ISO-first divergence,
including ISO-only values, and match each other.
- **Behavior:** XML datasource coverage with a non-ISO dateFormat (e.g.
dd/MM/yyyy) and an ISO-shaped value asserts LEGACY returns the ISO date, and
CORRECTED and EXCEPTION yield NULL in PERMISSIVE and fail in FAILFAST without a
SparkUpgradeException.
- **Compatibility:** The existing SPARK-60201 policy and mode expectations
for yyyy-dd-MM still hold unchanged.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -1172,6 +1180,23 @@ class XmlTokenizer(
}
object StaxXmlParser {
+ private[xml] def parseIsoDate(value: String): Option[Int] = {
+ if (value.nonEmpty && Character.isDigit(value.head)) {
+ val position = new ParsePosition(0)
+ val parsed = DateTimeFormatter.ISO_DATE.parseUnresolved(value, position)
Review Comment:
**Nit (P3):** The `Try` on line 1188 doesn't cover `parseUnresolved`. That
method returns `null` for ordinary parse errors, but its contract also declares
`@throws DateTimeException`, and `ISO_DATE`'s offset parser throws when the
offset is out of range. For example, `2020-01-01+25:00` throws
`DateTimeException: Value out of range: Hour[0-23], Minute[0-59],
Second[0-59]`; I reproduced this on JDK 11, 17 and 25.
The exception escapes `parseIsoDate`, so `parseXmlDate` never reaches
`dateFormatter.parse`. Under `LEGACY` with `dateFormat=yyyy-MM-dd` or
`yyyy-dd-MM`, the merge target reads this value as 2020-01-01, because
`FastDateFormat` ignores the trailing text. With this change, the record
handler (lines 245-250) turns the exception into a `BadRecordException`. The
row becomes a corrupt record in `PERMISSIVE`, is dropped in `DROPMALFORMED`,
and fails the read in `FAILFAST`. This affects only `LEGACY` reads with an
explicit `dateFormat` and a Gregorian locale; `CORRECTED` and `EXCEPTION` never
run the pre-check.
These are invalid `xs:date` values, so this should be rare. Still, every
value the ISO path can't accept is supposed to fall back to the configured
pattern. Catching `DateTimeException` around `parseUnresolved` keeps that
fallback total, and it combines naturally with the range-check change suggested
on line 1188.
See **Shared repair plan 1** in the review body.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -1172,6 +1180,23 @@ class XmlTokenizer(
}
object StaxXmlParser {
+ private[xml] def parseIsoDate(value: String): Option[Int] = {
+ if (value.nonEmpty && Character.isDigit(value.head)) {
+ val position = new ParsePosition(0)
+ val parsed = DateTimeFormatter.ISO_DATE.parseUnresolved(value, position)
+ if (parsed != null && position.getIndex == value.length) {
+ Try(LocalDate.parse(value, DateTimeFormatter.ISO_DATE)).toOption.map {
date =>
Review Comment:
**Nit (P3):** The new `parseUnresolved` pre-check avoids the exception for
non-ISO shapes. Values that are ISO-shaped but invalid still depend on
`Try(LocalDate.parse(...))` to reject them, though, so each one builds and
throws a `DateTimeParseException` with a stack trace.
With `yyyy-dd-MM`, the ISO-ambiguous pattern this PR targets, every value
whose day is 13-31 looks like an ISO date with an invalid month, for example
`2024-31-01`. That is 19 of 31 possible days, so in such a column most values
pay for an exception before the configured pattern parses them. In a local JDK
17 microbenchmark of this pre-check at stack depth 60, `2024-31-01` took about
6.5 us, compared with about 0.7 us for `2024-03-04`. Executors with deeper
stacks pay more. Parsed results don't change, and this applies only to `LEGACY`
reads with an explicit `dateFormat` and a Gregorian locale.
The description says the pre-check doesn't throw for routine inputs, which
is how the earlier per-value cost question was answered. `parseUnresolved`
already returns the parsed fields. Range-checking them before building the date
(month 1-12, and a day within that month's length for the parsed year) would
let these values return `None` without throwing.
See **Shared repair plan 1** in the review body.
--
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]
