feat: add Set container type - #593
Open
sshaplygin wants to merge 2 commits into
Open
Conversation
Closes ydb-platform#234. YDB has no dedicated wire type for sets: `Set<T>` is transferred as `Dict<T, Void>`. ydb-go-sdk implements it that way - `Set.ToYDB` emits a `DictType` with a void payload, and `setValue.toYDB` emits pairs whose payload is the void value - so this mirrors that encoding. - `Value::Set(Box<ValueSet>)` with `Value::set_from(example, members)`, validating member types against the example exactly like `list_from`. The example carries the element type, so an empty set still encodes as `Dict<T, Void>` rather than losing it to `EmptyDict` the way the Go SDK's `EmptySet` does. - Encoding to `RawType::Dict { key, payload: Void }` with `RawValue::Pairs`, each payload a null flag. The raw layer already had dict and pair support, so only the public mapping was missing. - Decoding `Dict<T, Void>` back to `Value::Set`. A dict with any other payload is a real dict, which the public API does not model yet, so it is rejected instead of silently dropping the payloads. - `HashSet<T>` conversions in both directions, the set analogue of the existing `Vec<T>` <-> `List<T>` mapping. `FromIterator for Value` already claims iterators for `List`, so sets convert through `From` on the concrete `HashSet`. Members are sent in the order given and are not de-duplicated, matching the Go SDK; the doc comment states that YDB requires unique keys. Tests cover the wire shape (dict type, void payload, null-flag pairs), an empty set keeping its element type, rejection of a dict with a real payload, the `set_from` validation paths, and the `HashSet` conversions including a mismatched element type. Adding the variant also extends the `Value::COUNT` fixture, so `Set` now goes through the existing value/proto round-trip test. A server-side test asserting YDB accepts the encoding and answers `DictContains` is added as `#[ignore]`, next to the other integration tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Running the new `Set` integration test against a real YDB surfaced two decoding bugs. `TryFrom<ProtoValue> for RawValue` rejected any `Value` message that carried no oneof, no items and no pairs, but that message is exactly how the wire spells two legitimate things: - **`Void`**, which has no representation of its own. It arrives as the payload of every pair in a `Dict<T, Void>`, i.e. of every `Set<T>` read back from the server. Handled in the pair conversion, where the payload is known to be void. - **An empty container.** An empty list or dict carries neither items nor pairs, so it decoded as "empty value item" instead of an empty value. The generic decoder now yields `NullFlag` and lets the type-directed layer above build the right empty value - which is what the existing `(RawType::List(_), RawValue::NullFlag)` arm was already written to expect. The second one is not new with `Set`: reading back an empty `List<T>` fails the same way on master. `empty_list_roundtrips_through_the_server` covers it, and both new integration tests fail without this change. The `Set` integration test now also covers an empty set and a `Set<Utf8>` alongside the `Set<Int32>` and `DictContains` checks. Verified against `ydbplatform/local-ydb:nightly`, the image CI uses: 314 tests pass with `--include-ignored`, none failing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #593 +/- ##
==========================================
+ Coverage 86.91% 86.98% +0.07%
==========================================
Files 198 198
Lines 19492 19791 +299
==========================================
+ Hits 16941 17216 +275
- Misses 2551 2575 +24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #234.
Encoding
YDB has no dedicated wire type for sets. Per the container types documentation,
Set<T>is "a special case of a dictionary with theVoidvalue type", and ydb-go-sdk implements it that way —Set.ToYDBemits aDictTypewith a void payload, andsetValue.toYDBemits pairs whose payload is the void value. This mirrors that encoding.The raw layer already had
RawType::DictandRawValue::Pairs, so only the public mapping was missing.What's added
Value::Set(Box<ValueSet>)withValue::set_from(example, members), validating members against the example exactly likelist_from.HashSet<T>↔Set<T>in both directions — the set analogue ofVec<T>↔List<T>.FromIterator for Valuealready claims iterators forList, so sets convert throughFromon the concreteHashSet.Setsection inexamples/container-types.rs.Two deliberate differences from the Go SDK:
EmptySet/EmptyDictbecause it has no type information when the item list is empty;set_fromalways has the example, so it emitsDict<T, Void>with zero pairs, which round-trips.Valuehas noDictvariant yet (feat: Containers (Struct Variant List Tuple) #309), so this fails loudly.Members are sent in the order given and are not de-duplicated, matching Go. YDB requires unique dict keys, so duplicates are the caller's responsibility — stated in the doc comment. Happy to add validation if you would prefer it over the O(n²) cost.
Two decoding bugs this surfaced
The unit tests passed, but running against a real server did not.
TryFrom<ProtoValue> for RawValuerejected anyValuemessage carrying no oneof, no items and no pairs — which is exactly how the wire spells two legitimate things:Void. It has no representation of its own, so it arrives as the payload of every pair in aDict<T, Void>. Fixed in the pair conversion, where the payload is known to be void."empty value item". The generic decoder now yieldsNullFlagand lets the type-directed layer build the right empty value — which is what the existing(RawType::List(_), RawValue::NullFlag)arm was already written to expect.The second is not new with
Set: reading back an emptyList<T>fails the same way on master today.empty_list_roundtrips_through_the_servercovers it. Both new integration tests fail if the fix is reverted.Verification
Run against
ydbplatform/local-ydb:nightly— the image CI uses:Master on the same image is 302 passed / 0 failed, so nothing regressed.
Server-side coverage:
Set<Int32>round-trip,DictContains(which only type-checks if the server really sees a dict/set), an empty set, and aSet<Utf8>. Unit coverage: the wire shape (dict type, void payload, null-flag pairs), rejection of a dict with a real payload, theset_fromvalidation paths, and theHashSetconversions including a mismatched element type. Adding the variant also extends theValue::COUNTfixture, soSetnow flows through the existing value/proto round-trip test.Note for reviewers
Adding a variant is source-compatible under the default
non_exhaustive, but breaking under theforce-exhaustive-allfeature — which is what that feature exists to surface. It compiles clean either way.Unrelated but worth a separate issue:
docker-compose.yamlpinsydbplatform/local-ydb:latest, while CI uses:nightly. On:latest, 11 of the existing integration tests fail withUnknown name: $val— every query-service parameter test. Local contributors following the README hit that before touching any code.🤖 Generated with Claude Code