Skip to content

fix: fall back from the native CSV scan for timestamps outside UTC - #6349

Merged
andygrove merged 1 commit into
apache:mainfrom
andygrove:fix-csv-v2-timestamp-timezone
Sep 30, 2026
Merged

andygrove merged 1 commit into
apache:mainfrom
andygrove:fix-csv-v2-timestamp-timezone

Conversation

@andygrove

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes #6332.

Rationale for this change

The native CSV V2 scan hands DataFusion's CsvSource the Spark schema, where TimestampType is Timestamp(Microsecond, "UTC"). As a result, arrow-csv reads a timestamp without an offset as UTC.

Spark's CSV reader interprets that same value in the CSV timeZone option, which defaults to the session timezone. Nothing passes that timezone to the native reader, because the CsvOptions proto has no field for it. In a non-UTC session, every such value was therefore silently shifted. For example, in America/Los_Angeles, 2024-01-15 18:30:45 came back as 2024-01-15T18:30:45Z instead of 2024-01-16T02:30:45Z.

What changes are included in this PR?

  • The CSV V2 branch of CometScanRule now falls back to Spark when the read schema has a TimestampType column and the CSV timezone isn't UTC. The CSV timezone is the timeZone option, or the session timezone when that option isn't set.
    • TimestampNTZType is unaffected, because neither reader applies a timezone to it.
    • The check normalizes the zone, so UTC, Etc/UTC, Z and +00:00 all stay native.
  • A new test in CometCsvNativeReadSuite covers four cases:
    • a Los Angeles session falls back
    • a Los Angeles session with timeZone=UTC stays native
    • a UTC session stays native
    • a UTC session with timeZone=Asia/Tokyo falls back

The scan is testing-only and off by default. Passing the timezone to the native reader would need a proto field and a native change to how timestamps are parsed, so this PR just makes the unsupported case fall back.

How are these changes tested?

  • With the old rule, the new test fails because the results don't match in the Los Angeles session.
  • With this change, all four CometCsvNativeReadSuite tests pass on Spark 4.1, including the spotless and scalastyle checks.

The native CSV V2 reader hands arrow-csv the Spark schema, whose TimestampType is labelled UTC, so a timestamp without an offset is parsed as UTC. Spark parses it in the CSV timeZone option, which defaults to the session timezone. Fall back when the read schema has a timestamp column and that timezone is not UTC.

Closes apache#6332.
@andygrove andygrove added backport-1.0 Candidate for backporting to 1.0 release branch backport-1.1 Candidate for backporting to 1.1 release branch labels Sep 28, 2026
@github-actions github-actions Bot added bug Something isn't working area:scan Parquet scan / data reading labels Sep 28, 2026

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

  • Prior state and problem: Native CSV scans interpreted offset-free timestamps as UTC, producing incorrect instants when Spark’s effective CSV timezone was non-UTC.
  • Design approach: Fall back to Spark when readDataSchema contains TimestampType and the effective timezone does not normalize to UTC.
  • Correctness / compatibility analysis: Verified timezone precedence and parsing against Spark 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0 sources. The rule matches Spark’s case-insensitive timeZone option and session default. A standalone Spark 4.1.3 parser probe passed 15 checks covering overrides, UTC aliases, and timezone-independent TimestampNTZType parsing.
  • Key design decisions: Uses existing fallback reporting without adding native or protocol abstractions. Additional work is confined to scan planning: a schema walk and, when needed, timezone resolution. No per-row processing is added.
  • Implementation sketch: Extends the CSV branch of CometScanRule and adds a regression test comparing results and execution paths under four timezone configurations.
  • Behavioral changes worth calling out: Non-UTC timestamp reads use Spark. UTC aliases and TimestampNTZType retain their existing eligibility. The native CSV feature remains testing-only and disabled by default.
  • Suggested improvements: None at P1/P2. No introduced P1/P2 issues found within this review.

Reviewed the entire two-file diff from e1d2c11729c2fc60a5def4e87bb17e5b28df2a29 to 603b1aa44adf0c8adabb9d4ea092fc5e92d51349. The PR is non-draft. The snapshot and live discussion contained no existing reviews or comments. Routed skill: review-comet-pr; no sibling review skill applies.

Exact-head CI: Spark 3.4/3.5/4.0 JVM compile/lint checks and the Spark 4.1 build passed. No failed checks were reported. Native build and Rust tests remained in progress, with no Comet CSV suite result available yet. Spark SQL integration jobs were skipped.

Validation limits: The local probe exercises Spark’s parser, not Comet’s complete execution path. I did not build the native library or run CometCsvNativeReadSuite or the Spark SQL integration suite locally. End-to-end validation therefore remains outstanding.

@andygrove
andygrove added this pull request to the merge queue Sep 29, 2026
Merged via the queue into apache:main with commit e788041 Sep 30, 2026
74 checks passed
@andygrove andygrove removed backport-1.1 Candidate for backporting to 1.1 release branch backport-1.0 Candidate for backporting to 1.0 release branch labels Oct 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:scan Parquet scan / data reading bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Native CSV V2 scan parses timestamps as UTC and ignores the session timezone

2 participants