| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
Thanks for the thorough PR, and for building the decode path on the existing IntLookup machinery from #135, that's the right foundation.
An int-keyed struct that is also tagged decodes fine as a concrete type, but breaks as a union member whenever the string tag is not the first key on the wire. This reproduces entirely in-library through the sorted encoder, which orders int keys ahead of the string tag:
class A(msgspec.Struct, tag=True, int_keys={"a": 1, "b": 2}):
a: int
b: int
class B(msgspec.Struct, tag=True, int_keys={"a": 1}):
a: int
buf = msgspec.msgpack.Encoder(order="sorted").encode(A(1, 2)) # {1:1, 2:2, "type":"A"}
msgspec.msgpack.decode(buf, type=A | B)
# ValidationError: Expected `str`, got `int` - at `key` in `$`So msgspec's own sorted encoder emits bytes its own union decoder then rejects. mpack_decode_struct_union scans map keys with mpack_decode_cstr to find the tag and can't skip past an int key to reach it; string-keyed unions are order-independent precisely because it can skip string keys, so this is a regression relative to them. A foreign producer that doesn't put the tag first hits the same wall. The test_sorted_order_tagged test only decodes as a concrete type, so CI stays green over the hole.
Fix needs one of: int-key handling in the union tag-scan, a guarantee the tag encodes first even under order="sorted", or an explicit tag + int_keys restriction. Whichever way, this path wants a union test.
(No memory-safety issues found in the C changes on a careful read: refcounting, GC traversal/clear, NULL-checks, the class-creation validation, and the sorted-path recursion/alloc handling are all balanced. The blocker above is a logic/interaction bug, not a leak.)
Both are documented and consistent (no crash, no round-trip mismatch), but neither gives the user any signal.
The motivation is smaller MessagePack, but array_like=True is already strictly smaller (no keys at all) and applies to both codecs, and rename to short names covers the readable-and-evolvable case on both codecs. The genuine niche int_keys fills is compact and schema-evolvable (protobuf-style stable field numbers: order-independent, tolerant of gaps, sparse with omit_defaults) which is real but narrow, and the PR doesn't spell it out. Separately, the MessagePack-only-with-silent-JSON-fallback framing cuts against how integer dict keys were handled in #243, where they were made to work in JSON via string coercion rather than being codec-specific.
This is new public API surface (a class kwarg + FieldInfo.int_key + a new __struct_encode_int_keys__ dunder) in the deliberately-small Struct-config area, so it deserves a design sign-off before it moves forward. @jcrist @provinzkraut could one of you weigh in on whether the niche justifies a standalone option, and on the JSON asymmetry? Happy to do a full correctness/test pass once the shape is settled.
Sorry, something went wrong.
…ion-tagging correctness
|
Thanks for the careful review — especially confirming the C changes are memory-clean. Correctness blocker (fixed)Fixed by teaching the union tag-scan to skip non-string keys. mpack_decode_struct_union Added union tests:
array_like + int_keys (now errors)int_keys together with array_like=True now raises ValueError at class creation JSON asymmetry (now implemented via string coercion)Rather than leave the asymmetry, we implemented the #243-style approach, so int_keys is
No overhead for structs without int_keys (the JSON encode/decode paths only branch when Happy to change the shape (back to msgpack-only, or an explicit error on JSON) if you'd Scope / the niche it fillsFair point that the motivation undersold this. The niche isn't just "smaller" — it's
Happy to fold this framing into the PR description, and to do the full correctness/test pass |
Sorry, something went wrong.
|
Hello, just checking in, wondering if anyone maybe had a look at the updates? |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Add int_keys: integer MessagePack map keys for Structs
Motivation
There's an inherent tension between having descriptive field names on a Struct and
the size of its encoded representation — every field name is repeated, in full, in
every message. Since one of msgspec's biggest draws is its encoding efficiency,
being able to encode a Struct's fields with integer keys instead of their names is
an attractive way to shrink MessagePack messages further.
rename to single-character names helps, and is the right tool when you need a
readable/valid JSON representation. But MessagePack can do better: it supports
integer map keys directly, and even in JavaScript the common MessagePack decoders
handle integer keys gracefully. This PR makes that available.
What this adds
A new Struct configuration option, int_keys, mapping field names to integers:
Design
ignores int_keys and keeps using the (possibly renamed) field names — no error,
and the existing fast JSON path is untouched.
and applies to both codecs; keeping int_keys separate avoids overloading its
meaning, and the two compose cleanly (a field can be renamed for JSON and
int-keyed for MessagePack).
decoding uses the same IntLookup reverse-lookup that already backs integer
union tags — so this leans on well-trodden code paths rather than introducing new
ones.
for Structs without int_keys, and the encode/decode fast paths are unchanged for
them. On decode, the key type is only inspected for Structs that opt in.
with its own keys, with nothing extra required at encode/decode time. Partial maps
are allowed (unlisted fields keep their string keys); keys must be unique within a
Struct and fit in a signed 64-bit integer.
msgspec.structs.fields(...).int_key.
Compatibility, tests, docs
Fully backward compatible — no behavior change for Structs that don't use
int_keys. Includes tests (tests/unit/test_msgpack.py, tests/unit/test_struct.py)
and documentation (docs/structs.rst).
Please consider this addition to msgspec — I'm eager to discuss any amendments or
adjustments that would help it fit the project.