Fix GEOHASH last character to always output '0' (52-bit precision) - #1635
Conversation
|
Request your review on this PR, Seth Speaks (@sspeaks) and Paulus Pärssinen (@PaulusParssinen) |
There was a problem hiding this comment.
Pull request overview
Updates Garnet’s GEOHASH output to match Redis behavior by forcing the 11th (final) geohash character to '0', since Garnet stores only 52 bits of precision (insufficient for the full 55 bits implied by 11 base32 characters).
Changes:
- Adjust
GeoHash.GetGeoHashCodeto force the last base32 character index to0(so the last character is always'0'). - Update geohash expectations in unit/integration tests to reflect the new Redis-compatible output.
- Change the pinned .NET SDK version in
global.jsonfrom10.0.103to10.0.102.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| libs/server/Objects/SortedSetGeo/GeoHash.cs | Forces the last geohash character to '0' for Redis compatibility under 52-bit precision. |
| test/Garnet.test/GeoHashTests.cs | Updates expected geohash strings and clarifies the '0' last-character convention. |
| test/Garnet.test/RespSortedSetGeoTests.cs | Updates expected GEOHASH output in RESP sorted set geo tests (e.g., Palermo). |
| global.json | Updates the repo’s pinned .NET SDK patch version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Paulus Pärssinen (PaulusParssinen)
left a comment
There was a problem hiding this comment.
Added comment with alternative approach without branches in the inner loop, either will do the job.
All other GEO* functionality will still use the available 52-bit precision that is persisted in the sorted set.
A standard geohash is 11 base-32 characters = 55 bits (11 x 5), but geo positions are stored as 52-bit values packed into sorted-set scores -- 3 bits short of what the 11th character needs. Since those bits are not stored, the 11th character cannot be derived and is emitted as '0' by convention. GetGeoHashCode now sets the last character to '0' and encodes only the first 10 characters from the stored bits. Previously the last character reflected leftover shifted bits (e.g. "sqc8b49rnys" instead of "sqc8b49rny0"); the first 10 characters were already correct. Fixes #1625 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c54c3cfa-79a3-4949-a2ec-ae0658bc7894
040fc19 to
8b7aff7
Compare
|
Rebased onto latest
|
Summary
Fixes the final character emitted by
GEOHASHso it matches the established geohash convention.A standard geohash is 11 base-32 characters = 55 bits (11 × 5). Garnet stores geo positions as 52-bit values packed into sorted-set scores — 3 bits short of what the 11th character needs. Because those 3 bits are not stored, the 11th character cannot be derived from the score and is emitted as
'0'by convention.Previously
GetGeoHashCodeshifted the leftover bits into the final 5-bit index, producing a divergent last character. The first 10 characters were already correct; only the 11th changed:GEOADD k 13.361389 38.115556 Palermo→GEOHASH k Palermosqc8b49rnyssqc8b49rny0Changes
libs/server/Objects/SortedSetGeo/GeoHash.cs—GetGeoHashCodenow sets the last character to'0'and encodes only the first 10 characters from the stored 52 bits.test/standalone/Garnet.test.collections/GeoHashTests.csandRespSortedSetGeoTests.cs— updated expectedGEOHASHoutputs to the…0form.Notes
GetCoordinatesFromLong), never from the 11-character string, so all otherGEO*commands retain full available precision.Fixes #1625