fix(core): make the canonical-encoding f64 round-trip bit-exact (local half of #130) - #176
Conversation
…local half of #130) decode(encode(x)) on a DoubleValue drifted 1-2 ULP for ~12% of arbitrary finite f64 through the Ourios canonical body encoding, breaking RFC 0001 §6.1's stored_bytes <-> AnyValue faithfulness (which lossy_flag = false rests on; cf. RFC 0005 §3.3). Empirical probing relocated the bug from where #130 placed it: the encoder (with-serde via serde_json) already emits shortest round-trip digits — for the repro value -1.5374084650425255e99 the emitted bytes parse back exactly with std's parser. The lossy half is serde_json's *default float parser*, which is approximate by design; its `float_roundtrip` feature exists precisely to make parsing correctly rounded. Enabling it makes the round-trip bit-exact for every finite f64 (verified over 200k random bit patterns: 0 drift), so no hand-rolled encoder is needed and the encode path — and every stored byte shape — is untouched. - ourios-core: serde_json += `float_roundtrip`, documented as load-bearing, not an optimisation preference - regression test pinning the #130 repro value and -0.0 sign-bit preservation, bit-for-bit via to_bits - proptest property: arbitrary finite f64 (raw u64 bit patterns, subnormals and -0.0 included) round-trips bit-exactly at top level and nested inside array / kvlist - pins the (empirically captured) non-finite encoding {"doubleValue":null} — NaN/±Inf bytes that do not decode back, a known pre-existing gap left out of scope - RFC 0001 §6.1: dated amendment + a `double` bullet in the rule; the existing byte-shape test passes unchanged (encoder bytes are identical before and after) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR establishes a bit-exact round-trip guarantee for finite ChangesCanonical f64 round-trip guarantee
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
This PR strengthens Ourios’s “canonical body encoding” invariants by making finite f64 values round-trip bit-exactly through the canonical JSON encode/decode boundary (local half of #130). It does so by enabling serde_json’s correctly-rounded float parsing (float_roundtrip) while keeping the emitted byte shape unchanged, and it adds regression/property tests plus RFC/docs clarifications (including pinning the current non-finite f64 → null encoding behavior).
Changes:
- Enable
serde_json’sfloat_roundtripfeature inourios-core(documented as load-bearing for RFC 0001 §6.1 faithfulness). - Update canonical encoding docs and RFC 0001 §6.1 to explicitly describe shortest-round-trip float emission + correctly-rounded parsing, and to note the known non-finite gap.
- Add targeted regression tests and a proptest property to validate bit-exact finite-
f64round-trips, plus a test pinning the non-finite encode byte shape.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| docs/rfcs/0001-template-miner.md | RFC amendment documenting exact finite-f64 round-trip requirements and the non-finite null gap. |
| crates/ourios-core/src/otlp.rs | Docs updated for f64 round-trip behavior; adds regression + property tests for bit-exact finite doubles and pins non-finite encoding. |
| crates/ourios-core/Cargo.toml | Enables serde_json float_roundtrip; adds proptest dev-dependency with rationale. |
| Cargo.lock | Locks new dev dependency (proptest). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ror text The non-finite test now pins both halves of the gap (null shape emitted AND decode rejects). Error Display + doc mentions say Ourios-canonical, matching the post-#166 naming. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
crates/ourios-core/src/otlp.rs:120
- The
Body::Structureddoc comment says the current miner still stores aDebugrendering ofAnyValueas a placeholder, butourios-miner::cluster::ingest_structurednow canonical-encodes theAnyValueviaourios_core::otlp::canonical::encode_any_valueand stores those JSON bytes. This doc is now incorrect and could mislead implementers of the exporter/reconstruction contract.
/// The *current* miner implementation (in
/// `ourios-miner::cluster::ingest_structured`) writes the
/// `Debug` rendering of the decoded `AnyValue` (`format!(
/// "{any_value:?}")`) as an interim placeholder — the
/// canonicalisation PR replaces it before any wire-export
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ology Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
What
Makes the Ourios canonical body encoding round-trip every finite
f64bit-exactly — the local half of #130 (the upstreamopentelemetry-protoquestion is tracked there; this PR deliberately does not auto-close it).Diagnosis correction (empirically verified)
#130 attributed the 1–2 ULP drift to a lossy
with-serdedouble formatter. Probing the actual bytes relocated the bug:serde_jsonemit byte-identical output for doubles (e.g. the repro value emits{"doubleValue":-1.5374084650425255e+99}on both) — and those digits are correct shortest-round-trip:"-1.5374084650425255e99".parse::<f64>()recovers the exact bits.with-serdehas no custom double formatter.serde_json's default float parser, which is approximate by design. Itsfloat_roundtripfeature exists precisely for this; with it enabled, 200k random finite-f64bit patterns round-trip with 0 drift, repro value included.So instead of hand-rolling a canonical encoder (drafted, then discarded: it emitted byte-identical output to the derive and fixed nothing), the fix is the decode-side feature flag. The encode path and every stored byte shape are untouched — the #167 byte-shape test (
encoder_emits_exact_canonical_bytes_per_variant) passes unchanged, as do all prior round-trip/determinism tests.Changes
ourios-core/Cargo.toml:serde_json+=float_roundtrip, with a comment marking it load-bearing for the §6.1 faithfulness guarantee (not an optimisation preference).proptestadded as a dev-dependency (workspace-consistent"1").otlp.rsmodule docs: explain that encode was already shortest-round-trip, decode requiresfloat_roundtrip, and non-finite doubles are a separate pre-existing gap. Corrects an inaccurate doc claim thatwith-serdemapsf64::NANto a"NaN"string — empirically it emits{"doubleValue":null}.-1.5374084650425255e99, plus-0.0(sign bit preserved — verified viato_bits),0.0,f64::MAX,f64::MIN_POSITIVE, and the smallest subnormal round-trip bit-exactly;finite_doubles_round_trip_bit_exact: arbitrary finitef64from rawu64bit patterns, round-tripped bit-exactly at top level and nested insidearrayValue/kvlistValue;nonfinite_doubles_encode_to_the_null_shape: pins the empirically captured{"doubleValue":null}bytes forNaN/+Inf/-Infso an upstream change can't silently drift stored bytes.doublebullet in the encoding rule. RFC 0005 §3.3 defers to §6.1 and needed no edit.Invariant / hazard statement (CLAUDE.md §4)
This touches hazard 7 territory (faithful reconstruction) via RFC 0001 §6.1's structured-body faithfulness:
stored_bytes ↔ AnyValue, whichlossy_flag = falserests on. The change strengthens that invariant (finite doubles now round-trip bit-exactly) without altering any stored byte: encode output is byte-identical before and after, so existing files and dedup behaviour are unaffected, and old stored bytes decode exactly under the new parser.Known gap surfaced (pre-existing, out of scope, now pinned by a test): non-finite doubles (
NaN, ±∞) encode to{"doubleValue":null}— bytes the decoder rejects, so the §6.1 faithfulness guarantee does not hold for them today and didn't before this PR. Flagged in the RFC amendment; likely warrants its own issue/OTel-spec question (proto3 JSON maps non-finite floats to the strings"NaN"/"Infinity"/"-Infinity", whichwith-serdedoes not implement forAnyValue).Verification
cargo test --all-features— 607 passed; 0 failed; 24 ignored (whole workspace)cargo fmt --all --check— cleancargo clippy --all-targets --all-features -- -D warnings— cleancargo doc --workspace --no-deps --all-features— cleanmdbook build— clean (pre-existing mdbook-mermaid version warning only)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests / Dev tooling