fix: replace __uint128_t with portable 64-bit decomposition in c_shard_info.pyx (MSVC build failure) - #950
Conversation
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Comment |
There was a problem hiding this comment.
Pull request overview
Replaces a compiler-specific 128-bit multiplication with a portable 64-bit decomposition for shard calculation.
Changes:
- Removes the unsupported
__uint128_tdeclaration. - Computes the product’s high bits using portable 64-bit arithmetic.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…d_info.pyx
cassandra/c_shard_info.pyx computed the high 64 bits of a 64x32-bit product
by casting biased_token to `__uint128_t` and shifting right by 64:
cdef int shardId = (<__uint128_t>biased_token * self.shards_count) >> 64;
`__uint128_t` is a GCC/Clang compiler-builtin extension type, not standard
C/C++ and not part of Cython's own type system. MSVC has no 128-bit integer
type at all, so it treats `__uint128_t` as an undeclared identifier and fails
with cascading syntax errors (C2065/C2146/C2059) in the generated
c_shard_info.c. This breaks Windows wheel builds for any change that
triggers a rebuild of this extension.
Replaced it with a portable multiply-high decomposition that splits the
64x32-bit multiplication into 32-bit halves, using only 64-bit arithmetic
(uint64_t), matching the existing pure-Python fallback already implemented
in cassandra/shard_info.py. This compiles identically on GCC, Clang, and
MSVC, and is numerically identical to the previous 128-bit computation
(verified against 2M+ random inputs, cross-checked against both the pure
-Python fallback and the actual compiled extension).
Found while investigating an unrelated Windows CI failure on PR scylladb#806; the
bug is pre-existing and independent of that PR's changes, so it's fixed
here as its own standalone commit rather than folded into that PR.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ab57ac7 to
13728e7
Compare
Bug
cassandra/c_shard_info.pyxcomputes the shard id for a token by taking thehigh 64 bits of a 64x32-bit product:
__uint128_tis a GCC/Clang compiler-builtin extension type — it is not partof the C or C++ standard, and it is not a type Cython itself understands
natively (the
cdef extern from *: ctypedef unsigned int __uint128_tblockjust tells Cython to trust that the C compiler has it). MSVC has no 128-bit
integer type at all, so on Windows the generated
c_shard_info.cfails tocompile with
error C2065: '__uint128_t': undeclared identifier, followed bya cascade of C2146/C2059 syntax errors. This breaks Windows wheel builds for
any change that touches (or merely triggers a rebuild of) this extension.
Fix
Replace the 128-bit multiply with a portable multiply-high decomposition that
uses only 64-bit arithmetic (
uint64_t), splitting the multiplication into32-bit halves — the same technique already used by the pure-Python fallback
in
cassandra/shard_info.py(_ShardingInfo.shard_id_from_token). Thiscompiles identically on GCC, Clang, and MSVC.
Verification
cleanly.
shard_id_from_tokenoutputagainst the pure-Python fallback (
cassandra.shard_info._ShardingInfo)across 2,000,000 randomized
(shards_count, sharding_ignore_msb, token)triples, covering the full
int64token range — 0 mismatches.tests/unit/suite (770 passed, 38 skipped, 0 failed) plusthe sharding-specific tests (
tests/unit/test_shard_aware.py,tests/unit/test_connection.py::TestShardawarePortGenerator,tests/unit/test_host_connection_pool.py) — all passing, with the realcompiled extension in place.
Context
This was originally found while investigating a Windows wheel-build CI
failure on an unrelated PR (#806, a
ResponseFuture.__init__cleanup). The__uint128_tbug is pre-existing, independent of that PR's changes, andwould affect the Windows build of any PR that happens to trigger a rebuild
of this extension — so it's being fixed here as its own standalone, focused
PR rather than folded into #806.