feat(parquet): implement RFC 0005 §3.3 canonical-JSON encoding (PR-L1) - #62
Merged
Conversation
Lands the long-deferred encoder/decoder for the storage layer's
three §3.3 columns (`attributes`, `resource_attributes`, and `body`
when `body_kind = Structured`) so the writer / reader stop rejecting
records with non-empty OTLP envelopes. Unblocks:
- the bench loader's 1:1 OTLP envelope mapping (PR-L2 follow-up
reverts the strip and restores the kvlist fixture record),
- richer OTLP corpora in CI (operators can point `bench.yml` at
fileexporter output without losing attributes),
- one of the two gates on the production OTLP receiver (the
Parquet data path is now ready; WAL-before-ack per RFC 0003
§6.5 + CLAUDE.md §3.4 remains the other).
**Engine** (`ourios_core::otlp::canonical`):
- Thin `serde_json::to_vec` / `from_slice` wrappers over
`opentelemetry-proto`'s `with-serde` derives — the same spec
mapping rotel's OTLP HTTP receiver uses on
`ExportLogsServiceRequest`. Keeps the OTLP-JSON spec
single-sourced through `opentelemetry-proto` rather than a
hand-rolled encoder that could drift.
- Round-trip tests: every `AnyValue` variant
(string / int / double / bool / bytes / array / kvlist)
encode → decode round-trips at the `AnyValue` level.
Determinism test: re-encoding the same in-memory tree
produces byte-identical bytes (RFC0006.7 carries through
the canonicalisation boundary). Empty `Vec<KeyValue>`
encodes to `[]`.
- `serde_json` added to `ourios-core`'s deps (smallest std
feature set).
**Miner** (`ourios-miner::cluster::ingest_structured`):
- The `format!("{any_value:?}")` Debug-rendering placeholder
is gone; `MinedRecord.body` for structured rows now carries
the canonical-JSON bytes the §3.3 column wants. Encoder
fallback (debug rendering + `lossy_flag = true`) survives
pathological inputs the receiver should narrow at wire
decode (e.g. `f64::NAN`).
**Writer** (`ourios-parquet::record_batch`):
- `append_attributes` routes non-empty input through
`canonical::encode_attributes`. Empty case still
short-circuits to literal `[]` (no per-row allocation on the
clean-attach hot path).
- The `StructuredBodyNotYetCanonical` rejection is gone; the
writer appends the miner-encoded body bytes verbatim.
- `BatchError::AttributesNotYetEncoded` →
`BatchError::AttributeEncode { column, count, source }` —
fires only on actual encoder failure, not on the presence
of non-empty input.
- Two existing deferred-error tests rewritten as round-trip
tests asserting the encoded bytes decode back to the
in-memory `Vec<KeyValue>` / `AnyValue`. The structured-body
test asserts the producer's canonical bytes land verbatim
in the column.
**Reader** (`ourios-parquet::reader`):
- `decode_attributes` consumes non-empty columns to recover
`Vec<KeyValue>`. Empty case short-circuits (mirrors the
writer).
- `ReaderError::AttributesNotYetDecoded` →
`ReaderError::AttributeDecode { column, row_index, source }`
— fires only on actual decoder failure (file corruption /
foreign producer).
**Test plan:**
- `cargo fmt --all --check` — clean.
- `cargo clippy --all-targets --all-features -- -D warnings` — clean.
- `cargo test --all-features` — 291 passed / 19 ignored.
- `mdbook build` — clean.
- E2E: ran `ourios-bench` on a synthetic single-line
`LogsData` containing a `kvlistValue` body (the path that
rejected before PR-L1). Writer accepts the structured row,
Parquet file lands, bench summary reports A1/C1/C2
normally. C1 PASS (0/0 non-lossy strings, structured rows
correctly excluded from the denominator).
Follow-up PR-L2 reverts the bench loader's `attributes`
strip and restores the kvlist record to the committed OTLP
fixture.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR implements RFC 0005 §3.3 canonical JSON encoding for OTLP structured-log storage. A new ChangesCanonical JSON encoding and storage
Sequence Diagram(s)sequenceDiagram
participant Producer as ourios-miner::ingest_structured
participant Canon as ourios_core::otlp::canonical
participant Writer as Parquet Writer::record_batch
participant Reader as Parquet Reader::batch_to_mined_records
Producer->>Canon: encode_any_value(body)
Canon-->>Producer: canonical JSON bytes or CanonicalJsonError
Producer->>Writer: MinedRecord with canonical body bytes
Writer->>Canon: encode_attributes(attrs)
Canon-->>Writer: canonical JSON bytes or CanonicalJsonError
Writer->>Reader: Arrow batch with JSON columns
Reader->>Canon: decode_attributes(json_string)
Canon-->>Reader: Vec<KeyValue> or CanonicalJsonError
Reader->>Reader: construct MinedRecord with decoded attributes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Implements RFC 0005 §3.3 OTLP-canonical-JSON encoding/decoding so the storage layer can persist non-empty attributes, resource_attributes, and structured body columns instead of returning deferred-encoding errors. A new ourios_core::otlp::canonical module wraps opentelemetry-proto's with-serde derives to provide encode/decode helpers; the Parquet writer/reader and the miner's structured-body path are wired through them, and the previous "not yet encoded/decoded" error variants are replaced with real encode/decode error variants that carry the underlying serde_json source.
Changes:
- Add
ourios_core::otlp::canonical(encode/decode forAnyValueandVec<KeyValue>) plusserde_jsondependency, with round-trip and determinism tests. - Replace
AttributesNotYetEncoded/StructuredBodyNotYetCanonical/AttributesNotYetDecodedwith realAttributeEncode/AttributeDecodeerror variants inourios-parquet; route non-empty attributes through the canonical helpers and append structured bodies verbatim. - Update miner
ingest_structuredto populatebodywith canonical-JSON bytes (with alossy_flag = truedebug-rendering fallback on encoder failure).
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-core/Cargo.toml | Add serde_json dependency for canonical-JSON helpers. |
| crates/ourios-core/src/otlp.rs | Introduce canonical submodule with encode_/decode_any_value, encode_/decode_attributes, CanonicalJsonError, and tests. |
| crates/ourios-miner/src/cluster.rs | Encode structured AnyValue via canonical helper; fallback to debug rendering + lossy_flag = true on encoder failure. |
| crates/ourios-parquet/src/record_batch.rs | Encode non-empty attribute lists; drop structured-body rejection; rename error variant; update tests to round-trip. |
| crates/ourios-parquet/src/reader.rs | Decode non-empty attribute columns; replace AttributesNotYetDecoded with AttributeDecode carrying row index and serde source. |
| Cargo.lock | Reflect serde_json dep on ourios-core. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ourios-parquet/src/record_batch.rs (1)
326-339:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject
BodyKind::Structuredrows whenbodyis missing.Line 336 currently allows
(BodyKind::Structured, None)and writes a NULL body. That silently persists unreconstructable structured rows.💡 Proposed fix
pub enum BatchError { + MissingBodyForStructured, ... } impl fmt::Display for BatchError { match self { + Self::MissingBodyForStructured => write!( + f, + "structured record has body = None; RFC 0005 §3.3 structured rows must carry canonical body bytes" + ), ... } } impl std::error::Error for BatchError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { match self { ... + Self::MissingBodyForStructured => None, } } } - match r.body.as_deref() { - Some(s) => self.body.append_value(s.as_bytes()), - None => self.body.append_null(), - } + match (r.body_kind, r.body.as_deref()) { + (BodyKind::Structured, None) => return Err(BatchError::MissingBodyForStructured), + (_, Some(s)) => self.body.append_value(s.as_bytes()), + (_, None) => self.body.append_null(), + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ourios-parquet/src/record_batch.rs` around lines 326 - 339, The match on r.body currently treats None the same for all BodyKind variants; change the branch so that when r.body is None and r.body_kind == BodyKind::Structured you reject the row instead of writing a NULL — e.g. detect this condition in the code path that contains the match (referencing r.body, r.body_kind, and the current self.body.append_null() call) and return or propagate an error (or otherwise skip/flag the record) with a clear message indicating a missing body for BodyKind::Structured; for other BodyKind values keep the existing behavior (append_value or append_null).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ourios-parquet/src/reader.rs`:
- Around line 357-373: The AttributeDecode errors are using the batch-local
index i (in the ReaderError::AttributeDecode for columns::ATTRIBUTES and
columns::RESOURCE_ATTRIBUTES), causing row_index to reset per batch; compute and
use the file-global row index (e.g. row_base + i or a single file_row_index
derived from the batch start offset you already have in the reader) and replace
i with that file-global index in both error constructions so multi-batch files
report correct absolute row numbers.
---
Outside diff comments:
In `@crates/ourios-parquet/src/record_batch.rs`:
- Around line 326-339: The match on r.body currently treats None the same for
all BodyKind variants; change the branch so that when r.body is None and
r.body_kind == BodyKind::Structured you reject the row instead of writing a NULL
— e.g. detect this condition in the code path that contains the match
(referencing r.body, r.body_kind, and the current self.body.append_null() call)
and return or propagate an error (or otherwise skip/flag the record) with a
clear message indicating a missing body for BodyKind::Structured; for other
BodyKind values keep the existing behavior (append_value or append_null).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 04775825-a36b-49ba-b099-2bff7cf6b819
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/ourios-core/Cargo.tomlcrates/ourios-core/src/otlp.rscrates/ourios-miner/src/cluster.rscrates/ourios-parquet/src/reader.rscrates/ourios-parquet/src/record_batch.rs
… (PR-L1) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… (PR-L1) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ourios-miner/src/cluster.rs`:
- Line 1683: The current assignment rec.body =
Some(String::from_utf8(bytes).expect("...")) changed the structured-body
contract but tests only check body.is_some(); add a colocated unit test in the
same file that constructs a non-trivial AnyValue, runs the code path that
produces rec (the code that sets rec.body in cluster.rs), and asserts rec.body
== "<expected_canonical_json>" (the exact canonical JSON string for that
AnyValue) to prevent debug/pretty-printed JSON regressions; name the test e.g.,
test_canonical_json_body_for_anyvalue and place it next to the code that assigns
rec.body so it runs as part of the module tests.
- Around line 1671-1678: The ingest path currently panics on canonical-JSON
encoding failure in MinerCluster::ingest_structured by calling
encode_any_value(...).unwrap_or_else(|e| panic!(...)); change this to a
recoverable error flow (return/propagate a Result or log and mark the record as
failed rather than aborting the process) so encoding failures do not abort the
process, and update the ingest_structured call-sites to handle the new Result.
Add a colocated unit test that constructs a structured record, computes the
expected canonical bytes via
ourios_core::otlp::canonical::encode_any_value(any_value) and asserts the
miner-produced rec.body equals those exact bytes (ensuring the emitted body is
OTLP-canonical JSON), and include a regression case that would have triggered
the previous panic to ensure the new code returns/handles an error instead of
panicking.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0dab3466-0b20-4c17-acfe-ab4adeafa5b8
📒 Files selected for processing (1)
crates/ourios-miner/src/cluster.rs
… (PR-L1) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… (PR-L1) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
jensholdgaard
added a commit
that referenced
this pull request
May 29, 2026
…2) (#63) PR-K4 stripped `attributes` / `resource_attributes` in the bench loader and removed the kvlist record from the committed OTLP fixture, both as workarounds for the deferred RFC 0005 §3.3 canonicalisation. PR-L1 (#62) landed the canonicalisation encoders/decoders end-to-end, so the workarounds can go: - `map_log_record` no longer empties `attributes` / `resource_attributes`; both fields map verbatim from the wire per RFC 0003 §6.6. The resource-attrs per-`ResourceLogs` hoist is restored alongside. - `crates/ourios-bench/tests/data/otlp/sample.jsonl` regains its kvlist body record; the fixture is now 3 LogsData lines / 4 records (1 + 2 + 1) as PR-K2 originally shipped it. - Loader test reverts: `loads_otlp_corpus_envelope_one_to_one` asserts `first.attributes.len() == 1` and `first.resource_attributes.len() == 2` again, and the structured-body test reads the fixture's 4th record (the inline-synthetic workaround test goes away too). - Module / `ingest_otlp_jsonl` / `map_log_record` rustdocs lose the "stripped pending §3.3" caveats now that the writer carries the envelope through. Local end-to-end on the restored fixture: 4 records ingest + write + read cleanly. C1 = 1.000000 (3/3 non-lossy strings; the kvlist record is correctly excluded from C1's denominator per RFC 0001 §6.4 — that exclusion landed in PR-K4 and stays). Test plan: cargo fmt / clippy / test --all-features (292 passed / 19 ignored, same count: -1 inline-synthetic test, +1 fixture-restored assertion). `mdbook build` clean. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 tasks
This was referenced Jun 9, 2026
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.
Summary
Lands the long-deferred RFC 0005 §3.3 encoder/decoder so the storage layer's three canonical-JSON columns (
attributes,resource_attributes, andbodywhenbody_kind = Structured) stop rejecting records with non-empty OTLP envelopes.Unblocks (in order):
bench.ymlat a real fileexporter output without losing attributes.Design
ourios_core::otlp::canonicalis a thin wrapper overopentelemetry-proto'swith-serdederives — the same spec mapping rotel's OTLP HTTP receiver uses onExportLogsServiceRequest. Keeps the OTLP-JSON spec single-sourced throughopentelemetry-protorather than a hand-rolled encoder that could drift.Four
pub fns, allResult<…, CanonicalJsonError>:Encoder failure is a wire-decode bug (e.g. an
f64::NANin aDoubleValue) — the production receiver narrows that at wire decode, so the fallback path iningest_structured(debug rendering +lossy_flag = true) is defence in depth, not a hot path.Touch points
ourios-coreotlp::canonicalmodule +serde_jsondep + 4 round-trip tests (everyAnyValuevariant, attribute lists, encoder determinism for RFC0006.7, empty-input sentinel)ourios-mineringest_structuredreplacesformat!("{any_value:?}")placeholder withencode_any_value; encoder-error fallback flags the row lossy so the reader returns the debug bytes verbatimourios-parquet(writer)append_attributesroutes non-empty input throughencode_attributes; structured-body rejection gone;AttributesNotYetEncoded→AttributeEncode { source }; two deferred-error tests rewritten as encode → decode round-trip testsourios-parquet(reader)decode_attributesconsumes non-empty columns; empty short-circuit mirrors the writer;AttributesNotYetDecoded→AttributeDecode { row_index, source }Test plan
cargo fmt --all --checkcargo clippy --all-targets --all-features -- -D warningscargo test --all-features— 291 passed / 19 ignored (+6 vs main: 4 canonical-helper tests + 2 round-trip tests replacing the old deferred-error tests)mdbook buildourios-benchon a synthetic single-lineLogsDatawhose body is akvlistValue(the exact path that rejected before this PR). Writer accepts the structured row, Parquet file lands, bench summary reports A1/C1/C2 normally. C1 = 1.000000 (0/0 non-lossy strings — structured rows correctly excluded from the denominator).Follow-up
attributes/resource_attributesstrip + restores the kvlist record to the committed OTLP fixture + RFC 0006 §3.1 amendment revert.ourios-walRFC + implementation, then RFC 0003 →specified→ implementation). This PR closes the storage-side gap.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests