feat(parquet)!: rfc 0025 green a — body_kind ordinal 2 for absent bodies - #380
Conversation
The RFC 0005 §3.2 amendment: BodyKind::Absent writes ordinal 2 with a NULL body cell; the reader accepts it; UnsupportedAbsentBody is retired (the writer's remaining loud rejection is timestamp overflow). The miner drops lossy_flag on absent rows — absence is not loss, reconstruction renders nothing exactly (RFC 0025 §3.1). The RFC 0024 P1 pinned-rejection arm flips to a round-trip. Discharges RFC0025.1 and RFC0025.2 (committed pre-amendment fixture generated with the two-ordinal writer). BREAKING CHANGE: files containing body_kind ordinal 2 are unreadable by pre-amendment readers (RFC0005.14 unknown-ordinal rejection); upgrade readers before writers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughBodyKind::Absent is now supported end-to-end: the parquet writer maps it to ordinal 2 instead of raising BatchError::UnsupportedAbsentBody, the reader documents ordinal 2, MinerCluster::ingest marks absent bodies as non-lossy, and RFC0025 tests were converted from stubs to real round-trip and fixture-parity assertions. ChangesAbsent body round-trip support
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
Implements RFC 0025’s first green slice by making BodyKind::Absent persistable in Parquet via body_kind ordinal 2 with a NULL body cell, updates read/write semantics accordingly, and flips previously-pinned rejection tests to round-trip/parity assertions.
Changes:
- Parquet writer: encode
BodyKind::Absentas ordinal2(retiringUnsupportedAbsentBody) and update unit tests accordingly. - Parquet reader: decode ordinal
2toBodyKind::Absentwhile keeping unknown ordinals as hard errors. - Miner + tests: treat absent body as non-lossy (
lossy_flag = false) and add RFC 0025 scenario tests + update RFC 0024 property suite expectations.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-parquet/tests/rfc0025_absent_body.rs | Adds RFC 0025 scenario tests for absent-body round-trip and pre-amendment fixture parity. |
| crates/ourios-parquet/tests/rfc0024_properties.rs | Removes the “absent body must reject” arm; absent bodies now must round-trip. |
| crates/ourios-parquet/src/record_batch.rs | Retires UnsupportedAbsentBody; makes body_kind_ordinal infallible and maps Absent → 2; updates writer tests. |
| crates/ourios-parquet/src/reader.rs | Decodes body_kind ordinal 2 as BodyKind::Absent; updates unknown-ordinal error detail. |
| crates/ourios-miner/src/cluster.rs | Emits absent-body records with lossy_flag = false and updates the corresponding test assertion. |
💡 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)
395-420: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNo write-time guard that
BodyKind::Absentimpliesbody = None.
Builders::appendenforces the String-kind body/separator invariants explicitly (InvalidSeparatorsForString,MissingBodyForLossyString), but there's no equivalent check forBodyKind::Absent. If a future producer ever setsbody_kind = Absentwithbody = Some(...), the writer will silently persist ordinal2with a non-NULL body cell, violating the exact contract this PR discharges ("wire-absent rows carry aNULLbody cell", per thebody_kind_ordinaldoc comment at Line 521). TodayMinerCluster::ingestalways leavesbody = Nonefor absent records, so this isn't reachable in practice, but the same defense-in-depth reasoning that motivatedMissingBodyForLossyStringapplies here.🛡️ Proposed guard mirroring the existing String-kind pattern
+ if r.body_kind == BodyKind::Absent && r.body.is_some() { + return Err(BatchError::UnexpectedBodyForAbsent); + } match r.body.as_deref() { Some(s) => self.body.append_value(s.as_bytes()), None => self.body.append_null(), }(would also need a new
BatchError::UnexpectedBodyForAbsentvariant +Display/sourcearms.)Also applies to: 435-449
🤖 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 395 - 420, Add a write-time validation in Builders::append for BodyKind::Absent so it explicitly rejects rows where r.body is Some, matching the existing String-kind invariant checks. Introduce a BatchError variant such as UnexpectedBodyForAbsent and wire it through the Display/source handling, then have the append path return that error before appending body_kind_ordinal and body so Absent rows can only persist with a NULL body cell.
🧹 Nitpick comments (3)
crates/ourios-parquet/tests/rfc0024_properties.rs (1)
93-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale comment: "two documented loud rejections" is now inconsistent.
This comment predates the change and still claims two loud rejections, contradicting the updated module doc (lines 81-82: "everything else... must round-trip") and the updated inline comment at lines 105-107 ("the writer's one remaining documented loud rejection"). Update it to avoid confusing future readers about the writer's actual contract.
📝 Proposed fix
- // The writer's two documented loud rejections. Everything else + // The writer's one remaining documented loud rejection. Everything else // must round-trip.🤖 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/tests/rfc0024_properties.rs` around lines 93 - 96, The inline comment in the properties test is stale and still mentions “two documented loud rejections,” which no longer matches the writer contract described in the surrounding test comments and module docs. Update the comment near the grouping-by-partition logic in rfc0024_properties.rs to reflect the current behavior of the writer, using the same wording as the rest of the test that describes everything else round-tripping and the single remaining documented loud rejection.crates/ourios-parquet/tests/rfc0025_absent_body.rs (1)
99-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMagic
0fortemplate_id/template_versionvs. doc comment's "NO_TEMPLATE".The doc comment says the absent record uses
NO_TEMPLATE, suggesting a named constant may exist elsewhere for this sentinel. Using a shared constant (if one exists) instead of the literal0here would keep this test aligned with the producer-side convention if it ever changes.#!/bin/bash # Check whether a NO_TEMPLATE constant exists for template_id/template_version. rg -nP 'NO_TEMPLATE' crates/ourios-core/src crates/ourios-miner/src🤖 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/tests/rfc0025_absent_body.rs` around lines 99 - 116, The absent-body test is hardcoding the NO_TEMPLATE sentinel as 0 for template_id and template_version, which should be aligned with the shared convention instead. Update absent_record to use the existing NO_TEMPLATE constant if one is defined in the producer/miner codebase, and keep the test referencing that symbol so it stays consistent with the BodyKind::Absent and MinedRecord contract if the sentinel ever changes.crates/ourios-parquet/src/record_batch.rs (1)
143-199: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueRemove stale
UnsupportedAbsentBodyreferences.docs/rfcs/0025-absent-body-representation.mdandcrates/ourios-parquet/src/record_batch.rs:825still mention the deleted variant.🤖 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 143 - 199, Stale references to the deleted BatchError variant remain in the absent-body RFC doc and in record_batch logic, so update those spots to use the current error model instead. In crates/ourios-parquet/src/record_batch.rs, replace any UnsupportedAbsentBody handling with the existing MissingBodyForLossyString path (or the appropriate current variant) and remove any dead match arms or comments that still mention the removed variant. Also revise docs/rfcs/0025-absent-body-representation.md so it no longer documents UnsupportedAbsentBody and reflects the current enum names from BatchError.
🤖 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/tests/rfc0025_absent_body.rs`:
- Around line 68-95: The structured fixture row in rfc0025_absent_body should
not be marked lossy; update the MinedRecord setup for the BodyKind::Structured
case so lossy_flag is false. Keep the canonical-JSON body bytes unchanged, and
ensure this fixture matches the structured-body contract used by the RFC0025
test data.
---
Outside diff comments:
In `@crates/ourios-parquet/src/record_batch.rs`:
- Around line 395-420: Add a write-time validation in Builders::append for
BodyKind::Absent so it explicitly rejects rows where r.body is Some, matching
the existing String-kind invariant checks. Introduce a BatchError variant such
as UnexpectedBodyForAbsent and wire it through the Display/source handling, then
have the append path return that error before appending body_kind_ordinal and
body so Absent rows can only persist with a NULL body cell.
---
Nitpick comments:
In `@crates/ourios-parquet/src/record_batch.rs`:
- Around line 143-199: Stale references to the deleted BatchError variant remain
in the absent-body RFC doc and in record_batch logic, so update those spots to
use the current error model instead. In
crates/ourios-parquet/src/record_batch.rs, replace any UnsupportedAbsentBody
handling with the existing MissingBodyForLossyString path (or the appropriate
current variant) and remove any dead match arms or comments that still mention
the removed variant. Also revise docs/rfcs/0025-absent-body-representation.md so
it no longer documents UnsupportedAbsentBody and reflects the current enum names
from BatchError.
In `@crates/ourios-parquet/tests/rfc0024_properties.rs`:
- Around line 93-96: The inline comment in the properties test is stale and
still mentions “two documented loud rejections,” which no longer matches the
writer contract described in the surrounding test comments and module docs.
Update the comment near the grouping-by-partition logic in rfc0024_properties.rs
to reflect the current behavior of the writer, using the same wording as the
rest of the test that describes everything else round-tripping and the single
remaining documented loud rejection.
In `@crates/ourios-parquet/tests/rfc0025_absent_body.rs`:
- Around line 99-116: The absent-body test is hardcoding the NO_TEMPLATE
sentinel as 0 for template_id and template_version, which should be aligned with
the shared convention instead. Update absent_record to use the existing
NO_TEMPLATE constant if one is defined in the producer/miner codebase, and keep
the test referencing that symbol so it stays consistent with the
BodyKind::Absent and MinedRecord contract if the sentinel ever changes.
🪄 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: 0f07c821-fc86-4526-b10e-0d233b39438f
⛔ Files ignored due to path filters (1)
testdata/rfc0025/pre-amendment.parquetis excluded by!**/*.parquet
📒 Files selected for processing (5)
crates/ourios-miner/src/cluster.rscrates/ourios-parquet/src/reader.rscrates/ourios-parquet/src/record_batch.rscrates/ourios-parquet/tests/rfc0024_properties.rscrates/ourios-parquet/tests/rfc0025_absent_body.rs
…ured lossy flag NonNullBodyForAbsent guards the §3.1 NULL-cell contract's other direction (with a unit test); the fixture's structured row now carries lossy_flag = false per RFC 0001 §6.1 (regenerated). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
First RFC 0025 green slice, discharging RFC0025.1 (absent-body round-trip) and RFC0025.2 (old-file parity).
body_kind_ordinalbecomes infallible withAbsent → 2(NULL body cell);BatchError::UnsupportedAbsentBodyis fully retired — the writer's one remaining documented loud rejection is timestamp overflow. The old rejection unit test flips to assert ordinal 2 + NULL body (contract change approved via RFC 0025).BodyKind::Absent; unknown ordinals still reject per RFC0005.14.lossy_flag— absence is not loss; reconstruction is defined and total (renders nothing). The pinned unit test flips with the RFC citation.testdata/rfc0025/pre-amendment.parquetgenerated with the two-ordinal writer before the change (provenance note in the generator test); .2 asserts byte-level parity through the amended reader.Invariants (§3.5 migration plan)
Additive-only schema change: old files never carry ordinal 2 and read identically (pinned by .2). Old readers encountering new files with ordinal 2 error loudly via the existing unknown-ordinal posture — hence the conventional-commit
!: upgrade readers before writers. No historical rewrite, no new columns.This closes the absent-body half of #362; the sink-quarantine half lands in green slice c.
Verification
cargo fmt --all --checkclean; workspace clippy clean (RC checked)cargo test --all-features: 128 suites, 916 passed, 0 failed — including the flipped P1 property over generated absent bodies🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes