Repository navigation
map_codec: option to leave default map entry fields out, as prost does - #562
benedikt-bartscher wants to merge 6 commits into
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
Covers the owned and the view encoders.
ba6d138 to
46251dc
Compare
iainmcgin
left a comment
There was a problem hiding this comment.
[claude code] Iain's decision on the design question: an opt-in codegen flag is the right shape for this, so the draft can go ahead. DynamicMessage, the checked-in well-known types and types from crates generated without the flag will keep writing both fields; please say so where the option is documented, since "byte for byte with prost" holds only for the types generated with it.
Three things to settle before it leaves draft.
1. A closed enum value of 0 that is not the enum's default is lost. ClosedEnum::is_default is value.to_i32() == 0, and the view path's map_element_is_written_expr uses to_i32() != 0. The decoder starts each entry from Default::default() (map_codec.rs, let mut val: VC::Value = Default::default()), and for a closed enum that is the first declared value. With
enum E { A = 1; B = 0; }an entry whose value is B is written without its value field and reads back as A. The reverse case costs bytes instead of data: a value of A is written although prost leaves it out, because prost compares against the enum's default (skip_val = val == val_default in prost/src/encoding.rs, where the generated code passes the enum's default). Comparing with E::default() fixes both, for owned messages and views. is_default_is_the_decoded_default passes today because its enum starts at 0; an enum of the shape above belongs in that test and in map_omit_defaults.proto.
2. A message value with a proto2 required field differs from prost. prost leaves out a message value that equals Default::default(). The PR leaves one out when it encodes to nothing. A message with a required field at its default equals the default and still encodes its field, so prost omits the value and this option writes it. Either compare the way prost does, or state the difference next to the -0.0 note in the option's docs and the migration guide.
3. Lazy views. map_omit_defaults.rs covers owned messages and strict views. Please add a case for the lazy family (lazy_views=true), or say in the docs what it writes.
Smaller: the changelog fragment and the guide rows read well; when you mark it ready, the usual two-sentence fragment lead (**…** (#562).) is all it needs.
…ot at 0 A missing closed enum value decodes to the enum's first value, which need not be 0, and prost compares with that default too. The view encoders call the same check.
…_defaults The table writes both fields of every map entry, so such a message falls back to the unrolled codec, under its own reason in the fallback summary.
…hat differs from prost A message value is still left out only when it encodes to nothing, so one whose required field holds its default is written, where prost leaves it out; views have no PartialEq to compare with the default as prost does.
|
Thanks. I merged 1. Closed enums (246975d). 2. 3. Lazy views (f3461e2). They use the strict views' map encoders. The fixture is compiled with From the merge (830fc33): since #477 the table codec handles maps and writes both fields of every entry, so under the option a message with a map field keeps the unrolled codec, with its own reason in the fallback summary. The docs now also say that each crate encodes as it was generated and that the well-known types and |
…it differs from prost The option's rustdoc now states that decoding is unchanged, puts the one value that does not survive a round trip (a -0.0 float) first, and lists the differences from prost and the option's scope separately. It also says that an exact-path table rule for a message with a map field is an error once the option is on. The migration page limits its promise to map entries and names two encoding differences the option does not remove. Two test comments no longer claim what another compiler accepts.
iainmcgin
left a comment
There was a problem hiding this comment.
[claude code] Thanks, the three points of the first review are answered: the closed-enum predicate now mirrors the decode default on the owned, view and lazy view paths, the required difference is documented, and the lazy view tests compare bytes. The table fallback in 830fc33 is right too.
Pushed one maintainer commit on top (8988a48), documentation only; no code or test logic changes.
Config::map_entries_omit_defaults: says that decoding is unchanged, puts the one value that does not survive a round trip (a-0.0float) first, and separates "Differences from prost" from "Scope". It also says that acodec_strategy_inrule selecting the table for a message with a map field by its exact path is an error once the option is on, and that theopen_enums_indifference needs an enum whose first value is not 0, which protoc rejects as a map value.CodeGenConfig::map_entries_omit_defaults: states the message-value difference from prost and the per-run scope, so a directbuffa-codegencaller sees them.docs/migration-from-prost.md: the promise is limited to map entries, with two differences the option does not remove (a proto3floatordoublewithoutoptionalat-0.0, and unknown fields that buffa writes back).- Two test comments no longer say what buf accepts; the fragment gains the exact-path clause.
Strict rustdoc and clippy pass, and the tests of buffa, buffa-codegen and buffa-build pass on this head (1791 passed).
buffa writes both the key and the value of every map entry, as the C++ and Java runtimes do. prost leaves either out when it holds the default. Both decode the same, but the bytes differ. That matters to a project moving from prost whose stored or hashed encodings must not change. Ours is a sync protocol whose frozen test vectors pin encoded bytes; with this option they replay byte for byte after the move.
This adds an opt-in
map_entries_omit_defaults:buffa_build::Config::map_entries_omit_defaults,CodeGenConfig::map_entries_omit_defaults,map_entries_omit_defaults=trueinprotoc-gen-buffa. It is off by default. With it on:false, empty, the enum's default: 0 for an open enum, the first value for a closed one) is left out of the entry-0.0float value decodes as0.0, as it does from prostWith the option off, the generated code is the same as before.
Changes:
map_codec):MapCodec::is_default. The trait is sealed, so the new method is not breaking.field_len_omitting_defaults/write_field_omitting_defaults, and the message-valuedmessage_field_len_omitting_defaults/write_message_field_omitting_defaults. Size and write decide the omissions once per entry. An empty message value loses its tag and length, but itswrite_tostill runs, so it consumes whateverSizeCacheslots itscompute_sizereserved.map_encoders). The view encoders inline the entry encoding and guard each field the same way, so an owned message and its view encode the same data to the same bytes. Lazy views use the same map encoders. UnderCodecStrategy::Table, a message with a map field keeps the unrolled codec, with its own reason in the fallback summary.CodeGenConfigand the plugin.docs/guide.mddocs/migration-from-prost.md, a "Map entries" note and a feature-table row. For maps with more than one entry to come out in prost's order, they also needmap_type(MapRepr::BTreeMap)where prost usedbtree_map.-0.0; the size equals the bytes written; the round tripis_defaultfor every codecSizeCacheslots of its own keeps the slots in stepbuffa-test: a schema with every value kind, compiled with the option and with lazy views. The owned bytes are pinned, the strict and the lazy view re-encode the same bytes, and every entry decodes back. A proto2 schema adds a closed-enum map and a message value whoserequiredfield holds its default.Not covered, as the docs say:
DynamicMessageand buffa-types' pre-generated well-known types (google.protobuf.Struct) keep writing both fields; each crate's types encode as that crate was generated. A message value is left out only when it encodes to nothing, so one whose proto2requiredfield holds its default is kept, unlike prost.cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warningsandcargo test --workspacepass (4241 tests).