gemini-code-assist[bot] commented on code in PR #39218:
URL: https://github.com/apache/beam/pull/39218#discussion_r3529967711
##########
sdks/python/apache_beam/coders/coder_impl.py:
##########
@@ -1058,26 +1044,22 @@ class VarIntCoderImpl(StreamCoderImpl):
def encode_to_stream(self, value, out, nested):
# type: (int, create_OutputStream, bool) -> None
try:
- # Fold uint64 values into signed int64 so Cython doesn't overflow.
- out.write_var_int64(_as_signed_int64(value))
+ out.write_var_int64(value)
except OverflowError as e:
raise OverflowError(
f"Integer value '{value}' is out of the encodable range for "
- f"VarIntCoder. This coder is limited to 64-bit integers: the "
- f"signed range -(2**63) to 2**63 - 1, plus unsigned values up to "
- f"2**64 - 1 which share the same wire encoding. "
+ f"VarIntCoder. This coder is limited to values that fit "
+ f"within a 64-bit signed integer (-(2**63) to 2**63 - 1). "
f"Original error: {e}") from e
def decode_from_stream(self, in_stream, nested):
# type: (create_InputStream, bool) -> int
return in_stream.read_var_int64()
def encode(self, value):
- # Compare as a Python object: a uint64 value overflows the int64_t cast
- # the compiled fast path used to do here. Non-small values (including
- # uint64) fall through to encode_to_stream, which folds them.
- if 0 <= value < len(small_ints):
- return small_ints[value]
+ ivalue = value # type cast
+ if 0 <= ivalue < len(small_ints):
+ return small_ints[ivalue]
return StreamCoderImpl.encode(self, value)
Review Comment:
In Cython compiled mode, `ivalue` is declared as `int64_t` in `.pxd`. The
assignment `ivalue = value` performs a type cast. If `value` is an integer that
overflows the 64-bit signed integer range, this assignment will raise an
`OverflowError` immediately.
This bypasses the fallback to `StreamCoderImpl.encode(self, value)` and the
custom `OverflowError` handling in `encode_to_stream` (lines 1048-1053), which
is designed to provide a helpful error message explaining the 64-bit signed
integer limit. Instead, the user receives a generic Cython `OverflowError`.
Wrapping the type cast and small integer check in a `try...except
OverflowError` block allows the overflow to be caught and safely delegated to
`StreamCoderImpl.encode`, which will then raise the descriptive error message.
```python
def encode(self, value):
try:
ivalue = value # type cast
if 0 <= ivalue < len(small_ints):
return small_ints[ivalue]
except OverflowError:
pass
return StreamCoderImpl.encode(self, value)
```
##########
sdks/python/apache_beam/coders/coders_test_common.py:
##########
@@ -437,24 +437,6 @@ def test_varint_coder(self):
for k in range(0, int(math.log(MAX_64_BIT_INT)))
])
- def test_varint_coder_uint64(self):
- # uint64 values [2**63, 2**64) must encode like the signed int64 with the
- # same bits instead of overflowing Cython's int64_t. Decoding is signed,
- # matching Java's VarIntCoder.
- coder = coders.VarIntCoder()
- impl = coder.get_impl()
- for v in [1 << 63, (1 << 63) + 12345, (1 << 64) - 1]:
- signed_twin = v - (1 << 64)
- encoded = coder.encode(v)
- self.assertEqual(encoded, coder.encode(signed_twin))
- self.assertEqual(impl.estimate_size(v), len(encoded))
- self.assertEqual(coder.decode(encoded), signed_twin)
-
- # Values outside the 64-bit range stay out of range on both paths.
- for v in [1 << 64, (1 << 70), -(1 << 63) - 1]:
- with self.assertRaises(OverflowError):
- coder.encode(v)
-
def test_varint32_coder(self):
Review Comment:
Since `test_varint_coder_uint64` is being removed, we should ensure that we
still have test coverage verifying that values outside the supported 64-bit
signed integer range (such as `1 << 63`, `1 << 64`, and `-(1 << 63) - 1`)
correctly raise `OverflowError`.
```suggestion
def test_varint_coder_overflow(self):
coder = coders.VarIntCoder()
for v in [1 << 63, 1 << 64, -(1 << 63) - 1]:
with self.assertRaises(OverflowError):
coder.encode(v)
def test_varint32_coder(self):
```
--
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]
