feat: experimental location aware routing- Part 3 #4294
Conversation
This commit adds the foundational SsFormat class that provides sortable string format (ssformat) encoding utilities. This encoding is used by Spanner for key ordering and routing. Key features: - Composite tag encoding for interleaved tables - Signed/unsigned integer encoding (increasing/decreasing) - String and bytes encoding with proper escaping - Double encoding with proper sign handling - Timestamp and UUID encoding - Null value markers with configurable ordering - TargetRange class for key range representation Includes unit tests for all encoding functions. This is part of the experimental location-aware routing for improved latency.
product-auto-label
bot
added
size: xl
Summary of ChangesHello @rahul2393, 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 introduces a comprehensive system for encoding and caching keys and key ranges within the Google Cloud Spanner client library. It provides a structured way to define key encoding "recipes" based on schema information, efficiently cache these recipes, and then use them to compute precise routing hints for various database operations like reads and mutations. This enhancement is designed to improve the performance and accuracy of key-based operations by standardizing and optimizing the serialization of keys. Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
rahul2393
changed the title
![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 key recipe cache to improve routing performance. The implementation includes new classes for key encoding (KeyRecipe, SsFormat), caching (KeyRecipeCache), and data representation (TargetRange), along with comprehensive golden tests. My review focuses on correctness and adherence to Java best practices. I've identified critical issues with date validation that could lead to incorrect behavior, and I've also suggested improvements for logging and class design to enhance maintainability and robustness.
| if (month < 1 || month > 12 || day < 1 || day > 31 || hour > 23 || minute > 59 || second > 59) { | ||
| throw new IllegalArgumentException("Invalid TIMESTAMP string: " + ts); | ||
| } | ||
| // Year must be 0000-9999 (year 0 is allowed) | ||
| if (year < 0 || year > 9999) { | ||
| throw new IllegalArgumentException("Invalid TIMESTAMP string: " + ts); | ||
| } |
There was a problem hiding this comment.
The date validation for timestamps is incomplete. The current check allows invalid dates like 2023-02-31. This could lead to incorrect behavior when processing invalid timestamp strings.
For more robust validation, I suggest using java.time.LocalDateTime.of() to validate the date and time components together. This will correctly handle complexities like leap years and the number of days in each month.
// Year must be 0000-9999 (year 0 is allowed)
if (year < 0 || year > 9999) {
throw new IllegalArgumentException("Invalid TIMESTAMP string: " + ts);
}
try {
java.time.LocalDateTime.of(year, month, day, hour, minute, second);
} catch (java.time.DateTimeException e) {
throw new IllegalArgumentException("Invalid date part in TIMESTAMP string: " + ts, e);
}| if (month < 1 || month > 12 || day < 1 || day > 31) { | ||
| throw new IllegalArgumentException("Invalid DATE string: " + dateStr); | ||
| } | ||
| // Year can be 0000-9999 for DATE | ||
| if (year < 0 || year > 9999) { | ||
| throw new IllegalArgumentException("Invalid DATE string: " + dateStr); | ||
| } |
There was a problem hiding this comment.
The date validation in validateDate is incomplete. The check day < 1 || day > 31 can allow invalid dates, for example, 2023-02-31. This could lead to incorrect key encoding.
To ensure correct date validation, I recommend using java.time.LocalDate.of() to validate the year, month, and day components.
// Year can be 0000-9999 for DATE
if (year < 0 || year > 9999) {
throw new IllegalArgumentException("Invalid DATE string: " + dateStr);
}
try {
java.time.LocalDate.of(year, month, day);
} catch (java.time.DateTimeException e) {
throw new IllegalArgumentException("Invalid DATE string: " + dateStr, e);
}| } | ||
| } catch (IllegalArgumentException e) { | ||
| // Log or handle failed recipe creation | ||
| System.err.println("Failed to add recipe: " + recipeProto + ", error: " + e.getMessage()); |
There was a problem hiding this comment.
Using System.err.println for logging in a library is not recommended as it cannot be configured by the library's users. Please use a logging framework like java.util.logging (JUL) instead. This would allow applications to control log levels and output destinations.
For example: logger.log(Level.WARNING, "Failed to add recipe: " + recipeProto, e);
This should be applied to all System.err.println calls in this file (e.g., lines 101, 114, 122, 131, 141, 146).
| public ByteString start; | ||
| public ByteString limit; | ||
| public boolean approximate; |
There was a problem hiding this comment.
The fields start, limit, and approximate are public and mutable. This can lead to unexpected behavior if a TargetRange object is modified from different parts of the code.
Consider making this class immutable by declaring the fields as private final and providing public getter methods. This would make the class thread-safe and its behavior more predictable. If you make this change, the mergeFrom method will also need to be updated to return a new TargetRange instance instead of modifying the existing one.
This commit adds the key recipe system for translating requests to sortable string format (ssformat) keys for routing. Key components: - KeyRecipe: Translates requests to ssformat keys using server-provided recipes - KeyRecipeCache: Caches recipes by request fingerprint for efficient reuse - PreparedRead: Helper class for preparing read requests with routing info - RecipeGoldenTest: Golden tests for recipe encoding validation - RecipeTestCases: Test case definitions for recipe tests This is part of the experimental location-aware routing for improved latency.
rahul2393
force-pushed
the
feat/key-recipe-cache
branch
from
ef2c8a1 to
f71ee0e
Compare
2 participants
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> ☕️
If you write sample code, please follow the samples format.