gemini-code-assist[bot] commented on code in PR #39047:
URL: https://github.com/apache/beam/pull/39047#discussion_r3487371190
##########
sdks/python/apache_beam/coders/coders_test_common.py:
##########
@@ -437,6 +438,25 @@ 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 past 64 bits stay out of range (only the Cython stream enforces
it).
+ if coder_impl.is_compiled:
+ for v in [1 << 64, (1 << 70)]:
+ with self.assertRaises(OverflowError):
+ coder.encode(v)
Review Comment:
Once the 64-bit range check is enforced in `_as_signed_int64` for both
compiled and uncompiled paths, we can remove the `if coder_impl.is_compiled:`
guard. This ensures that the overflow behavior is tested and verified across
both implementations.
```python
# Values past 64 bits stay out of range.
for v in [1 << 64, (1 << 70)]:
with self.assertRaises(OverflowError):
coder.encode(v)
```
##########
sdks/python/apache_beam/coders/coder_impl.py:
##########
@@ -90,6 +90,18 @@
is_compiled = False
fits_in_64_bits = lambda x: -(1 << 63) <= x <= (1 << 63) - 1
+
+def _as_signed_int64(value):
+ # type: (int) -> int
+
+ """Folds a uint64 to the signed int64 with the same bits (and VarInt
+ encoding), which the Cython int64_t stream params accept. Larger values pass
+ through and still overflow downstream."""
+ if (1 << 63) <= value < (1 << 64):
+ return int(value) - (1 << 64)
+ return value
Review Comment:
To ensure consistent behavior between the compiled (Cython) and uncompiled
(pure-Python) implementations, we should enforce the 64-bit integer range limit
in the pure-Python path as well. Currently, the uncompiled path silently allows
values outside the 64-bit range (e.g., `>= 2**64` or `< -2**63`), which can
lead to hard-to-debug discrepancies between local development and production
environments.
By raising an `OverflowError` in `_as_signed_int64` for out-of-range values,
both paths will consistently raise the same descriptive `OverflowError` (since
`encode_to_stream` and `estimate_size` already catch and wrap `OverflowError`).
```python
def _as_signed_int64(value):
# type: (int) -> int
"""Folds a uint64 to the signed int64 with the same bits (and VarInt
encoding), which the Cython int64_t stream params accept. Larger values
pass
through and still overflow downstream."""
if (1 << 63) <= value < (1 << 64):
return int(value) - (1 << 64)
if not (-(1 << 63) <= value < (1 << 63)):
raise OverflowError(
f"Value {value} is out of the 64-bit integer range.")
return value
```
--
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]
