feat(collector): materialize canonical replay Parquet - #661
Conversation
|
Warning Review limit reached
Next review available in: 28 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (3)
📝 WalkthroughWalkthroughAdds a Binance replay Parquet materializer. It verifies raw replay triplets, converts selected snapshots and updates into canonical events, writes immutable ZSTD-compressed Parquet artifacts, emits hashed manifests, and adds end-to-end success and corruption tests. ChangesBinance replay materialization
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Materializer
participant RawTriplets
participant Parquet
participant Manifest
CLI->>Materializer: provide mission, symbol, source paths, and hashes
Materializer->>RawTriplets: verify triplets and continuity
RawTriplets-->>Materializer: verified replay source
Materializer->>Parquet: write ordered canonical events
Parquet-->>Materializer: return artifact hash
Materializer->>Manifest: publish artifact and source metadata
Manifest-->>CLI: return canonical manifest
🚥 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c192ed7124
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
rust_hft/tools/collector/src/bin/binance-replay-parquet-materializer.rs (1)
312-315: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that
sequenceis a row ordinal.
sequenceis a 1-based position in the emitted tape. It is not a Binance update id. The upstreamReplaySequenceEventexposes no exchange sequence field, so the ordinal is the only value available here.The manifest publishes this value as
sequence_startandsequence_end. A consumer can read those names as exchange sequence coverage. Add a comment here, and consider renaming the manifest fields torow_ordinal_startandrow_ordinal_end, so the coverage claim stays unambiguous.🤖 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 `@rust_hft/tools/collector/src/bin/binance-replay-parquet-materializer.rs` around lines 312 - 315, Document near the sequence calculation in the materializer that sequence is a 1-based emitted-tape row ordinal, not a Binance update ID, because ReplaySequenceEvent provides no exchange sequence. Rename the manifest fields sequence_start and sequence_end to row_ordinal_start and row_ordinal_end, updating their producers and consumers consistently.rust_hft/tools/collector/tests/binance_replay_parquet_materializer.rs (2)
99-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the dependency on the external
zstdbinary.The fixture shells out to
zstd. The test fails on any machine or CI image without that binary. The failure message is a bare assertion, so the cause is not obvious. The crate already links thezstdC library throughparquet'szstdfeature, so azstddev-dependency adds no new system requirement. Compress in-process instead.If you keep the CLI call, add a message that names the missing binary.
♻️ Compress in-process
Add the dev-dependency in
rust_hft/tools/collector/Cargo.toml:[dev-dependencies] zstd = "0.13"Then replace the CLI invocation:
- let raw = directory.join("part-1.jsonl"); let data = directory.join("part-1.jsonl.zst"); - let mut raw_file = File::create(&raw).unwrap(); + let mut encoder = zstd::Encoder::new(File::create(&data).unwrap(), 3).unwrap(); for row in rows { - serde_json::to_writer(&mut raw_file, row).unwrap(); - raw_file.write_all(b"\n").unwrap(); + serde_json::to_writer(&mut encoder, row).unwrap(); + encoder.write_all(b"\n").unwrap(); } - assert!(Command::new("zstd") - .args(["-q", "-f"]) - .arg(&raw) - .arg("-o") - .arg(&data) - .status() - .unwrap() - .success()); - fs::remove_file(raw).unwrap(); + encoder.finish().unwrap().sync_all().unwrap();🤖 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 `@rust_hft/tools/collector/tests/binance_replay_parquet_materializer.rs` around lines 99 - 107, Remove the external zstd Command invocation in the test fixture and add the zstd crate as a dev-dependency in the collector Cargo.toml. Update the surrounding materialization flow in binance_replay_parquet_materializer to compress the raw fixture in-process through the zstd API, preserving the existing output path and cleanup behavior.
226-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider covering more than one row group.
The fixture produces 3 canonical rows, so
write_parquetemits a single row group. The bounded row-group path inwrite_parquet, which chunks byROW_GROUP_ROWS, stays untested. Add a fixture with more rows thanROW_GROUP_ROWS, or expose the bound so a test can lower it, then assertnum_row_groups()and cross-row-group order.🤖 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 `@rust_hft/tools/collector/tests/binance_replay_parquet_materializer.rs` around lines 226 - 232, Extend the parquet materializer test around write_parquet to exercise multiple row groups by supplying more than ROW_GROUP_ROWS canonical rows, or by using an exposed smaller bound. Assert the resulting file_metadata().num_row_groups() exceeds one and verify collected rows retain their expected order across row-group boundaries.
🤖 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 `@rust_hft/tools/collector/src/bin/binance-replay-parquet-materializer.rs`:
- Around line 139-143: Update normalize_levels and ReplayPayload to retain
validated source decimal strings instead of converting values to f64, while
preserving numeric validation. Serialize bids and asks as string pairs, and
update the materializer test assertions around the JSON payload to expect those
original strings.
- Around line 451-468: Update publish_temp_immutable so the hard_link error
branch for an absent destination removes temporary before returning the original
error. Preserve the existing cleanup behavior for successful linking and when
the destination already exists, while ensuring cleanup does not replace or mask
the hard_link failure.
---
Nitpick comments:
In `@rust_hft/tools/collector/src/bin/binance-replay-parquet-materializer.rs`:
- Around line 312-315: Document near the sequence calculation in the
materializer that sequence is a 1-based emitted-tape row ordinal, not a Binance
update ID, because ReplaySequenceEvent provides no exchange sequence. Rename the
manifest fields sequence_start and sequence_end to row_ordinal_start and
row_ordinal_end, updating their producers and consumers consistently.
In `@rust_hft/tools/collector/tests/binance_replay_parquet_materializer.rs`:
- Around line 99-107: Remove the external zstd Command invocation in the test
fixture and add the zstd crate as a dev-dependency in the collector Cargo.toml.
Update the surrounding materialization flow in
binance_replay_parquet_materializer to compress the raw fixture in-process
through the zstd API, preserving the existing output path and cleanup behavior.
- Around line 226-232: Extend the parquet materializer test around write_parquet
to exercise multiple row groups by supplying more than ROW_GROUP_ROWS canonical
rows, or by using an exposed smaller bound. Assert the resulting
file_metadata().num_row_groups() exceeds one and verify collected rows retain
their expected order across row-group boundaries.
🪄 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: 30e80f86-d7f6-476d-a53b-26c0feaf5574
⛔ Files ignored due to path filters (1)
rust_hft/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
rust_hft/tools/collector/Cargo.tomlrust_hft/tools/collector/src/bin/binance-replay-parquet-materializer.rsrust_hft/tools/collector/tests/binance_replay_parquet_materializer.rs
1 participant
Change contract
Add a fail-closed CLI that verifies selected Binance LOB raw triplets before publishing a SHA-addressed canonical replay Parquet partition and canonical manifest.
Issue relationship
Closes #651
Out of scope
Local PVC/ESSD cache warmer and hft-backtest reader (#652); ClickHouse materialization (#653); ACK/PVC/ClickHouse provisioning and every collector/runtime cutover (#654).
Dependencies and merge order
Based on main. #652 and #653 remain blocked until this contract is merged.
Focused validation
Rollout and rollback
None. This adds an offline materialization CLI only; it changes no collector deployment, cache, ClickHouse, or runtime. Roll back by reverting c192ed7.
Scope exception
821 added lines include 27 generated Cargo.lock lines; the remaining 794 lines are one binary and its required public CLI fixture for one fail-closed behavior and rollback unit. Splitting verification, immutable publication, and the fixture would leave a non-reviewable contract. Named reviewer /root/review_standards approved this atomic exception on 2026-08-03.
Summary by CodeRabbit
New Features
Bug Fixes
Tests