Skip to content

feat(pyamber): hash epoch timestamps on Windows - #8957

Open
carloea2 wants to merge 1 commit into
apache:mainfrom
carloea2:codex/feature-windows-epoch-hash
Open

carloea2 wants to merge 1 commit into
apache:mainfrom
carloea2:codex/feature-windows-epoch-hash

Conversation

@carloea2

@carloea2 carloea2 commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Support naive local epoch timestamps in Python tuple hashing on Windows. Attach the local timezone before computing epoch seconds; aware timestamps keep their existing conversion.

Any related issues, documentation, discussions?

Closes #8211

Replaces #8212, which the stale bot closed. The original PR cannot be reopened by this account.

How was this PR tested?

Retested with Python 3.12 on Windows in a fresh detached worktree from upstream main at 7426431337, which includes the merged loop-completion change #8938. Regression before implementation: 1 failed. After implementation: 356 passed, 1 expected failure; no deselections.

The existing negative-epoch reference fixture now uses UTC arithmetic so Windows can construct it. Its expected Java hash values are unchanged.

Generate protobufs with bash bin/python-proto-gen.sh, then run from amber:

python -m pytest src/test/python/core/models src/test/python/core/architecture/sendsemantics/test_partitioners.py -q --tb=line -p no:cacheprovider
python -m ruff check src/main/python src/test/python
python -m ruff format --check src/main/python src/test/python

Ruff lint and formatting pass for all 217 Python files.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Codex (GPT-6)

@carloea2

Copy link
Copy Markdown
Contributor Author

This replaces stale-closed #8212. The problem still reproduces on current main. The patch passes its regression, 349 broader tests and Ruff checks.

/request-review @aglinxinyuan

@carloea2

Copy link
Copy Markdown
Contributor Author

/request-review @aglinxinyuan

@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @eugenegujing, @aglinxinyuan
    You can notify them by mentioning @eugenegujing, @aglinxinyuan in a comment.

@codecov-commenter

codecov-commenter commented Oct 12, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.30%. Comparing base (7426431) to head (9aba936).

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #8957   +/-   ##
=========================================
  Coverage     92.30%   92.30%           
  Complexity     5046     5046           
=========================================
  Files          1260     1260           
  Lines         53887    53888    +1     
  Branches       6711     6711           
=========================================
+ Hits          49743    49744    +1     
  Misses         2486     2486           
  Partials       1658     1658           
Flag Coverage Δ *Carryforward flag
access-control-service 77.38% <ø> (ø) Carriedforward from 7426431
agent-service 99.16% <ø> (ø) Carriedforward from 7426431
amber 87.32% <ø> (ø) Carriedforward from 7426431
computing-unit-managing-service 60.48% <ø> (ø) Carriedforward from 7426431
config-service 87.37% <ø> (ø) Carriedforward from 7426431
file-service 82.00% <ø> (ø) Carriedforward from 7426431
frontend 96.55% <ø> (ø) Carriedforward from 7426431
notebook-migration-service 83.73% <ø> (ø) Carriedforward from 7426431
pyamber 98.53% <100.00%> (+<0.01%) ⬆️
workflow-compiling-service 72.43% <ø> (ø) Carriedforward from 7426431

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

🟢 0 better · 🔴 5 worse · ⚪ 10 noise (<±5%) · 0 without baseline

Compared against main 7426431 benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

config throughput MB/s latency max Δ latest / 7d
🔴 bs=10 sw=10 sl=64 396 0.242 24,640/34,217/34,217 us 🔴 +13.5% / 🔴 +121.2%
🔴 bs=100 sw=10 sl=64 794 0.485 123,854/165,870/165,870 us 🔴 +10.5% / 🔴 +59.2%
⚪ bs=1000 sw=10 sl=64 899 0.549 1,114,875/1,161,930/1,161,930 us ⚪ within ±5% / 🔴 -18.2%
Baseline details

Latest main 7426431 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 396 tuples/sec 431 tuples/sec 822.11 tuples/sec -8.1% -51.8%
bs=10 sw=10 sl=64 MB/s 0.242 MB/s 0.263 MB/s 0.502 MB/s -8.0% -51.8%
bs=10 sw=10 sl=64 p50 24,640 us 21,710 us 12,129 us +13.5% +103.2%
bs=10 sw=10 sl=64 p95 34,217 us 34,883 us 15,469 us -1.9% +121.2%
bs=10 sw=10 sl=64 p99 34,217 us 34,883 us 18,357 us -1.9% +86.4%
bs=100 sw=10 sl=64 throughput 794 tuples/sec 814 tuples/sec 1,058 tuples/sec -2.5% -24.9%
bs=100 sw=10 sl=64 MB/s 0.485 MB/s 0.497 MB/s 0.646 MB/s -2.4% -24.9%
bs=100 sw=10 sl=64 p50 123,854 us 120,672 us 97,399 us +2.6% +27.2%
bs=100 sw=10 sl=64 p95 165,870 us 150,071 us 104,184 us +10.5% +59.2%
bs=100 sw=10 sl=64 p99 165,870 us 150,071 us 117,615 us +10.5% +41.0%
bs=1000 sw=10 sl=64 throughput 899 tuples/sec 916 tuples/sec 1,099 tuples/sec -1.9% -18.2%
bs=1000 sw=10 sl=64 MB/s 0.549 MB/s 0.559 MB/s 0.671 MB/s -1.8% -18.1%
bs=1000 sw=10 sl=64 p50 1,114,875 us 1,098,490 us 946,743 us +1.5% +17.8%
bs=1000 sw=10 sl=64 p95 1,161,930 us 1,120,446 us 985,437 us +3.7% +17.9%
bs=1000 sw=10 sl=64 p99 1,161,930 us 1,120,446 us 1,012,622 us +3.7% +14.7%
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,505.17,200,128000,396,0.242,24640.22,34217.37,34217.37
1,100,10,64,20,2517.96,2000,1280000,794,0.485,123853.62,165870.10,165870.10
2,1000,10,64,20,22243.55,20000,12800000,899,0.549,1114874.58,1161930.40,1161930.40

@aglinxinyuan

Copy link
Copy Markdown
Contributor

What might be the problem in the current main if we don't have this?

@carloea2

Copy link
Copy Markdown
Contributor Author

On Windows, hashing a valid epoch timestamp can raise OSError: Invalid argument and stop hash routing. I reproduced it today: datetime.fromtimestamp(0) produces 1969-12-31 18:00 locally, and the naive timestamp() call fails. The new regression passes, along with all 356 broader tests. Python CI passes on 3.11, 3.12, and 3.13. I am checking the separate Ubuntu loop failure, which reports S3 GetObject AccessDenied.

@carloea2

Copy link
Copy Markdown
Contributor Author

I reproduced the storage failure against RustFS rc.6 and Lakekeeper 0.11.0. A controlled read during PyIceberg's signer replacement gets 403; the same read succeeds once signing resumes. Separate filesystem instances also pass.

This matches the upstream signer race. Its proposed fix, #3783, is still open.

The full local REST-backed loop run finished with 7 passing tests and 1 canceled as flaky after a retry, not a clean eight-test pass. The JDBC-backed run passed all 8 without cancellation.

@aglinxinyuan, could you rerun the failed Ubuntu job? This account cannot rerun it. I have kept the timestamp change separate from this shared storage problem.

This branch has not been deployed

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tuple hashing fails for local epoch timestamps on Windows

3 participants