You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "AviKndr (via GitHub)" <gi...@apache.org> on 2026/06/20 23:03:30 UTC
[PR] [BEAM] Fix Python VarIntCoder OverflowError on uint64 values [beam]
AviKndr opened a new pull request, #39047:
URL: https://github.com/apache/beam/pull/39047
### What
`VarIntCoder` raises `OverflowError` when handed a Python int in the unsigned 64-bit range `[2**63, 2**64)` (a uint64), even though such a value has a well-defined VarInt encoding.
### Why
In the Cython build, the stream methods take a **signed** `int64_t`:
```cython
cpdef write_var_int64(self, libc.stdint.int64_t signed_v):
cdef libc.stdint.uint64_t v = signed_v # body already works on unsigned
```
The body re-casts to `uint64_t` and encodes the bit pattern correctly, but Cython converts the incoming Python int to the signed `int64_t` parameter *at the call boundary* — before the body runs — so any uint64 value is rejected with `OverflowError`. The same applies to `get_varint_size` used by `estimate_size`.
### Fix
A uint64 value `v` and the signed int64 `v - 2**64` (its two's-complement twin) produce **identical** VarInt bytes. `VarIntCoderImpl` now folds uint64 values to that signed twin before they cross into Cython, in both `encode_to_stream` and `estimate_size`:
```python
def _as_signed_int64(value):
if (1 << 63) <= value < (1 << 64):
return int(value) - (1 << 64)
return value
```
- **Wire-compatible** — byte-identical to Java's signed `VarIntCoder`.
- **No regression** — normal signed ints pass through untouched; the pure-Python `slow_stream` path produces the same bytes.
- **Still bounded** — values `>= 2**64` are left unchanged and still overflow in the Cython path, preserving the coder's 64-bit contract.
Note: decoding remains **signed** (`read_var_int64` returns `int64_t`), matching Java — so the encoding of `2**64 - 1` decodes back to `-1`. This change removes the crash and guarantees correct wire bytes; it does not make the coder round-trip uint64 back to unsigned (that would be a separate coder).
### Tests
Adds `test_varint_coder_uint64` to the shared `coders_test_common.py` suite (runs both compiled and uncompiled): no-overflow encoding, wire equivalence to the signed twin, size estimation, signed decode semantics, and the still-raising out-of-range case (guarded on the compiled implementation).
------------------------
Thank you for your contribution! Follow [this checklist](https://github.com/apache/beam/blob/master/CONTRIBUTING.md) to help us incorporate your contribution quickly and easily:
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [BEAM] Fix Python VarIntCoder OverflowError on uint64 values [beam]
Posted by "gemini-code-assist[bot] (via GitHub)" <gi...@apache.org>.
gemini-code-assist[bot] commented on PR #39047:
URL: https://github.com/apache/beam/pull/39047#issuecomment-4824856699
## Summary of Changes
Hello, I'm Gemini Code Assist[^1]! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request addresses an issue where the Python VarIntCoder incorrectly raised an OverflowError for unsigned 64-bit integers. By normalizing these values to their signed 64-bit counterparts before they reach the Cython implementation, the coder now correctly handles the full 64-bit range while maintaining compatibility with existing wire formats and Java implementations.
### Highlights
* **VarIntCoder Overflow Fix**: Implemented a helper function to fold unsigned 64-bit integers into their signed 64-bit two's-complement equivalents before passing them to Cython, preventing unnecessary OverflowErrors.
* **Wire Compatibility**: Ensured that the encoding remains wire-compatible with Java's VarIntCoder, maintaining consistent byte representation for uint64 values.
* **Test Coverage**: Added comprehensive tests in coders_test_common.py to verify uint64 encoding, size estimation, and signed decoding semantics.
<details>
<summary><b>New Features</b></summary>
🧠 You can now enable **Memory** (public preview) to help **Gemini Code Assist** learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. **Click [here](https://codeassist.google/code-review/login) to enable Memory in your admin console.**
</details>
<details>
<summary><b>Using Gemini Code Assist</b></summary>
<br>
The full guide for Gemini Code Assist can be found on our [documentation page](https://developers.google.com/gemini-code-assist/docs/review-github-code), here are some quick tips.
<b>Invoking Gemini</b>
You can request assistance from Gemini at any point by creating a comment using either `/gemini <command>` or `@gemini-code-assist <command>`. Below is a summary of the supported commands on the current page.
Feature | Command | Description
--- | --- | ---
Code Review | `/gemini review` | Performs a code review for the current pull request in its current state.
Pull Request Summary | `/gemini summary` | Provides a summary of the current pull request in its current state.
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help | `/gemini help` | Displays a list of available commands.
<b>Customization</b>
To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a `.gemini/` folder in the base of the repository. Detailed instructions can be found [here](https://developers.google.com/gemini-code-assist/docs/customize-gemini-behavior-github).
<b>Limitations & Feedback</b>
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with :thumbsup: and :thumbsdown: on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up [here](https://google.qualtrics.com/jfe/form/SV_2cyuGuTWsEw84yG).
</details>
[^1]: Review the [Privacy Notices](https://policies.google.com/privacy), [Generative AI Prohibited Use Policy](https://policies.google.com/terms/generative-ai/use-policy), [Terms of Service](https://policies.google.com/terms), and learn how to configure Gemini Code Assist in GitHub [here](https://developers.google.com/gemini-code-assist/docs/customize-gemini-behavior-github). Gemini can make mistakes, so double check it and [use code with caution](https://support.google.com/legal/answer/13505487).
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [BEAM] Fix Python VarIntCoder OverflowError on uint64 values [beam]
Posted by "gemini-code-assist[bot] (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [BEAM] Fix Python VarIntCoder OverflowError on uint64 values [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39047:
URL: https://github.com/apache/beam/pull/39047#issuecomment-4827448652
Assigning reviewers:
R: @jrmccluskey for label python.
Note: If you would like to opt out of this review, comment `assign to next reviewer`.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [BEAM] Fix Python VarIntCoder OverflowError on uint64 values [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39047:
URL: https://github.com/apache/beam/pull/39047#issuecomment-4824998570
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment `assign set of reviewers`
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix Python VarIntCoder OverflowError on uint64 values [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #39047:
URL: https://github.com/apache/beam/pull/39047#discussion_r3509547178
##########
sdks/python/apache_beam/coders/coder_impl.pxd:
##########
@@ -135,7 +135,6 @@ cdef class TimestampCoderImpl(StreamCoderImpl):
cdef list small_ints
cdef class VarIntCoderImpl(StreamCoderImpl):
- @cython.locals(ivalue=libc.stdint.int64_t)
Review Comment:
We even had dedicated micro-optimization for its Java counterpart: #29689
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix Python VarIntCoder OverflowError on uint64 values [beam]
Posted by "jrmccluskey (via GitHub)" <gi...@apache.org>.
jrmccluskey merged PR #39047:
URL: https://github.com/apache/beam/pull/39047
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix Python VarIntCoder OverflowError on uint64 values [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #39047:
URL: https://github.com/apache/beam/pull/39047#discussion_r3509521379
##########
sdks/python/apache_beam/coders/coder_impl.pxd:
##########
@@ -135,7 +135,6 @@ cdef class TimestampCoderImpl(StreamCoderImpl):
cdef list small_ints
cdef class VarIntCoderImpl(StreamCoderImpl):
- @cython.locals(ivalue=libc.stdint.int64_t)
Review Comment:
This is a common (hot) code path. There might be performance implication here as it added multiple branches and removed this `libc.stdint.int64_t` C type.
Did some local benchmark
current HEAD:
```
VarIntCoderImpl benchmark (SLOW_STREAM=False)
Iterations per test: 20,000,000
--- Small Integers (<32767) ---
Encode: 2.4943s (8,018,329 ops/sec)
Decode: 1.1410s (17,528,902 ops/sec)
--- Large Integers (64-bit) ---
Encode: 3.1621s (6,324,850 ops/sec)
Decode: 1.3801s (14,492,127 ops/sec)
```
reverting this change:
```
VarIntCoderImpl benchmark (SLOW_STREAM=False)
Iterations per test: 20,000,000
--- Small Integers (<32767) ---
Encode: 1.9062s (10,492,259 ops/sec)
Decode: 1.1310s (17,683,731 ops/sec)
--- Large Integers (64-bit) ---
Encode: 2.5946s (7,708,456 ops/sec)
Decode: 1.4151s (14,133,459 ops/sec)
```
Ran it several times and the difference appears to be significant
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org