Repository navigation
feat(workflow-core): store a timestamp before 1883 at the wall clock it holds - #9076
Merged
Merged
Conversation
…t holds IcebergUtil wrote a Timestamp through an instant. A Timestamp gives a date before 1900 the zone's standard offset, and java.time reads the instant back with the zone's local mean time before 1883, so the stored value moved by the difference. It is now stored as its own wall clock. Fixes apache#9074 Co-Authored-By: Claude Opus 5.5 <[email protected]>
Contributor
Automated Reviewer SuggestionsBased on the
|
Contributor
Author
carloea2
approved these changes
Oct 23, 2026
carloea2
left a comment
carloea2
left a comment
Contributor
There was a problem hiding this comment.
Verified on fresh upstream main with JDK 17. The regression fails with the old conversion, shifting 08:30 to 08:37:02, and passes with this change. All 24 IcebergUtilSpec tests and scalafmtCheckAll pass. Code looks good. Current CI is blocked before tests by the Ampel action allowlist.
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 378 | 0.23 | 26,021/38,143/38,143 us | 🔴 +25.2% / 🔴 +212.0% |
| 🔴 | bs=100 sw=10 sl=64 | 829 | 0.506 | 119,342/135,939/135,939 us | 🟢 -8.4% / 🔴 +56.0% |
| ⚪ | bs=1000 sw=10 sl=64 | 951 | 0.58 | 1,046,196/1,128,602/1,128,602 us | ⚪ within ±5% / 🔴 +38.0% |
Baseline details
Latest main 8fbd4f2 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 378 tuples/sec | 423 tuples/sec | 1,019 tuples/sec | -10.6% | -62.9% |
| bs=10 sw=10 sl=64 | MB/s | 0.23 MB/s | 0.258 MB/s | 0.622 MB/s | -10.9% | -63.0% |
| bs=10 sw=10 sl=64 | p50 | 26,021 us | 20,784 us | 10,144 us | +25.2% | +156.5% |
| bs=10 sw=10 sl=64 | p95 | 38,143 us | 35,643 us | 12,225 us | +7.0% | +212.0% |
| bs=10 sw=10 sl=64 | p99 | 38,143 us | 35,643 us | 15,527 us | +7.0% | +145.7% |
| bs=100 sw=10 sl=64 | throughput | 829 tuples/sec | 863 tuples/sec | 1,371 tuples/sec | -3.9% | -39.5% |
| bs=100 sw=10 sl=64 | MB/s | 0.506 MB/s | 0.526 MB/s | 0.837 MB/s | -3.8% | -39.5% |
| bs=100 sw=10 sl=64 | p50 | 119,342 us | 111,520 us | 79,910 us | +7.0% | +49.3% |
| bs=100 sw=10 sl=64 | p95 | 135,939 us | 148,438 us | 87,155 us | -8.4% | +56.0% |
| bs=100 sw=10 sl=64 | p99 | 135,939 us | 148,438 us | 106,643 us | -8.4% | +27.5% |
| bs=1000 sw=10 sl=64 | throughput | 951 tuples/sec | 946 tuples/sec | 1,415 tuples/sec | +0.5% | -32.8% |
| bs=1000 sw=10 sl=64 | MB/s | 0.58 MB/s | 0.577 MB/s | 0.864 MB/s | +0.5% | -32.9% |
| bs=1000 sw=10 sl=64 | p50 | 1,046,196 us | 1,048,957 us | 780,407 us | -0.3% | +34.1% |
| bs=1000 sw=10 sl=64 | p95 | 1,128,602 us | 1,176,012 us | 817,814 us | -4.0% | +38.0% |
| bs=1000 sw=10 sl=64 | p99 | 1,128,602 us | 1,176,012 us | 848,683 us | -4.0% | +33.0% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,529.77,200,128000,378,0.230,26021.02,38142.55,38142.55
1,100,10,64,20,2412.39,2000,1280000,829,0.506,119341.82,135939.44,135939.44
2,1000,10,64,20,21035.67,20000,12800000,951,0.580,1046196.43,1128601.83,1128601.83
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9076 +/- ##
=========================================
Coverage 91.81% 91.81%
- Complexity 5069 5070 +1
=========================================
Files 1271 1271
Lines 55208 55208
Branches 6987 6987
=========================================
+ Hits 50688 50690 +2
+ Misses 2800 2799 -1
+ Partials 1720 1719 -1
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this PR?
IcebergUtil.toGenericRecordstored a TIMESTAMP field by turning thejava.sql.Timestampinto an instant and back into a wall clock in the machine's zone. The two steps follow different zone rules. A Timestamp gives a date before 1900 the zone's standard offset, andjava.timereads the instant back with the zone's full history, which is local mean time before 1883. In America/Los_Angeles a stored1600-06-15 08:30:00came back as08:37:02.The field is now stored as
ts.toLocalDateTime, the wall clock the Timestamp holds.fromRecordalready reads it back withTimestamp.valueOf, which is its inverse, so a value comes out of result storage as it went in. Timestamps after 1883 are unchanged, since both rules agree there.Any related issues, documentation, discussions?
Closes #9074.
How was this PR tested?
A test in
IcebergUtilSpecsets the default zone to America/Los_Angeles, stores1600-06-15 08:30:00and1677-09-21 00:00:00, and checks that the record holds each wall clock and reads back to the same tuple. The zone is set in the test because UTC has no local mean time, so the old conversion passes there. On the old code the test fails with1600-06-15T08:37:02 did not equal 1600-06-15T08:30. All 24 tests in the spec pass.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5.5)
🤖 Generated with Claude Code