Skip to content

feat(pyamber): preserve minus-one shuffle hashes - #8254

Open
carloea2 wants to merge 1 commit into
apache:mainfrom
carloea2:fix/preserve-minus-one-shuffle-hash
Open

feat(pyamber): preserve minus-one shuffle hashes#8254
carloea2 wants to merge 1 commit into
apache:mainfrom
carloea2:fix/preserve-minus-one-shuffle-hash

Conversation

@carloea2

@carloea2 carloea2 commented Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Hash shuffle now uses the tuple's raw Java-compatible hash method directly, avoiding Python's reserved -1 to -2 normalization before receiver selection.

Any related issues, documentation, discussions?

Closes #8253

How was this PR tested?

Retested with Python 3.12 in a fresh detached worktree from upstream main at e9e5fda4bb: 38 partitioner tests pass. The routing override now also explains why it calls the raw hash method.

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

python -m pytest 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 218 Python files.

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

Generated-by: Codex

@github-actions github-actions Bot added the fix label Aug 31, 2026
@github-actions

github-actions Bot commented Aug 31, 2026 •
edited
Loading

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: @Ma77Ball
    You can notify them by mentioning @Ma77Ball in a comment.

@Yicong-Huang Yicong-Huang added the release/v1.2 back porting to release/v1.2 label Aug 31, 2026
@github-actions
github-actions Bot requested a review from xuang7 August 31, 2026 05:22
@github-actions

github-actions Bot commented Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Backport auto-label report

This fix: PR was checked against each actively-supported release branch. release/* labels drive the post-merge backport, so add or remove one to change where this fix lands.

Release branch Analysis
⚠️ release/v1.3 Not labeled automatically — none of the files this PR modifies exist on this branch (amber/src/main/python/core/architecture/sendsemantics/hash_based_shuffle_partitioner.py, amber/src/test/python/core/architecture/sendsemantics/test_partitioners.py). The fix may target code that isn't on this release, or the files were moved/renamed after the branch was cut. Please check and add release/v1.3 by hand if this fix should be backported here.
✅ release/v1.2 Already labeled — this fix is queued to backport here.

Auto-label run.

@codecov-commenter

codecov-commenter commented Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.81%. Comparing base (e9e5fda) to head (4869cf2).

Additional details and impacted files 
@@            Coverage Diff            @@
##               main    #8254   +/-   ##
=========================================
  Coverage     91.81%   91.81%           
  Complexity     5021     5021           
=========================================
  Files          1277     1277           
  Lines         56381    56381           
  Branches       7362     7362           
=========================================
  Hits          51765    51765           
  Misses         2762     2762           
  Partials       1854     1854
Flag Coverage Δ *Carryforward flag
access-control-service 77.44% <ø> (ø) Carriedforward from e9e5fda
agent-service 99.53% <ø> (ø) Carriedforward from e9e5fda
amber 85.77% <ø> (ø) Carriedforward from e9e5fda
computing-unit-managing-service 64.05% <ø> (ø) Carriedforward from e9e5fda
config-service 87.73% <ø> (ø) Carriedforward from e9e5fda
file-service 82.31% <ø> (ø) Carriedforward from e9e5fda
frontend 96.54% <ø> (ø) Carriedforward from e9e5fda
notebook-migration-service 83.77% <ø> (ø) Carriedforward from e9e5fda
pyamber 98.50% <100.00%> (ø)
workflow-compiling-service 78.30% <ø> (ø) Carriedforward from e9e5fda

*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 Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

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

Compared against main e9e5fda 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 539 0.329 18,056/24,104/24,104 us 🔴 +13.7% / 🔴 +69.9%
🔴 bs=100 sw=10 sl=64 1,243 0.759 79,054/109,856/109,856 us 🔴 +6.0% / 🔴 +14.0%
🔴 bs=1000 sw=10 sl=64 1,461 0.892 674,379/832,234/832,234 us 🔴 +15.6% / 🟢 +22.9%
Baseline details

Latest main e9e5fda from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 539 tuples/sec 585 tuples/sec 880.16 tuples/sec -7.9% -38.8%
bs=10 sw=10 sl=64 MB/s 0.329 MB/s 0.357 MB/s 0.537 MB/s -7.8% -38.8%
bs=10 sw=10 sl=64 p50 18,056 us 15,877 us 11,544 us +13.7% +56.4%
bs=10 sw=10 sl=64 p95 24,104 us 27,205 us 14,186 us -11.4% +69.9%
bs=10 sw=10 sl=64 p99 24,104 us 27,205 us 17,122 us -11.4% +40.8%
bs=100 sw=10 sl=64 throughput 1,243 tuples/sec 1,238 tuples/sec 1,151 tuples/sec +0.4% +8.0%
bs=100 sw=10 sl=64 MB/s 0.759 MB/s 0.756 MB/s 0.703 MB/s +0.4% +8.0%
bs=100 sw=10 sl=64 p50 79,054 us 80,205 us 89,900 us -1.4% -12.1%
bs=100 sw=10 sl=64 p95 109,856 us 103,665 us 96,346 us +6.0% +14.0%
bs=100 sw=10 sl=64 p99 109,856 us 103,665 us 110,344 us +6.0% -0.4%
bs=1000 sw=10 sl=64 throughput 1,461 tuples/sec 1,477 tuples/sec 1,189 tuples/sec -1.1% +22.9%
bs=1000 sw=10 sl=64 MB/s 0.892 MB/s 0.901 MB/s 0.726 MB/s -1.0% +22.9%
bs=1000 sw=10 sl=64 p50 674,379 us 682,686 us 873,675 us -1.2% -22.8%
bs=1000 sw=10 sl=64 p95 832,234 us 719,634 us 918,440 us +15.6% -9.4%
bs=1000 sw=10 sl=64 p99 832,234 us 719,634 us 953,075 us +15.6% -12.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,370.91,200,128000,539,0.329,18055.81,24103.54,24103.54
1,100,10,64,20,1608.73,2000,1280000,1243,0.759,79053.88,109855.99,109855.99
2,1000,10,64,20,13690.21,20000,12800000,1461,0.892,674378.64,832233.63,832233.63

@Yicong-Huang

Yicong-Huang commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

I am converting this PR to a draft: please follow our PR template and mark it ready for review.

@Yicong-Huang
Yicong-Huang marked this pull request as draft August 31, 2026 20:06
@carloea2
carloea2 marked this pull request as ready for review August 31, 2026 20:18
@carloea2

carloea2 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

The description now follows the current template, and this is ready for review.

@carloea2 carloea2 changed the title fix(pyamber): preserve minus-one shuffle hashes feat(pyamber): preserve minus-one shuffle hashes Aug 31, 2026
@mengw15 mengw15 removed the release/v1.2 back porting to release/v1.2 label Aug 31, 2026
@xuang7 xuang7 removed the fix label Sep 1, 2026
@xuang7
xuang7 removed their request for review September 1, 2026 01:44
@carloea2
carloea2 force-pushed the fix/preserve-minus-one-shuffle-hash branch from 02bcf80 to 99fd075 Compare October 15, 2026 06:44
@github-actions github-actions Bot added the fix label Oct 15, 2026
@carloea2 carloea2 mentioned this pull request Oct 15, 2026
Merged
@carloea2

carloea2 commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

/request-review @sshiv012 @parshimers

eugenegujing
eugenegujing approved these changes Nov 10, 2026
View reviewed changes

@eugenegujing eugenegujing 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.

LGTM!

@carloea2
carloea2 force-pushed the fix/preserve-minus-one-shuffle-hash branch from 99fd075 to 4869cf2 Compare November 10, 2026 21:27
@carloea2

carloea2 commented Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

@mengw15, CI is green and Eugene's comment is addressed. Could you review the latest head and merge? Please also switch the fix label to feature.

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

Reviewers

1 more reviewer
@eugenegujing eugenegujing eugenegujing approved these changes
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@carloea2 carloea2

Labels

fix pyamber

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Python hash normalization can change shuffle receivers

6 participants

@carloea2 @codecov-commenter @Yicong-Huang @eugenegujing @mengw15 @xuang7