Skip to content

chore(spanner): add sortable-string util methods - #6214

Open
olavloite wants to merge 1 commit into
googleapis:mainfrom
olavloite:spanner-ssformat
Open

chore(spanner): add sortable-string util methods#6214
olavloite wants to merge 1 commit into
googleapis:mainfrom
olavloite:spanner-ssformat

Conversation

@olavloite

Copy link
Copy Markdown
Contributor

Adds Sortable String Format util methods to the Rust client. These methods are used for location-aware routing in the Spanner client. This allows the client to route requests directly to the Spanner server that holds a given range of data.

This implementation is a port of the corresponding Java client implementation.

@olavloite
olavloite requested review from a team as code owners July 29, 2026 16:51
@product-auto-label product-auto-label Bot added the api: spanner Issues related to the Spanner API. label Jul 29, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the routing module and its submodule ssformat to the spanner crate, providing utilities to encode various Spanner data types into a byte format that preserves lexicographic ordering, along with comprehensive unit tests. The reviewer's feedback focuses on improving code readability and idiomatic Rust patterns, specifically suggesting the use of key.to_vec() for simpler vector allocation, refactoring nested conditionals to return early, and extracting duplicated initialization logic in the signed integer encoding function.

Comment thread src/spanner/src/routing/ssformat.rs Outdated
Comment thread src/spanner/src/routing/ssformat.rs
Comment thread src/spanner/src/routing/ssformat.rs Outdated
@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.51024% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.04%. Comparing base (334b540) to head (77440ad).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/spanner/src/routing/ssformat.rs 98.51% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6214      +/-   ##
==========================================
+ Coverage   96.02%   96.04%   +0.01%     
==========================================
  Files         269      270       +1     
  Lines       67282    67819     +537     
==========================================
+ Hits        64607    65135     +528     
- Misses       2675     2684       +9     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces location-aware routing internal modules for Spanner, specifically implementing Sortable String Format (ssformat) encoding utilities to serialize various Spanner data types (such as integers, doubles, strings, bytes, timestamps, UUIDs, and nulls) into lexicographically ordered byte sequences. The review feedback highlights that f64::to_bits() does not guarantee a canonical representation for NaN values in Rust, and suggests normalizing NaN values in both append_double_increasing and append_double_decreasing to ensure consistent encoding and sorting.

Comment thread src/spanner/src/routing/ssformat.rs
Comment thread src/spanner/src/routing/ssformat.rs
@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new module routing::ssformat in the Spanner crate, which provides Sortable String Format (ssformat) encoding utilities for Spanner keys to preserve lexicographic ordering. The review feedback highlights two key areas for improvement: first, replacing the bit-shifted constant TIMESTAMP_SECONDS_OFFSET with i64::MIN for better readability and safety; second, addressing a potential usize underflow bug in append_int64_internal by performing the buffer bounds check before calculating the index 7 - len.

Comment thread src/spanner/src/routing/ssformat.rs Outdated
Comment thread src/spanner/src/routing/ssformat.rs Outdated
@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new routing module containing ssformat utilities for encoding Spanner keys while preserving lexicographic ordering. A review comment identified a bug in the make_prefix_successor implementation, which fails to correctly handle certain byte sequences; a code-based fix was provided to ensure robust prefix successor generation.

Comment thread src/spanner/src/routing/ssformat.rs Outdated
@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new internal module routing containing ssformat encoding utilities for Spanner keys, enabling lexicographical sortable encoding of various Spanner data types. The reviewer suggested a minor improvement to ensure platform-independent NaN canonicalization by using a hardcoded IEEE 754 quiet NaN bit pattern constant instead of f64::NAN.to_bits().

Comment thread src/spanner/src/routing/ssformat.rs
@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new routing module and a ssformat submodule in the spanner crate, implementing Sortable String Format encoding utilities for Spanner keys along with comprehensive unit tests. The review feedback suggests removing an unnecessary else block in canonical_double_bits to adhere to the repository style guide, and optimizing the integer encoding logic in append_int64_internal by utilizing standard library intrinsics like leading_zeros and leading_ones instead of loops.

Comment thread src/spanner/src/routing/ssformat.rs
Comment thread src/spanner/src/routing/ssformat.rs
Adds Sortable String Format util methods to the Rust client. These methods are used
for location-aware routing in the Spanner client. This allows the client to route
requests directly to the Spanner server that holds a given range of data.

This implementation is a port of the corresponding Java client implementation.
@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new internal module routing and its submodule ssformat to the Spanner crate. The ssformat module implements Sortable String Format encoding utilities for Spanner keys, supporting ascending and descending lexicographical sorting for various data types (booleans, integers, doubles, strings, bytes, nullable markers, timestamps, and UUIDs). It also includes comprehensive unit tests verifying correct ordering behavior and edge cases. There are no review comments to address, and I have no additional feedback to provide.

@olavloite
olavloite requested a review from sakthivelmanii July 29, 2026 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: spanner Issues related to the Spanner API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant