Skip to content

[SkipRecovery] Re-enable CORRECTED Parquet timestamp checks [databricks] - #15969

Open
wjxiz1992 wants to merge 1 commit into
NVIDIA:mainfrom
wjxiz1992:codex/skip132-parquet-corrected
Open

[SkipRecovery] Re-enable CORRECTED Parquet timestamp checks [databricks]#15969
wjxiz1992 wants to merge 1 commit into
NVIDIA:mainfrom
wjxiz1992:codex/skip132-parquet-corrected

Conversation

@wjxiz1992

Copy link
Copy Markdown
Collaborator

JaCoCo production line coverage: +0 lines (measured: sql-plugin, delta-lake, iceberg, shuffle-plugin, udf-compiler; Scala 2.12, shim 350, vs compatible nightly b34, anchor 2d438c020496)

Contributes to #132.

Description

This is a test-and-documentation recovery; production behavior is unchanged. The existing CORRECTED Parquet timestamp round-trip cases produce matching CPU/GPU results on the tested runtime, but an old expected-failure marker still hides future failures. Remove that marker so these cases enforce parity, and correct the compatibility documentation's blanket warning about ancient CORRECTED timestamps.

  • Restores normal failure reporting for 196 existing timestamp combinations across ordinary and flow-controlled readers, without adding tests or parameter combinations.
  • Follows the issue's request to retest and the later observation that the discrepancy no longer reproduces on Spark 3.1+. The documented supported case is CPU-written files with both datetime and INT96 write rebasing set to CORRECTED.
  • Leaves LEGACY rebasing, INT96 timestamp conversion, older-writer files, and the other issue-linked generator restrictions unchanged. This does not close the broader issue.

Validation used Apache Spark 3.5.0, Python 3.10.18, Java 17, and an RTX 5880 Ada. No CPU/GPU divergence was observed in the exercised scope. Databricks and other Spark shims have not been validated locally.

Local validation and coverage measurement
  • Maven package -pl dist,integration_tests -am -Dbuildver=350 -DskipTests, with an isolated Maven cache: BUILD SUCCESS.
  • Before editing, both affected tests with --runxfail: 392 passed.
  • After editing, all 196 recovered combinations with each of TIMESTAMP_MICROS and TIMESTAMP_MILLIS: 196 passed per encoding. INT96 with a second seed and forced OOM injection: 196 passed.
  • A temporary diagnostic checked 11 boundary values (including null, year 1, the calendar cutover, 1590, 1900, epoch, and year 9999), three encodings, three GPU readers, and V1/V2 scans: 18 passed in each of UTC, Asia/Shanghai, and America/New_York. It explicitly required GPU scan nodes and rejected CPU scan fallback.
  • Full modified parquet_test.py: 3721 passed, 89 skipped in 1232.50 seconds, exit 0. The 89 skips are existing runtime guards, not expected failures restored by this change.

Affected existing tests: test_parquet_read_round_trip and test_parquet_read_multithread_flow_ctrl_round_trip in integration_tests/src/main/python/parquet_test.py. The recovered timestamp-only selection is -k '[Timestamp]' (52 ordinary-reader cases plus 144 flow-control cases). Runs used seed 20260911, with 20260912 for the forced-OOM run; both CORRECTED write-rebase settings are set by the tests.

JaCoCo replayed all 196 recovered cases against the exact compatible b34 anchor JAR and merged their execution data with that baseline. All five regenerated module baseline counters matched the published report, with no class-ID mismatch; each module's marginal line delta was zero. This measures restored regression enforcement, not new production lines. b34 is a compatible baseline, not the latest nightly.

AI assistance: The change and PR description were prepared with Codex assistance.

Checklists

Documentation

  • Updated for new or modified user-facing features or behaviors
  • No user-facing change

Testing

  • Added or modified tests to cover new code paths
  • Covered by existing tests
    (Please provide the names of the existing tests in the PR description.)
  • Not required

Performance

  • Tests ran and results are added in the PR description
  • Issue filed with a link in the PR description
  • Not required

Signed-off-by: Allen Xu <allxu@nvidia.com>
Copilot AI lite review requested due to automatic review settings September 11, 2026 06:41
@wjxiz1992 wjxiz1992 added documentation Improvements or additions to documentation test Only impacts tests labels Sep 11, 2026
@wjxiz1992 wjxiz1992 self-assigned this Sep 11, 2026
@greptile-apps

greptile-apps Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

The PR appears safe to merge; the restored tests stay within the documented CORRECTED rebasing scope and no actionable defect was identified.

Summary

  • Removes the issue-132 expected-failure mark from the shared timestamp generator parameter.
  • Exercises the restored parameter through the ordinary and flow-controlled Parquet reader matrices.
  • Documents that LEGACY rebasing and INT96 timestamp-conversion limitations remain unchanged.

Reviews (1) · Last reviewed commit: "Re-enable CORRECTED Parquet timestamp ro..."

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

Changes are well validated; the remaining documentation nit is non-blocking.

Pull request overview

Re-enables existing CORRECTED Parquet timestamp parity checks and updates compatibility guidance without changing production behavior.

Changes:

  • Removes expected-failure markers from timestamp round-trip tests.
  • Documents supported CORRECTED timestamp behavior and limitations.
  • Notes version-specific Parquet rebasing configuration keys.
File summaries
File Description
integration_tests/src/main/python/parquet_test.py Restores normal failure reporting for timestamp round-trip cases.
docs/compatibility.md Updates Parquet timestamp compatibility guidance.
Review details

Suppressed comments (1)

docs/compatibility.md:319

  • The write-rebase keys are version-dependent in this repository: delta_lake_write_test.py:1089-1098 uses the spark.sql.legacy.parquet.* keys before Spark 4.0 and the spark.sql.parquet.* keys from Spark 4.0 onward. As written, this compatibility statement can mislead users of the supported pre-4.0 Spark versions into setting keys that are not the version-appropriate controls; please document both forms or scope this claim to Spark 4.0+.
on Spark 3.0. For files written by the CPU on supported Spark versions with both
`spark.sql.parquet.datetimeRebaseModeInWrite` and `spark.sql.parquet.int96RebaseModeInWrite`
set to `CORRECTED`, GPU reads support timestamps before the transition between the Julian
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@wjxiz1992

Copy link
Copy Markdown
Collaborator Author

build

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation test Only impacts tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants