test(spanner): add location router finder golden tests and centralize textproto test utils - #6482
Conversation
There was a problem hiding this comment.
Code Review
This pull request consolidates and refactors the textproto parsing utilities for Spanner golden tests into a shared helper module (textproto_test_utils.rs), removing duplicate parsing code from key_range_cache/golden_tests.rs and key_recipe/golden_tests.rs. It also introduces a new set of golden conformance tests for the location router (location_router/golden_tests.rs). The review feedback suggests improving null value handling in the newly introduced parsing helpers: specifically, adding support for null_value: NULL_VALUE in parse_constant_value and allowing serde_json::Value::Null in query_params_to_target_range to prevent premature termination of key encoding.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6482 +/- ##
=========================================
Coverage 96.37% 96.38%
=========================================
Files 297 297
Lines 84017 85683 +1666
=========================================
+ Hits 80971 82584 +1613
- Misses 3046 3099 +53 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… textproto test utils - Add golden conformance test suite for `finder_test.textproto` (36 test cases, 203 request events) validating query key encoding, range cache lookups, server selection, and routing hints. - Centralize all golden test fixtures and parsers (`recipe_test`, `cache_test`, `finder_test`) into `textproto_test_utils.rs`. - Add `unescape_bytes` support for hex `\xHH` sequences and standard C escapes. - Introduce reusable `skip_block` helper and decouple nested list value parsing. - Tighten golden routing hint assertions with strict struct equality, verifying `skipped_tablet_uid` and per-event unhealthy server isolation matching Java and Go reference implementations.
b98507a to
bc7b235
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request consolidates textproto parsing logic from multiple golden test files into a shared utility module, textproto_test_utils.rs, while adding support for hex escape sequences and comprehensive unit tests. It also introduces a new golden conformance test suite for the location router. The review feedback highlights opportunities to optimize performance by avoiding unnecessary vector clones in location_router/golden_tests.rs and suggests enhancing parse_constant_value in textproto_test_utils.rs to support parsing float values.
| } | ||
| } | ||
|
|
||
| let start = ss_key.clone(); |
There was a problem hiding this comment.
The ss_key vector is not used after this point, so cloning it into start is unnecessary. We can avoid the allocation and copy by moving ss_key directly.
| let start = ss_key.clone(); | |
| let start = ss_key; |
References
- Scrutinize expensive uses of clone() and move the data instead if it is not necessary to copy. (link)
| } | ||
| } | ||
|
|
||
| let start = ss_key.clone(); |
There was a problem hiding this comment.
The ss_key vector is not used after this point, so cloning it into start is unnecessary. We can avoid the allocation and copy by moving ss_key directly.
| let start = ss_key.clone(); | |
| let start = ss_key; |
References
- Scrutinize expensive uses of clone() and move the data instead if it is not necessary to copy. (link)
| pub(crate) fn parse_constant_value(trimmed: &str) -> Option<serde_json::Value> { | ||
| if let Some(string_val) = extract_value(trimmed, "string_value:") { | ||
| Some(serde_json::Value::String(string_val.to_string())) | ||
| } else if let Some(number_val) = extract_value(trimmed, "number_value:") { | ||
| number_val | ||
| .parse::<i64>() | ||
| .ok() | ||
| .map(|num| serde_json::Value::Number(num.into())) | ||
| } else { | ||
| extract_value(trimmed, "bool_value:") | ||
| .map(|bool_val| serde_json::Value::Bool(bool_val == "true")) | ||
| } | ||
| } |
There was a problem hiding this comment.
Currently, parse_constant_value only attempts to parse number_value as an i64. If a float value (e.g., 3.14) is provided as a constant or query parameter in the textproto, parsing will fail and return None. We should support parsing floats as well to ensure robustness and parity with json_to_spanner_value.
pub(crate) fn parse_constant_value(trimmed: &str) -> Option<serde_json::Value> {
if let Some(string_val) = extract_value(trimmed, "string_value:") {
Some(serde_json::Value::String(string_val.to_string()))
} else if let Some(number_val) = extract_value(trimmed, "number_value:") {
if let Ok(integer) = number_val.parse::<i64>() {
Some(serde_json::Value::Number(integer.into()))
} else if let Ok(float) = number_val.parse::<f64>() {
serde_json::Number::from_f64(float).map(serde_json::Value::Number)
} else {
None
}
} else {
extract_value(trimmed, "bool_value:")
.map(|bool_val| serde_json::Value::Bool(bool_val == "true"))
}
}
finder_test.textproto(36 test cases, 203 request events) validating query key encoding, range cache lookups, server selection, and routing hints.recipe_test,cache_test,finder_test) intotextproto_test_utils.rs.unescape_bytessupport for hex\xHHsequences and standard C escapes.skip_blockhelper and decouple nested list value parsing.skipped_tablet_uidand per-event unhealthy server isolation matching Java and Go reference implementations.