repr: pin the PackedNumeric layout to its durable byte offsets - #38402
Open
antiguru wants to merge 1 commit into
Open
repr: pin the PackedNumeric layout to its durable byte offsets#38402antiguru wants to merge 1 commit into
antiguru wants to merge 1 commit into
Conversation
`PackedNumeric` is a `FixedSizeCodec` over forty bytes, so its layout is durable. It stores the coefficient units in the twenty-six bytes at offset eight, the flags in the byte at offset thirty-four, and leaves the last five unused. The encoder wrote thirteen units because that is what fits, but nothing said so, and nothing tied it to `NUMERIC_DATUM_WIDTH`. That coupling is a hazard in one direction. A wider `Numeric` would write its coefficient over the flags byte, which still fits inside `SIZE`, so the encoder would neither fail to compile nor panic; it would corrupt persisted values, and since the flags carry the sign, a negative value would come back positive. Name the offsets, derive the unit count from them, and assert at compile time that the datum width matches, so changing the width is a build failure that says what it needs instead of a silent format change. The encoded bytes are unchanged, which `packed_numeric_stability` covers. Add `packed_numeric_max_precision_keeps_flags`, which round-trips the largest positive and negative values through the codec. `proptest_packed_numeric_ roundtrip` and `packed_numeric_stability` already fail on an overrun, but only for inputs that happen to use the last unit; this names the case and checks the sign explicitly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
PackedNumericis aFixedSizeCodecover forty bytes, so its layout is durable. It stores the coefficient units in the twenty-six bytes at offset eight, the flags in the byte at offset thirty-four, and leaves the last five unused. The encoder wrote thirteen units because that is what fits, but nothing in the code said so, and nothing tied it toNUMERIC_DATUM_WIDTH, which is where the thirteen comes from.The coupling is a hazard in one direction. A wider
Numericwould write its coefficient over the flags byte, which still fits insideSIZE, so the encoder would neither fail to compile nor panic. It would corrupt persisted values, and because the flags carry the sign, a negative value would come back positive.This names the offsets, derives the unit count from them, and asserts at compile time that the datum width matches. Changing the datum width then fails the build and says what it needs, a new packed encoding and a migration, rather than silently changing a durable format.
The encoded bytes are unchanged, which the existing
packed_numeric_stabilitytest covers.Tests
Adds
packed_numeric_max_precision_keeps_flags, which round-trips the largest positive and negative values through the codec and checks the sign survives.proptest_packed_numeric_roundtripandpacked_numeric_stabilityalready fail on an overrun, so the new test is not the only guard; it names the specific failure mode and does not depend on a random input happening to use the last unit. All three were confirmed to fail when the encoder is modified to overrun the flags byte, and to pass once it is not.Note for reviewers
bin/lintfails on this branch incheck-pipeline-topics.shandcheck-python-files.sh. Both concern files this change does not touch, reporting a"LiteralString" is unknown import symbolpyright error in twomzcompose.pyfiles and a traceback insidelint_pipeline_topics.py. They look like local Python tooling problems rather than anything from this change; CI will confirm.🤖 Generated with Claude Code