Fix Python VarIntCoder OverflowError on uint64 values#39047
Conversation
The Cython write_var_int64/get_varint_size stream methods take a signed int64_t parameter. A Python int in the unsigned 64-bit range [2**63, 2**64) -- a uint64 -- is converted to that signed parameter at the call boundary and rejected with an OverflowError before the method body runs, even though the body already operates on the unsigned bit pattern and the VarInt wire encoding is well-defined. Fold such values to the signed int64 with the identical bit pattern (and thus identical VarInt encoding) before handing them to the stream. This matches Java's signed VarIntCoder on the wire; decoding remains signed. Values past 64 bits are left unchanged and still overflow downstream, preserving the coder's documented 64-bit limit. Adds test_varint_coder_uint64 covering no-overflow encoding, wire equivalence to the signed twin, size estimation, signed decode, and the still-raising out-of-range case (guarded on the compiled implementation). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! 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
New Features🧠 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 to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization 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 Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a helper function _as_signed_int64 to fold uint64 values in the range [2**63, 2**64) into signed int64 values, preventing Cython overflow issues in VarIntCoderImpl and aligning Python's behavior with Java's VarIntCoder. The feedback suggests enforcing the 64-bit integer range limit in the pure-Python path as well to ensure consistent behavior between the compiled and uncompiled implementations, which would also allow simplifying the new unit tests by removing the compiled-only guard.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
Address review feedback: raise OverflowError in _as_signed_int64 for values outside the 64-bit range (reusing fits_in_64_bits) so the pure-Python path matches the Cython int64_t overflow instead of silently encoding out-of-range values. Both encode_to_stream and estimate_size already wrap OverflowError, so the user-facing message is unchanged. Drop the is_compiled guard in test_varint_coder_uint64 so the out-of-range case runs on both paths, add the negative lower-bound case (-(2**63) - 1), and remove the now-unused coder_impl import. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The compiled encode() cast value to a C int64_t (ivalue, typed via coder_impl.pxd) for the small-ints fast path, which overflowed on uint64 values before reaching encode_to_stream's fold -- the cause of the test_varint_coder_uint64 CI failure on the Python PreCommit suites. Do the small-ints check with a plain Python-object comparison so it can't overflow; non-small values (including uint64 and out-of-range ints) fall through to StreamCoderImpl.encode -> encode_to_stream, where _as_signed_int64 folds them and raises the wrapped OverflowError for genuinely out-of-range values. Drop the now-unused ivalue int64_t local hint from coder_impl.pxd. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Assigning reviewers: R: @jrmccluskey for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
AviKndr
changed the title
|
|
||
| cdef list small_ints | ||
| cdef class VarIntCoderImpl(StreamCoderImpl): | ||
| @cython.locals(ivalue=libc.stdint.int64_t) |
There was a problem hiding this 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
There was a problem hiding this comment.
We even had dedicated micro-optimization for its Java counterpart: #29689
3 participants
Fixes #39048
Addresses #19696 — this makes
VarIntCoderoverflow behavior consistent between the compiled and pure-Python paths, but does not fully resolve that issue, so it is intentionally not closed on merge.What
VarIntCoderraisesOverflowErrorwhen 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:The body re-casts to
uint64_tand encodes the bit pattern correctly, but Cython converts the incoming Python int to the signedint64_tparameter at the call boundary — before the body runs — so any uint64 value is rejected withOverflowError. The same applies toget_varint_sizeused byestimate_size.Fix
A uint64 value
vand the signed int64v - 2**64(its two's-complement twin) produce identical VarInt bytes.VarIntCoderImplnow folds uint64 values to that signed twin before they cross into Cython, in bothencode_to_streamandestimate_size:VarIntCoder.slow_streampath produces the same bytes.>= 2**64are left unchanged and still overflow in the Cython path, preserving the coder's 64-bit contract.Note: decoding remains signed (
read_var_int64returnsint64_t), matching Java — so the encoding of2**64 - 1decodes 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_uint64to the sharedcoders_test_common.pysuite (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 to help us incorporate your contribution quickly and easily:
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, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.🤖 Generated with Claude Code