Fix legacy ORC timestamp rebasing [databricks] - #15935
Conversation
Signed-off-by: Allen Xu <allxu@nvidia.com>
|
There was a problem hiding this comment.
🟡 Changes recommended
The added GPU timestamp rebasing work introduces new GPU allocations on a path that is currently outside the ORC decode OOM-retry scope, which can reduce runtime reliability under memory pressure.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes a GPU/CPU correctness mismatch when reading Spark 2.4 legacy-calendar ORC timestamp columns by applying Spark’s timezone-specific Julian-to-Gregorian timestamp rebasing on the GPU, with targeted host fallbacks for edge cases.
Changes:
- Introduces a Spark-package bridge (
RebaseDateTimeBridge) and GPU rebase utility (GpuTimestampRebaseUtils) to apply Spark’s exact timezone-specific timestamp rebase maps. - Integrates legacy timestamp rebasing into the ORC timezone rebasing flow, including nested columns, while preserving the existing proleptic-calendar rule-correction path.
- Re-enables the previously excluded SPARK-31284 read-compatibility tests and adds focused unit coverage for timestamp rebasing boundaries and fallbacks.
File summaries
| File | Description |
|---|---|
| tests/src/test/spark330/scala/org/apache/spark/sql/rapids/utils/RapidsTestSettings.scala | Removes the SPARK-31284 legacy ORC timestamp read exclusion now that behavior is fixed. |
| tests/src/test/scala/com/nvidia/spark/rapids/timezone/OrcTimezoneSuite.scala | Extends timestamp test vectors to include the SPARK-31284 legacy timestamp value. |
| tests/src/test/scala/com/nvidia/spark/rapids/OrcCalendarSuite.scala | Adds an end-to-end CPU vs GPU equality test for Spark 2.4 legacy ORC timestamp reading in a fixed session timezone. |
| tests/src/test/scala/com/nvidia/spark/rapids/GpuTimestampRebaseSuite.scala | Adds direct unit tests asserting GPU rebasing matches Spark at boundaries, for fixed-offset/short IDs, and for fallback scenarios. |
| sql-plugin/src/main/scala/org/apache/spark/sql/rapids/RebaseDateTimeBridge.scala | Provides access to Spark’s runtime-specific rebase records (and slow-path fallback) from a sql-package bridge. |
| sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuTimestampRebaseUtils.scala | Implements GPU-side Julian→Gregorian timestamp rebasing using Spark’s rebase tables with BCE/unknown-ID fallbacks. |
| sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuOrcTimezoneUtils.scala | Wires legacy timestamp rebasing into the ORC timestamp conversion path and propagates it through nested columns. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| val legacyTimestampRebase = if (writerUsedProlepticGregorian) { | ||
| None | ||
| } else { | ||
| Some(new GpuTimestampRebaseUtils.LazyJulianToGregorianMicrosContext(readerZone.getId)) | ||
| } |
| if (delegate == null) { | ||
| delegate = createJulianToGregorianMicrosContext(timeZoneId) | ||
| } | ||
| delegate.rebase(input) | ||
| } |
Signed-off-by: Allen Xu <allxu@nvidia.com>
Signed-off-by: Allen Xu <allxu@nvidia.com>
|
build |
Signed-off-by: Allen Xu <allxu@nvidia.com>
|
build |
Signed-off-by: Allen Xu <allxu@nvidia.com>
|
build |
1 similar comment
|
build |
|
It will take some time to complete the review. |
JaCoCo production line coverage: +140 lines (
sql-plugin +140; all affected JVM production modules:sql-plugin; exact changed-class fix-line intersection, Spark shim 330)Fixes #15471. Contributes to #131.
Description
This correctness fix makes historical timestamps in legacy-calendar ORC files, including Spark 2.4 files, match Spark CPU reads. The GPU reader already converted the writer timezone to the reader timezone but omitted Spark's timezone-specific Julian-to-Gregorian calendar rebase, producing incorrect dates and times.
Apply that rebase using the reader JVM's default timezone, including when
spark.sql.session.timeZonediffers. This covers the legacy timestamp case in #15471; the separate proleptic Gregorian cross-timezone cutover discrepancy remains tracked by #131 and documented as a limitation.integration_tests/src/main/python/orc_test.py. Generate real legacy/proleptic ORC files, verify stripe metadata, compare CPU/GPU timestamp microseconds and null masks, and assert GPU scans. The PR adds no unit-suite tests.Review-sensitive areas are the Spark package bridge, GPU lookup boundaries, timezone normalization, and resource ownership during retries.
Original Spark test:
OrcSourceSuite.scala, lines 512–518.AI assistance: The change and PR description were prepared with Codex assistance.
Validation
OrcCalendarSuiteandOrcTimezoneSuite: 57 succeeded, 0 failed, 0 canceled, 0 ignored, 0 pending. Spark 3.3 tests/distribution/integration-test reactor: BUILD SUCCESS.git diff --checkpassed.Integration-test scope and performance
The generated-file matrix covers seven writer timezones, PERFILE/COALESCING/MULTITHREADED, V1/V2, CPU row/vectorized readers, chunking, scalar/struct/array timestamps, parent/child nulls, mixed modern/legacy values, empty files, every runtime map switch ±1 microsecond, BCE, and the normalized EST regression. Each case compares two SQL session timezones. Reader JVM timezones are configured by the test harness for both driver and executors; only driver-side fixture generation temporarily changes the writer timezone. The original Spark 2.4 fixture also retains its full V1/vectorized/chunked/session-timezone product and OOM injection.
The missing-map case runs when the reader JVM uses a region absent from Spark's bundled maps; map-boundary cases apply when records exist. The preserved proleptic-cutover reproducer runs with a UTC reader JVM and is a strict expected failure linked to #131. Its CPU/GPU equality assertion remains intact, so unexpected recovery fails the test. Existing migrated-test exclusions remain in place.
Performance was measured on the unchanged production implementation at commit 7425160: one million modern timestamps, RTX 5880 Ada / Java 17, one warmup and five measured rounds, JaCoCo enabled. America/Los_Angeles → UTC reads averaged CPU 121 ms / GPU 70 ms; same-timezone reads averaged CPU 77 ms / GPU 69 ms. CPU/GPU summaries and GPU scan checks passed. These are end-to-end timings, not a before/after regression estimate. Fixed-offset normalization runs only in the host fallback, once per non-null value, without adding device allocations or synchronization to GPU-map or modern fast paths.
Checklists
Documentation
Testing
(Please provide the names of the existing tests in the PR description.)
Performance