feat(streaming): feed the accumulators one record at a time (#102) - #107
Conversation
`--streaming` read the whole file, parsed a full DOM, then materialised a Vec<JsonEvent> beside it — 402 MB against the DOM path's 233 MB on a 15 MB input. The sketch accumulators were already bounded; what was missing was a way to feed them. input DOM --streaming 15 MB / 120k 227 MB 7.2 MB 32x 142 MB / 1.2M 2,231 MB 7.7 MB 288x Ten times the input, the same memory: bounded, not merely smaller. vajra-core::records pulls top-level records one at a time — NDJSON through StreamDeserializer, a top-level array element by element through a SeqAccess visitor, which is the shape the docs use as the large-input example and the one that is not simply line-splitting. emit_record_events converts each into a reused buffer rooted at `$[*]`, so paths match a whole-document walk; there is a test asserting the value events are identical. Two limits are real and stated rather than implied. Memory is bounded by the largest single record, not the file. And stdin, URLs, git repositories and the non-JSON formats go through readers that produce a whole document by construction — passing --streaming to those now says the input was loaded whole rather than silently doing nothing. --- A wrong number this surfaced With a real record stream the exact-tracking threshold is actually reached, and the fallback was reporting the top-k buffer size as cardinality: 100 distinct values for a field with 120,000, and entropy log2(100) = 6.643856 against a true 16.87 — in the same fields the DOM path uses for measurements. Those paths now carry `exact: false`, omitted when true so DOM output is unchanged. `cardinality` is documented as a lower bound, which is provable: k occupied counters means at least k distinct values were seen. No bound is claimed for entropy — Space-Saving admits an evicted item at min_count + 1, so it over-attributes rather than cleanly grouping, and calling it a bound would be a claim I cannot prove. I wrote it as one first and removed it after checking the eviction rule. A distinct-count sketch is #106. Below the threshold streaming is not an approximation at all: entropy agrees with the DOM path to 1e-12, asserted. OUTPUT_SCHEMA_VERSION goes to 2 for the added field — the first use of the versioning added in #104. The "does not yet bound memory" warning from #103 is removed and the test that asserted it is replaced by its inverse, since a stale caveat is its own kind of false claim. CLAUDE.md rule 1 now says partially met, with the measurement. Closes #102
|
Warning Review limit reached
Next review available in: 45 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 selected for processing (7)
📝 WalkthroughWalkthrough
ChangesStreaming statistics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant RecordReader
participant EventBuffer
participant StatsAccumulator
participant JSONOutput
CLI->>RecordReader: detect and decode JSON or NDJSON records
RecordReader->>EventBuffer: provide one record
EventBuffer->>StatsAccumulator: emit reusable record events
StatsAccumulator->>JSONOutput: return exact or approximate path statistics
JSONOutput-->>CLI: serialize exact marker when false
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
vajra-core/src/records.rs (3)
158-186: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused
PhantomDatamarker onArraySeed.
ArraySeed<F>already ownsf: F, which fully constrains the generic parameter;_marker: PhantomData<fn()>serves no purpose here (there's no unconstrained lifetime or zero-sized use ofFto anchor). As per coding guidelines, "Avoid dead code and speculative abstractions; prefer a few duplicated lines over a premature abstraction."♻️ Proposed cleanup
struct ArraySeed<F> { f: F, - _marker: PhantomData<fn()>, } @@ ArraySeed { f, - _marker: PhantomData, }(also drop the now-unused
use std::marker::PhantomData;import)🤖 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 `@vajra-core/src/records.rs` around lines 158 - 186, Remove the unused _marker: PhantomData<fn()> field from ArraySeed and delete its initializer when constructing ArraySeed before deserialize. Also remove the now-unused std::marker::PhantomData import, leaving the f field and deserialization behavior unchanged.Source: Coding guidelines
262-288: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't actually verify incremental decoding.
The docstring claims the callback "sees record 1 before record N exists," but the assertions only check that record
i==0arrives atcount==0— true for any implementation that visits records in order, including one that fully buffers the array first and then iterates. This doesn't distinguish genuine streaming from a DOM-then-iterate fallback, which is the exact regression#102is meant to prevent.Consider wrapping the reader in a small byte-counting
Readadapter and asserting the first callback fires having consumed fewer bytes than the full file — that would actually demonstrate incrementality.🤖 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 `@vajra-core/src/records.rs` around lines 262 - 288, Strengthen records_are_visited_incrementally by wrapping the input reader in a byte-counting Read adapter and recording the bytes consumed when the first callback runs. Assert that this count is less than the complete JSON file size, while retaining the existing record-count and ordering assertions; update the test setup or helper invocation as needed to pass the wrapped reader through the decoding path.
188-289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest module uses
.expect()/panic!(), contrary to the project's error-handling guideline.As per coding guidelines,
**/*.rs: "Do not useunwrap(),expect(), orpanic!(); errors must include enough context to diagnose the failure without a debugger." This test module leans on.expect("create"),.expect("write"),.expect("stream"), and an explicitpanic!("expected a parse error, got {other:?}")(line 258), whereas the siblingstream.rstests in this same PR returnResult<(), Box<dyn std::error::Error>>and propagate failures with?. Aligning this file with that established pattern would both satisfy the guideline and match the local convention.🤖 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 `@vajra-core/src/records.rs` around lines 188 - 289, Update the tests module in records.rs to return Result<(), Box<dyn std::error::Error>> and propagate setup, write, streaming, and assertion-related errors with ?. Remove all expect calls and the explicit panic in malformed_json_reports_the_source_file, preserving its parse-error matching and validation while returning an error for unexpected variants.Source: Coding guidelines
🤖 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 `@docs/src/commands.md`:
- Line 65: Update the --streaming option description in the commands
documentation to state that it is supported only by stats for JSON/NDJSON file
inputs. Replace the broad “Stream records” wording while preserving the
bounded-memory sketch-based statistics context.
In `@docs/src/streaming.md`:
- Around line 3-22: Update the streaming memory-bound documentation around the
Status section and the corresponding claims at the referenced locations to state
that memory scales with the maximum record/event-buffer size, O(distinct paths),
and configured sketch state. Qualify the flat-RSS measurements and “same memory”
claim as applying to a fixed schema, and avoid claiming memory is bounded solely
by the largest record.
In `@vajra-cli/src/main.rs`:
- Around line 1802-1807: Update the text, Markdown, and Compact AI renderers for
the result type containing the exact field so exact results are labeled EXACT
and inexact results are labeled APPROXIMATE. Preserve the existing JSON
serialization behavior and ensure every rendered path that shows cardinality or
entropy includes the marker.
- Around line 969-972: Update the record-processing flow around detect_shape and
for_each_record so top-level JSON objects retain DOM-mode root path semantics
instead of being treated as NDJSON records rooted at $[*]. Stream only top-level
arrays or actual NDJSON, or pass explicit singleton-document root handling
through the record-event path while preserving existing array and NDJSON
behavior.
In `@vajra-cli/tests/streaming_bounded.rs`:
- Around line 11-53: Remove all expect()-based failure handling from the
streaming test helpers, including vajra, json, and corpus, and make them return
Result types with contextual errors. Update the affected tests through the
covered range to return Result and propagate helper, file, write, command,
status, and JSON parsing failures with ?. Preserve the existing assertions’
failure context without using unwrap(), expect(), or panic!().
In `@vajra-core/src/records.rs`:
- Around line 48-66: Update the documentation for detect_shape to explicitly
state that its first-non-whitespace-byte heuristic cannot distinguish a JSON
array document from NDJSON whose records are arrays, and that array-prefixed
NDJSON may be classified as JsonArray. Preserve the existing detection behavior
and parsing flow.
---
Nitpick comments:
In `@vajra-core/src/records.rs`:
- Around line 158-186: Remove the unused _marker: PhantomData<fn()> field from
ArraySeed and delete its initializer when constructing ArraySeed before
deserialize. Also remove the now-unused std::marker::PhantomData import, leaving
the f field and deserialization behavior unchanged.
- Around line 262-288: Strengthen records_are_visited_incrementally by wrapping
the input reader in a byte-counting Read adapter and recording the bytes
consumed when the first callback runs. Assert that this count is less than the
complete JSON file size, while retaining the existing record-count and ordering
assertions; update the test setup or helper invocation as needed to pass the
wrapped reader through the decoding path.
- Around line 188-289: Update the tests module in records.rs to return
Result<(), Box<dyn std::error::Error>> and propagate setup, write, streaming,
and assertion-related errors with ?. Remove all expect calls and the explicit
panic in malformed_json_reports_the_source_file, preserving its parse-error
matching and validation while returning an error for unexpected variants.
🪄 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: 01687ff4-0270-4b7d-abf5-5a26cce8ef1d
📒 Files selected for processing (12)
CLAUDE.mddocs/src/commands.mddocs/src/streaming.mdvajra-cli/src/main.rsvajra-cli/tests/format_honesty.rsvajra-cli/tests/streaming_bounded.rsvajra-core/src/lib.rsvajra-core/src/records.rsvajra-core/src/stream.rsvajra-stats/src/analyzer.rsvajra-stats/src/streaming.rsvajra-types/src/lib.rs
| fn vajra(args: &[&str]) -> std::process::Output { | ||
| Command::new(env!("CARGO_BIN_EXE_vajra")) | ||
| .args(args) | ||
| .output() | ||
| .expect("vajra invocation failed") | ||
| } | ||
|
|
||
| fn json(args: &[&str]) -> serde_json::Value { | ||
| let out = vajra(args); | ||
| assert!( | ||
| out.status.success(), | ||
| "vajra {args:?} failed: {}", | ||
| String::from_utf8_lossy(&out.stderr) | ||
| ); | ||
| serde_json::from_slice(&out.stdout).expect("invalid JSON") | ||
| } | ||
|
|
||
| /// `records` rows with `distinct` distinct values in `v`, so the exact/sketch | ||
| /// boundary can be crossed deliberately. | ||
| fn corpus( | ||
| dir: &std::path::Path, | ||
| name: &str, | ||
| records: usize, | ||
| distinct: usize, | ||
| ndjson: bool, | ||
| ) -> String { | ||
| let path = dir.join(name); | ||
| let mut f = std::fs::File::create(&path).expect("create"); | ||
| if ndjson { | ||
| for i in 0..records { | ||
| writeln!(f, r#"{{"v":"item{}","n":{}}}"#, i % distinct, i).expect("write"); | ||
| } | ||
| } else { | ||
| write!(f, "[").expect("write"); | ||
| for i in 0..records { | ||
| if i > 0 { | ||
| write!(f, ",").expect("write"); | ||
| } | ||
| write!(f, r#"{{"v":"item{}","n":{}}}"#, i % distinct, i).expect("write"); | ||
| } | ||
| write!(f, "]").expect("write"); | ||
| } | ||
| path.to_str().expect("utf-8 path").to_owned() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove panic-based test setup.
The new test helpers use expect() throughout. Return Result from helpers/tests and propagate failures with ? so failures retain context without violating the no-expect() rule.
As per coding guidelines, “Do not use unwrap(), expect(), or panic!().”
Also applies to: 70-201
🤖 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 `@vajra-cli/tests/streaming_bounded.rs` around lines 11 - 53, Remove all
expect()-based failure handling from the streaming test helpers, including
vajra, json, and corpus, and make them return Result types with contextual
errors. Update the affected tests through the covered range to return Result and
propagate helper, file, write, command, status, and JSON parsing failures with
?. Preserve the existing assertions’ failure context without using unwrap(),
expect(), or panic!().
Source: Coding guidelines
… the claims CodeRabbit on #107. Four findings, all correct; one was a correctness bug I introduced. A file holding a single JSON object was treated as a one-record stream and rooted at `$[*]`, so streaming reported `$[*].a` where the DOM path reports `$.a` — the same input answered two ways depending on a flag. My equivalence test only covered arrays, so it passed. `{"a":1}` and NDJSON share a leading byte, so they cannot be told apart from the prefix. `stream_ndjson` now holds the first value while attempting a second: one value means the file is that document, more means a stream. Two records live at the boundary, still bounded. RecordRoot carries the decision through to emit_record_events, and path_names_match_the_dom_path_for_every_top_level_shape covers all four shapes at the CLI level so this class cannot recur. The other three: - Text and Markdown showed cardinality and entropy with no indication which were sketch output, though the JSON has carried `exact` since #102. A BASIS column appears only when something is approximate — adding it always would put "exact" beside every row of ordinary output — with a note saying what approx means. - "bounded by the largest record" overclaimed. Memory also scales with distinct paths times per-path sketch state; the flat-RSS measurements share a schema. Both limits are now stated, along with what memory does *not* scale with, which is the number of records. - The global flag description implied every command streams. Only `stats` does, and only for JSON/NDJSON files.
1 participant
--streamingread the whole file, parsed a full DOM, then materialised aVec<JsonEvent>beside it — 402 MB against the DOM path's 233 MB on a 15 MB input. The sketch accumulators were already bounded and tested; what was missing was a way to feed them.--streamingTen times the input, the same memory. Bounded, not merely smaller.
How
vajra-core::recordspulls top-level records one at a time:StreamDeserializer.SeqAccessvisitor.StreamDeserializerhandles concatenated values, not the inside of an array, sonext_elementdraws one element at a time from a buffered reader. This is the shape vajra's own docs use as the large-input example, and the one that is not simply line-splitting.emit_record_eventsconverts each record into a reused buffer rooted at$[*], so nothing grows with the record count and paths match a whole-document walk.per_record_events_match_a_whole_document_walkasserts the value events are identical — if they weren't,--streamingwould silently answer a different question.Two limits, stated rather than implied
Memory is bounded by the largest single record, not the file — one enormous record is still materialised.
stdin, URLs, git repositories and the non-JSON formats go through readers that produce a whole document by construction. Passing
--streamingto those now reports that the input was loaded whole, rather than silently doing nothing.A wrong number this surfaced
With a real record stream the exact-tracking threshold is actually reached, and the fallback was reporting the top-k buffer size as cardinality:
$[*].id$[*].scoreEntropy followed it:
6.643856, exactlylog2(100), against a true 16.87 — in the same fields the DOM path uses for measurements.Those paths now carry
exact: false, omitted when true so DOM output is byte-unchanged.cardinalityis documented as a lower bound, which is provable: k occupied counters means at least k distinct values were seen.No bound is claimed for entropy. I wrote one first — folding the untracked tail into a single symbol, since grouping can only lower entropy — then checked the eviction rule and removed it: Space-Saving admits an evicted item at
min_count + 1, so it over-attributes rather than cleanly grouping, and the tail branch was dead code besides (counters always sum to at least the observation count). Calling it a bound would have been a claim I can't prove. A distinct-count sketch is the real fix, filed as #106.Below the threshold streaming is not an approximation at all — entropy agrees with the DOM path to 1e-12, asserted.
Schema
OUTPUT_SCHEMA_VERSIONgoes to 2 for the added field, with a history comment. First use of the versioning added in #104, which is what makes this detectable by a consumer rather than a silent shape change.Tests
vajra-core: 7 for the record stream (both shapes, order preservation, empty array, malformed input naming the source file, incremental visiting) and 2 for event equivalence.vajra-cli/tests/streaming_bounded.rs: exact agreement below the threshold, NDJSON parity, sketch results flagged and never exceeding truth, DOM output unflagged, and the fallback reported.The
"does not yet bound memory"warning from #103 is removed, and the test asserting it is replaced by its inverse rather than deleted — a stale caveat is its own kind of false claim.CLAUDE.md rule 1 now reads "partially met", with the measurement and the scope:
stats --streamingon JSON/NDJSON files. Other commands still materialise the corpus.Gate
cargo test --workspaceclean,cargo clippy --workspace --all-features -- -D warningsclean,cargo fmt --checkclean.Closes #102
Summary by CodeRabbit
New Features
--streamingsupport for JSON and NDJSON inputs with bounded memory usage.exact: false.Bug Fixes