Repository navigation
Conversation
product-auto-label
Bot
added
the
api: bigtable
![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 enhances map deduplication in google::cloud::bigtable::Value by recursively handling nested maps, arrays of maps, and structs containing maps. It introduces helper functions TypeContainsMap and DedupProtoMaps, updates Value::DedupProtoMap() to use them, and switches to insert_or_assign to ensure duplicate keys are overwritten with their latest values. Comprehensive unit tests have been added to verify these changes. Feedback is provided to replace auto with explicit StatusOr types in Value::GetValue to adhere to the repository's style guide.
285547d to
ddbdfc2
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the Bigtable Value class to recursively deduplicate keys in nested maps, arrays of maps, and structs containing maps. It introduces the TypeContainsMap and DedupProtoMaps helper functions to handle deep structures, updates map extraction to use insert_or_assign to ensure the last-write-wins behavior for duplicate keys, and adds comprehensive unit tests to verify these scenarios. I have no feedback to provide as the implementation is correct and conforms to the repository style guides.
scotthart
added
the
do not merge
ddbdfc2 to
73888f9
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16655 +/- ##
==========================================
- Coverage 92.53% 92.52% -0.01%
==========================================
Files 2269 2269
Lines 222896 223079 +183
==========================================
+ Hits 206250 206413 +163
- Misses 16646 16666 +20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Deduplicate duplicate keys recursively across nested MAP values (inside ARRAY, STRUCT, or outer MAP values) with last-key-wins semantics per the Bigtable Type.Map specification, and use insert_or_assign during deserialization as a defense-in-depth safeguard.
73888f9 to
223d0c7
Compare
Summary
bigtable::Valuepreviously only invokedDedupProtoMap()when the top-levelgoogle::bigtable::v2::Typewasmap_type, andDedupProtoMap()only deduplicated entries at the top level. When a SQLMAPwas nested inside anARRAY<MAP<...>>,STRUCT<..., MAP<...>>, or outerMAP<..., MAP<...>>, duplicate keys in the nestedMAPwere not deduplicated with last-key-wins semantics:Value::get<std::unordered_map<K, V>>()usedstd::unordered_map::insert, retaining the first occurrence of a duplicate key instead of the last occurrence per thegoogle.bigtable.v2.Type.Mapspecification.operator==,operator<<, andToProto()retained duplicate entries in nestedMAPproto representations.Changes
TypeContainsMap()and recursiveDedupProtoMaps()ingoogle/cloud/bigtable/value.ccto deduplicateMAPentries with last-key-wins semantics acrossMAP,ARRAY, andSTRUCTtypes without allocating or traversing values whose schema contains noMAP.DedupProtoMap()inValueconstructors formap_type,array_type, andstruct_typevalues, and useinsert_or_assigninValue::GetValue(std::unordered_map<K, V> const&, ...)as a defense-in-depth safeguard.value_test.cc(ProtoArrayOfMapsWithDuplicateKeys,ProtoStructWithNestedMapDuplicateKeys,ProtoNestedMapOfMapsWithDuplicateKeys) andpartial_result_set_source_test.cc(NestedMapsWithDuplicateKeys).