Repository navigation
Conversation
Backport auto-label reportThis
|
Automated Reviewer SuggestionsBased on the
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 376 | 0.229 | 26,244/34,971/34,971 us | 🔴 -9.8% / 🔴 +126.1% |
| 🔴 | bs=100 sw=10 sl=64 | 777 | 0.474 | 127,365/155,030/155,030 us | 🔴 +12.0% / 🔴 +48.8% |
| 🔴 | bs=1000 sw=10 sl=64 | 908 | 0.554 | 1,095,506/1,253,655/1,253,655 us | 🔴 +6.9% / 🔴 +27.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 | 376 tuples/sec | 417 tuples/sec | 822.11 tuples/sec | -9.8% | -54.3% |
| bs=10 sw=10 sl=64 | MB/s | 0.229 MB/s | 0.254 MB/s | 0.502 MB/s | -9.8% | -54.4% |
| bs=10 sw=10 sl=64 | p50 | 26,244 us | 23,959 us | 12,129 us | +9.5% | +116.4% |
| bs=10 sw=10 sl=64 | p95 | 34,971 us | 34,649 us | 15,469 us | +0.9% | +126.1% |
| bs=10 sw=10 sl=64 | p99 | 34,971 us | 34,649 us | 18,357 us | +0.9% | +90.5% |
| bs=100 sw=10 sl=64 | throughput | 777 tuples/sec | 816 tuples/sec | 1,058 tuples/sec | -4.8% | -26.5% |
| bs=100 sw=10 sl=64 | MB/s | 0.474 MB/s | 0.498 MB/s | 0.646 MB/s | -4.8% | -26.6% |
| bs=100 sw=10 sl=64 | p50 | 127,365 us | 122,571 us | 97,399 us | +3.9% | +30.8% |
| bs=100 sw=10 sl=64 | p95 | 155,030 us | 138,468 us | 104,184 us | +12.0% | +48.8% |
| bs=100 sw=10 sl=64 | p99 | 155,030 us | 138,468 us | 117,615 us | +12.0% | +31.8% |
| bs=1000 sw=10 sl=64 | throughput | 908 tuples/sec | 921 tuples/sec | 1,099 tuples/sec | -1.4% | -17.4% |
| bs=1000 sw=10 sl=64 | MB/s | 0.554 MB/s | 0.562 MB/s | 0.671 MB/s | -1.4% | -17.4% |
| bs=1000 sw=10 sl=64 | p50 | 1,095,506 us | 1,083,209 us | 946,743 us | +1.1% | +15.7% |
| bs=1000 sw=10 sl=64 | p95 | 1,253,655 us | 1,173,147 us | 985,437 us | +6.9% | +27.2% |
| bs=1000 sw=10 sl=64 | p99 | 1,253,655 us | 1,173,147 us | 1,012,622 us | +6.9% | +23.8% |
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,532.29,200,128000,376,0.229,26244.32,34971.40,34971.40
1,100,10,64,20,2573.75,2000,1280000,777,0.474,127364.93,155029.65,155029.65
2,1000,10,64,20,22016.70,20000,12800000,908,0.554,1095505.80,1253655.32,1253655.32
|
I am converting this PR to a draft: please follow our PR template and mark it ready for review. |
|
The description now follows the current template, and this is ready for review. |
carloea2
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8258 +/- ##
============================================
- Coverage 92.30% 92.30% -0.01%
Complexity 5046 5046
============================================
Files 1260 1260
Lines 53887 53893 +6
Branches 6711 6714 +3
============================================
+ Hits 49743 49746 +3
- Misses 2486 2488 +2
- Partials 1658 1659 +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:
|
|
/request-review @Yicong-Huang |
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🔴 1 must-fix · 3 advisory · 0 polish: the NaN fix is correct and well tested, but it patches one cell of a conversion that is missing entirely.
Correctness (2)
range_based_shuffle_partitioner.py:55: no double to long narrowing, so fractional keys diverge from the JVM (must-fix, see inline)range_based_shuffle_partitioner.py:54: bucket width divides by the raw channel list, not the de-duplicated receivers (advisory, question, see inline)
Simplifications (1)
test_partitioners.py:334: nothing pins the description's infinite-key claim (advisory, see inline)
Conventions (1)
test_partitioners.py:334:featalso takes this out of the auto-backport path (advisory, question, see inline)
Verification trace
Checked the parity claim against the JVM twin rather than reading it off. RangeBasedShufflePartitioner.scala:43 narrows the key with .toLong and then divides integers, while the Python side divides the raw float. Executing both under this PR's own test config (range -10 to 9, two receivers), key -0.5 gives index 0 here and 1 on the JVM. A 0.1-step sweep found 9 diverging keys for that config and 18 for a [-50, 49] range over five receivers, all just below a bucket boundary in a range spanning negatives. Positive fractional keys agree, which is why this survived until a NaN made it raise. The infinite-key claim in the description does hold.
| @@ -51,6 +52,8 @@ def __init__(self, partitioning: RangeBasedShufflePartitioning): | |||
| ) | |||
|
|
|||
| def get_receiver_index(self, column_val) -> int: | |||
There was a problem hiding this comment.
Advisory:
A question about keys_per_receiver, which is computed at :46 outside this diff, so the comment sits on the method it feeds. Python divides the range by len(partitioning.channels), the raw channel list, while the JVM divides by receivers.length after .distinct, and Python's own build_receiver_batches de-duplicates as well. The two divisors genuinely differ. I could not construct a partitioner instance holding two channels with the same to_worker_id, so I am asking rather than asserting: is that shape reachable?
There was a problem hiding this comment.
I reproduced this with channels S1 to A, S2 to A, and S1 to B: two receivers, but key 9 produces index 2. #8246 covers the divisor change separately. I have not demonstrated that topology through the planner.
There was a problem hiding this comment.
I traced ResourceAllocator to LinkConfig and executed a 2 by 2 range configuration (-10 to 9). It produces four channels, which OutputManager passes to the partitioner. Key 9 raises IndexError in Python; the JVM chooses bucket 1. Single-sender controls pass. Keeping the divisor fix in #8246.
7c8830d to
df2c526
Compare
carloea2
changed the title
|
/request-review @Yicong-Huang |
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🟢 3 resolved · 1 open · 0 new (0 new = 0 newly introduced · 0 late catches)
Clean round: the round-1 must-fix (JVM-style finite-double narrowing) and both round-1 advisories (Infinity test pinning, backport note) are all verified fixed in the tree. The one open thread is the keys_per_receiver divisor question, which the author answered and deferred to #8246, so it stays open.
The three verified-fixed threads have been marked resolved.
Verification trace
Confirmed the new int() narrowing matches JVM Double.toLong on the discriminating inputs (e.g. -0.5 now routes to B on both sides), infinite keys saturate to the endpoint receivers as on the JVM, and the description's backport statement matches the backport-auto-label.yml rule.
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🚫 Merge conflict with main — no review was performed on this revision.
This PR cannot merge in its current state, so its diff is not the change that would land. Please merge or rebase onto main, resolve the conflict, and re-request review.
df2c526 to
d955f14
Compare
|
/request-review @Yicong-Huang Rebased onto current main and preserved the shared partitioner refactor. All 43 partitioner tests and Ruff checks pass. The receiver-divisor advisory remains separate in #8246. |
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🟢 0 resolved · 1 open · 0 new (0 new = 0 newly introduced · 0 late catches)
Clean round: the rebase preserves the reviewed fix and its tests exactly. The one open thread is the keys_per_receiver divisor question, deferred to #8246.
Verification trace
Verified the rebase preserved the fix: the PR diff vs the new base holds only the narrowing and the six parametrized tests, and the shared-base add_tuple_to_batch/flush/flush_state match the old inlined bodies. Also re-checked the description's test counts at the rebased head (43 cases; 2 failed / 41 passed before the fix).
What changes were proposed in this PR?
Align Python range-key conversion with the JVM: NaN becomes zero and finite doubles truncate toward zero. Infinite values still route to the endpoint receivers.
This adds previously missing support, so the type remains feat. No release backport is requested.
Any related issues, documentation, discussions?
Closes #8257
The finite-key change overlaps #8256. Duplicate receiver counting remains separately covered by #8246.
How was this PR tested?
Rebased onto upstream main at
7426431337, preserving the shared indexed-shuffle partitioner. The regressions fail before the implementation change: 2 failed and 41 passed. After the change, all 43 partitioner tests pass. NaN, finite fractions and both infinities are covered.Generate protobufs with
bash bin/python-proto-gen.sh, then run fromamber:Ruff lint and formatting pass for all 217 Python files.
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Codex