Yicong-Huang commented on code in PR #8258:
URL: https://github.com/apache/texera/pull/8258#discussion_r4058859013
##########
amber/src/main/python/core/architecture/sendsemantics/range_based_shuffle_partitioner.py:
##########
@@ -51,6 +52,8 @@ def __init__(self, partitioning:
RangeBasedShufflePartitioning):
)
def get_receiver_index(self, column_val) -> int:
+ if isinstance(column_val, float) and math.isnan(column_val):
+ column_val = 0
Review Comment:
**Must fix:**
The root cause in #8257 is not NaN, it is that this method never narrows the
double at all. NaN was the loud symptom because it raised. The quiet one is on
the next line and survives this fix. `int((column_val - self.range_min) //
self.keys_per_receiver)` floor-divides a float, while the JVM truncates toward
zero first (`RangeBasedShufflePartitioner.scala:43`, `.toLong`) and then
divides integers. Under this PR's own test config (range -10 to 9, two
receivers), key `-0.5` routes to worker A here and worker B on the JVM. I
executed both formulas. A 0.1-step sweep finds 9 diverging keys for that
config, and 18 for a [-50, 49] range over five receivers, all just below a
bucket boundary. The replacement below matched the JVM over 4207 values, and
its `isfinite` guard is load-bearing, since `int(float('inf'))` raises
`OverflowError`.
```suggestion
if isinstance(column_val, float):
# Mirror the JVM partitioner's Double -> Long narrowing in
# RangeBasedShufflePartitioner.scala: NaN becomes 0 and a finite
# value truncates toward zero. Infinities stay floats, since the
# comparisons below already saturate them and int() would raise
# OverflowError.
if math.isnan(column_val):
column_val = 0
elif math.isfinite(column_val):
column_val = int(column_val)
```
##########
amber/src/main/python/core/architecture/sendsemantics/range_based_shuffle_partitioner.py:
##########
@@ -51,6 +52,8 @@ def __init__(self, partitioning:
RangeBasedShufflePartitioning):
)
def get_receiver_index(self, column_val) -> int:
Review 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?
##########
amber/src/test/python/core/architecture/sendsemantics/test_partitioners.py:
##########
@@ -331,6 +331,19 @@ def test_value_in_range_routes_by_quotient(self,
partitioner):
assert partitioner.get_receiver_index(8) == 2
assert partitioner.get_receiver_index(9) == 2
+ def test_nan_key_routes_like_scala_zero(self):
Review Comment:
**Advisory:**
Not a claim that `feat` is wrong: AGENTS.md maps support that never existed
to `feat`, Python NaN keys never worked, and you retitled from `fix(`
deliberately. The unstated part is the consequence for backports.
`backport-auto-label.yml:110` matches only titles starting `fix(...)`, so a
`feat` title takes this out of the auto-backport path. Meanwhile
`get_receiver_index` on both `release/v1.2` and `release/v1.3` still has no NaN
guard (I fetched and read both). Worth one sentence either way.
##########
amber/src/test/python/core/architecture/sendsemantics/test_partitioners.py:
##########
@@ -331,6 +331,19 @@ def test_value_in_range_routes_by_quotient(self,
partitioner):
assert partitioner.get_receiver_index(8) == 2
assert partitioner.get_receiver_index(9) == 2
+ def test_nan_key_routes_like_scala_zero(self):
Review Comment:
**Advisory:**
The description's claim that infinite-key behavior is unchanged holds today,
verified by reading. `isnan` does not fire on an infinity, and the existing `<
range_min` and `> range_max` comparisons saturate them to the first and last
receiver. It is worth pinning, because the natural completion of this fix adds
an `int()` conversion, whose naive form raises `OverflowError` on an infinity.
Two assertions on this same partitioner would turn the claim into something the
suite defends.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
