Skip to content

Commit 9d760a9

Browse files
committed
[SPARK-57884][SQL] Make XML schema inference honor preferDate, consistent with CSV
### What changes were proposed in this pull request? Make XML schema inference honor the `preferDate` option as a gate on whether date inference is attempted, matching `CSVInferSchema`. Currently, `XmlInferSchema.tryParseDouble` and `tryParseTime` fall through to `tryParseDate(field)` **unconditionally**, ignoring `options.preferDate`. As a result, with `preferDate=false`, a value that matches the (default) date format is still inferred as `DateType`. This contradicts the option's purpose — `preferDate=false` is meant to disable date inference (date/timestamp inference is ambiguous, and the option lets users opt out) — and diverges from the CSV datasource, whose `tryParseDouble`/`tryParseTime` gate the date attempt on `options.preferDate`. This change routes the date attempt through `preferDate`: when `preferDate` is true, a bare date still infers as `DateType`; when false, date inference is skipped and the value falls through to timestamp inference. TIME inference is unaffected (it remains tried ahead of date/timestamp regardless of `preferDate`, unchanged from before). ### Why are the changes needed? `preferDate` is documented and structured (see `XmlOptions.dateFormatInRead`, which is conditioned on `preferDate`) as controlling whether date inference happens, but the `tryParse*` control flow does not honor it, so a bare ISO date leaks through as `DateType` regardless. The CSV datasource — which XML's inference cascade mirrors — already gates date inference on `preferDate`. This aligns XML with CSV so `preferDate=false` behaves consistently across the two text datasources. ### Does this PR introduce _any_ user-facing change? Yes. With `preferDate=false`, XML schema inference no longer infers `DateType` for date-shaped values; such values now fall through to timestamp inference (as they already do in CSV). With the default `preferDate=true`, behavior is unchanged. ### How was this patch tested? Updated the `preferDate` tests in `XmlInferSchemaSuite` (core) and `XmlInferSchemaTypeCastingSuite` (catalyst) to assert the gated behavior: `preferDate=true` -> `DateType`, `preferDate=false` -> `TimestampType`. Existing XML inference suites pass. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Anthropic Claude Opus) Closes #56966 from cloud-fan/SPARK-preferDate-xml-followup. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
1 parent 41b2739 commit 9d760a9

4 files changed

Lines changed: 76 additions & 19 deletions

File tree

AGENTS.md

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,15 @@ Spark Connect protocol is defined in proto files under `sql/connect/common/src/m
2020

2121
Avoid introducing non-ASCII characters in code or comments. String literals may contain non-ASCII when the content requires it (error messages, test data, etc.). Identifiers are ASCII by convention. The common failure mode is typographic characters (em-dash, smart quotes, ellipsis, non-breaking space) sneaking into comments; scalastyle flags some of these. Spot-check before committing: `grep -rn -P "[^\x00-\x7F]" <files>`.
2222

23+
Keep source lines within 100 characters — the linters enforce this for Scala, Java, and Python, and LLMs commonly overrun it in comments and long expressions. A quick scan of just the changed files catches most cases in seconds, far cheaper than a CI round trip:
24+
25+
{ git diff --name-only --diff-filter=ACM HEAD; git ls-files --others --exclude-standard; } \
26+
| grep -E '\.(scala|java|py)$' | sort -u \
27+
| xargs -r awk 'length>100 && $0 !~ /^[[:space:]]*(import|package) / && $0 !~ /https?:\/\// \
28+
{print FILENAME":"FNR": "length" chars"}'
29+
30+
This is only a hint: it approximates the linters' exemptions (imports, URLs) rather than matching them exactly, so it can over- or under-report. The linters remain the source of truth.
31+
2332
## Scala Test Base Classes
2433

2534
When writing a new Scala test suite, pick the lowest base class that provides what the test actually needs. Spark uses the `AnyFunSuite` ScalaTest style throughout, so the bases below are the chain to choose from. Each adds capability on top of the previous:
@@ -147,22 +156,32 @@ Run a single test case:
147156

148157
## Investigating PR CI Failures
149158

150-
Do NOT download full job logs to grep for errors — they are very large and slow. Instead, use the test report annotations on the fork.
159+
Enumerate all failing check runs first, then drill into each by type. Do not assume a single failure: a PR can fail tests, linters, and the build at once, and these surface through different channels.
151160

152161
Step 1 — Get the fork owner and the latest commit SHA of the PR:
153162

154163
gh api repos/apache/spark/pulls/<PR_NUMBER> --jq '{owner: .head.repo.owner.login, sha: .head.sha}'
155164

156-
Step 2 — Find the "Report test results" check run on the fork's commit:
165+
Step 2 — List every failing check run on the fork's commit. This is the complete failure set:
166+
167+
gh api repos/<OWNER>/spark/commits/<SHA>/check-runs --paginate \
168+
--jq '.check_runs[] | select(.conclusion == "failure") | {name, id: .id}'
169+
170+
A passing (or absent) "Report test results" does NOT mean CI is green. That check aggregates only test-case failures; linter, license, dependency, MiMa, compile, and doc-build failures are separate check runs that produce no test annotations. Always work from the list in Step 2, not from any single check.
171+
172+
Step 3 — Drill into each failure according to its kind:
173+
174+
- **Test jobs** (e.g. "Report test results", "Build modules: ..."): fetch failure annotations. Each annotation contains the test class, test name, and failure message:
157175

158-
gh api repos/<OWNER>/spark/commits/<SHA>/check-runs \
159-
--jq '.check_runs[] | select(.name == "Report test results") | {id: .id, annotations: .output.annotations_count}'
176+
gh api repos/<OWNER>/spark/check-runs/<CHECK_RUN_ID>/annotations
160177

161-
Step 3 — Fetch failure annotations:
178+
- **Non-test jobs** (e.g. "Linters, licenses, and dependencies", "Build"): find the failed step, then read only that job's log:
162179

163-
gh api repos/<OWNER>/spark/check-runs/<CHECK_RUN_ID>/annotations
180+
gh api repos/<OWNER>/spark/actions/jobs/<JOB_ID> \
181+
--jq '{name, steps: [.steps[] | select(.conclusion == "failure") | .name]}'
182+
gh api repos/<OWNER>/spark/actions/jobs/<JOB_ID>/logs
164183

165-
Each annotation contains the test class, test name, and failure message.
184+
Avoid downloading the large per-shard *test* job logs — they are very large and slow; use the annotations for those. Lint, license, dependency, and build job logs are small and fine to read directly when a step fails.
166185

167186
## Checking PR Merge Status
168187

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlInferSchema.scala

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -682,16 +682,20 @@ class XmlInferSchema(private val options: XmlOptions, private val caseSensitive:
682682
} else if (isTimeTypeEnabled && isTime(field)) {
683683
// TIME is tried ahead of date/timestamp, matching the ordering of the previous cascade.
684684
TimeType(TimeType.DEFAULT_PRECISION)
685-
} else {
685+
} else if (options.preferDate) {
686686
tryParseDate(field)
687+
} else {
688+
tryParseTimestampNTZ(field)
687689
}
688690
}
689691

690692
private def tryParseTime(field: String): DataType = {
691693
if (isTimeTypeEnabled && isTime(field)) {
692694
TimeType(TimeType.DEFAULT_PRECISION)
693-
} else {
695+
} else if (options.preferDate) {
694696
tryParseDate(field)
697+
} else {
698+
tryParseTimestampNTZ(field)
695699
}
696700
}
697701

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/xml/XmlInferSchemaTypeCastingSuite.scala

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -155,11 +155,44 @@ class XmlInferSchemaTypeCastingSuite extends SparkFunSuite with SQLHelper {
155155
assert(inferSchema.inferFrom("2024-01-15T10:00:00", DateType) == TimestampType)
156156
}
157157

158-
test("date is inferred regardless of preferDate") {
158+
test("preferDate gates date inference (consistent with CSV)") {
159+
// preferDate controls whether date inference is attempted, matching CSVInferSchema: when true
160+
// a bare date infers as DateType; when false date inference is skipped and the value falls
161+
// through to timestamp inference.
162+
assert(newInferSchema(Map("preferDate" -> "true"))
163+
.inferFrom("2024-01-15", NullType) == DateType)
164+
assert(newInferSchema(Map("preferDate" -> "false"))
165+
.inferFrom("2024-01-15", NullType) == TimestampType)
166+
}
167+
168+
test("preferDate gates the incremental temporal re-entry (tryParseTime)") {
169+
// Refining a temporal `typeSoFar` re-enters the cascade at `tryParseTime`, which must honor
170+
// `preferDate` just like the fresh path (`tryParseDouble`). With a `Date`-so-far field seeing
171+
// another date-only value: preferDate=true re-infers `Date` and the merge stays `Date`;
172+
// preferDate=false skips date inference so the value falls through to `Timestamp`, and the
173+
// merge widens `Date` to `Timestamp`. This guards the `tryParseTime` branch, which the
174+
// incremental-vs-legacy parity test does not cover (it runs only at the default
175+
// preferDate=true).
176+
assert(newInferSchema(Map("preferDate" -> "true"))
177+
.inferFrom("2024-01-15", DateType) == DateType)
178+
assert(newInferSchema(Map("preferDate" -> "false"))
179+
.inferFrom("2024-01-15", DateType) == TimestampType)
180+
}
181+
182+
test("TIME inference precedes the preferDate gate") {
183+
// TIME is tried ahead of date/timestamp in both `tryParseDouble` (fresh path) and
184+
// `tryParseTime` (incremental temporal re-entry), so a TIME-shaped value infers as `TimeType`
185+
// regardless of `preferDate`. This guards against a refactor that would move the TIME guard
186+
// inside the `preferDate` branch and silently drop TIME inference when preferDate=false.
187+
val time = TimeType(TimeType.DEFAULT_PRECISION)
159188
Seq("true", "false").foreach { preferDate =>
160189
val inferSchema = newInferSchema(Map("preferDate" -> preferDate))
161-
assert(inferSchema.inferFrom("2024-01-15", NullType) == DateType,
162-
s"expected DateType with preferDate=$preferDate")
190+
// Fresh path (typeSoFar == NullType, enters via tryParseDouble).
191+
assert(inferSchema.inferFrom("10:00:00", NullType) == time,
192+
s"expected TimeType with preferDate=$preferDate")
193+
// Incremental temporal re-entry (typeSoFar == TimeType, enters via tryParseTime).
194+
assert(inferSchema.inferFrom("10:00:00", time) == time,
195+
s"expected TimeType with preferDate=$preferDate")
163196
}
164197
}
165198
}

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/xml/XmlInferSchemaSuite.scala

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -722,14 +722,15 @@ class XmlInferSchemaSuite
722722
assert(readData(doubleThenLong).schema.fields.head.dataType === DoubleType)
723723
}
724724

725-
test("date is inferred regardless of preferDate") {
725+
test("preferDate gates date inference (consistent with CSV)") {
726726
val xmlDate = Seq("""<ROW><d>2024-01-15</d></ROW>""")
727-
// preferDate governs which date formatter is used, not whether date inference is attempted.
728-
Seq("true", "false").foreach { preferDate =>
729-
val df = readData(xmlDate, Map("preferDate" -> preferDate))
730-
assert(df.schema.fields.head.dataType === DateType,
731-
s"expected DateType with preferDate=$preferDate")
732-
}
727+
// preferDate controls whether date inference is attempted, matching CSVInferSchema: when true
728+
// a bare date infers as DateType; when false date inference is skipped and the value falls
729+
// through to timestamp inference.
730+
assert(readData(xmlDate, Map("preferDate" -> "true"))
731+
.schema.fields.head.dataType === DateType)
732+
assert(readData(xmlDate, Map("preferDate" -> "false"))
733+
.schema.fields.head.dataType === TimestampType)
733734
}
734735

735736
test("incremental type casting yields the same schema as the legacy batch path") {

0 commit comments

Comments
 (0)