feat(collector): materialize ClickHouse analytics contracts - #667
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds a ClickHouse analytics materializer for replay, PIT feature, and backtest-result artifacts. It validates manifests and artifacts, creates immutable plans, preserves lineage, performs idempotent partition writes, and adds unit and integration tests. ChangesClickHouse analytics materialization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Materializer
participant PlanFile
participant ClickHouse
CLI->>Materializer: select artifact and execution mode
Materializer->>Materializer: validate manifest, hashes, schema, and lineage
Materializer->>PlanFile: publish immutable JSONEachRow plan
Materializer->>ClickHouse: insert pending registry state
Materializer->>ClickHouse: insert typed analytics rows
Materializer->>ClickHouse: mark partition complete
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
rust_hft/tools/collector/CLICKHOUSE_ANALYTICS.md (1)
20-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the "three-writer inserts" wording.
A single run materializes one input kind.
send_to_clickhouseinserts the pending registry row, then the data rows for that one table, then the complete registry row. It never writes all three data tables in one run. The current text suggests three data inserts per run.📝 Proposed wording
-Remote writes claim the identity as `pending` before data rows and publish a -`complete` registry row only after all three-writer inserts succeed; a retry -seeing a pending claim fails closed instead of hiding a partial materialization. +Remote writes claim the identity as `pending` before data rows and publish a +`complete` registry row only after every data insert for that input kind +succeeds; a retry seeing a pending claim fails closed instead of hiding a +partial materialization.🤖 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/CLICKHOUSE_ANALYTICS.md` around lines 20 - 22, Update the documentation text describing the remote write sequence to state that each run inserts data rows for one input table between the pending and complete registry-row inserts. Remove the wording “all three-writer inserts” so it does not imply that one run writes all three data tables, while preserving the retry and partial-materialization behavior.rust_hft/tools/collector/src/bin/clickhouse-analytics-materializer.rs (2)
454-454: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
with_capacity(manifest.rows)trusts a self-declared count.
manifest.rowscomes from the manifest body. The manifest hash proves integrity, not sanity. A very large value causes one large allocation before any row is read. Cap the reservation against the Parquet row count fromreader.metadata().file_metadata().num_rows(), or clamp it to a bound.🤖 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/clickhouse-analytics-materializer.rs` at line 454, Update the rows allocation near the materializer’s Parquet reader flow so it does not trust manifest.rows for an unbounded reservation. Use reader.metadata().file_metadata().num_rows() to cap the requested capacity (or apply an equivalent safe upper bound), while preserving row collection behavior and avoiding large allocations from malicious manifest values.
698-705: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueSet explicit ClickHouse request timeouts.
reqwest::Client::new()uses reqwest’s default request timeouts, which may still allow stalled ClickHouse connections to consume the materializer process longer than wanted. Build the client with timeouts and reportreqwest::Erroras ClickHouse failures if no connection can complete.🤖 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/clickhouse-analytics-materializer.rs` around lines 698 - 705, Update the client construction in the materializer request flow around reqwest::Client::new to use an explicit request timeout configuration, including the required connect and overall request limits. Build the client through the fallible builder path and propagate its reqwest::Error as the existing ClickHouse failure type before sending the request, preserving the current authentication and query behavior.rust_hft/tools/collector/tests/clickhouse_analytics_materializer.rs (1)
179-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
partition_identityequality across rows, not just non-emptiness.The loop checks
manifest_sha256,source_revision,venue,market,symbol, andschema_versionfor equality againstrows[0], but only checks thatpartition_identityis non-empty on each row, not that all rows share the same identity. Sincepartition_identityis a key lineage field tying rows of one partition together, assert its equality across rows too.♻️ Proposed strengthened assertion
for row in &rows { assert_eq!(row["manifest_sha256"], rows[0]["manifest_sha256"]); assert_eq!(row["source_revision"], rows[0]["source_revision"]); assert_eq!(row["venue"], "binance"); assert_eq!(row["market"], "usdm"); assert_eq!(row["symbol"], "BTCUSDT"); assert_eq!(row["schema_version"], "binance-replay-parquet-v1"); assert_eq!(row["start_time_us"], 1_000); assert_eq!(row["end_time_us"], 2_000); - assert!(!row["partition_identity"].as_str().unwrap().is_empty()); + assert_eq!(row["partition_identity"], rows[0]["partition_identity"]); + assert!(!row["partition_identity"].as_str().unwrap().is_empty()); }🤖 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/clickhouse_analytics_materializer.rs` around lines 179 - 189, Update the row-validation loop in the ClickHouse materializer test to assert each row’s partition_identity equals rows[0]["partition_identity"], while retaining the existing non-empty validation if needed.
🤖 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/CLICKHOUSE_ANALYTICS.md`:
- Around line 61-64: Update the result manifest documentation near the existing
backtest-result metadata requirements to state that result_plan requires the
filename to be exactly <manifest-sha256>.result-manifest.json. Clarify that
arbitrary result-manifest paths are rejected, while preserving the existing
replay-manifest filename guidance.
In `@rust_hft/tools/collector/src/bin/clickhouse-analytics-materializer.rs`:
- Around line 587-588: The partition identity built near partition_identity is
ambiguous when venue or symbol contains a colon. Validate the caller-supplied
venue and symbol inputs to reject ':' before constructing the identity, while
preserving the existing non-empty validation and colon-delimited format.
- Around line 713-748: The pending claim around the registry read and insert is
not concurrency-safe, allowing conflicting materializations for one partition
identity. Serialize claims using a lightweight lock mechanism (or equivalent
external per-identity serialization), then read back and verify the winning
manifest_sha256 before inserting data; preserve the existing idempotent and
conflict outcomes. Document and implement the recovery procedure for stale
materialization_state="pending" rows after failed data insertion, including how
they may be safely cleared or retried.
- Around line 247-259: Update the artifact path validation before joining in the
materializer to reject absolute paths as well as relative paths containing
ParentDir components. Ensure manifest.artifact_path cannot bypass the
manifest-directory containment check, while preserving the existing
relative-path rejection and subsequent join behavior.
- Around line 64-65: Update the CLI configuration for the password field in the
argument struct so it is no longer accepted as a plain command-line value; read
it through a secure environment-variable mechanism instead, and enable Clap’s
env feature in the collector dependency configuration. Preserve the existing
password field and downstream usage while preventing exposure through process
listings and shell history.
- Around line 800-809: Update sql_literal to escape backslashes as \\ before
escaping single quotes, ensuring partition_identity values cannot alter the
ClickHouse string literal parsed by identity_query; preserve the existing
surrounding-quote behavior.
---
Nitpick comments:
In `@rust_hft/tools/collector/CLICKHOUSE_ANALYTICS.md`:
- Around line 20-22: Update the documentation text describing the remote write
sequence to state that each run inserts data rows for one input table between
the pending and complete registry-row inserts. Remove the wording “all
three-writer inserts” so it does not imply that one run writes all three data
tables, while preserving the retry and partial-materialization behavior.
In `@rust_hft/tools/collector/src/bin/clickhouse-analytics-materializer.rs`:
- Line 454: Update the rows allocation near the materializer’s Parquet reader
flow so it does not trust manifest.rows for an unbounded reservation. Use
reader.metadata().file_metadata().num_rows() to cap the requested capacity (or
apply an equivalent safe upper bound), while preserving row collection behavior
and avoiding large allocations from malicious manifest values.
- Around line 698-705: Update the client construction in the materializer
request flow around reqwest::Client::new to use an explicit request timeout
configuration, including the required connect and overall request limits. Build
the client through the fallible builder path and propagate its reqwest::Error as
the existing ClickHouse failure type before sending the request, preserving the
current authentication and query behavior.
In `@rust_hft/tools/collector/tests/clickhouse_analytics_materializer.rs`:
- Around line 179-189: Update the row-validation loop in the ClickHouse
materializer test to assert each row’s partition_identity equals
rows[0]["partition_identity"], while retaining the existing non-empty validation
if needed.
🪄 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: 1618514d-be10-4563-bbbb-3fdb83c43fe2
📒 Files selected for processing (4)
rust_hft/tools/collector/CLICKHOUSE_ANALYTICS.mdrust_hft/tools/collector/Cargo.tomlrust_hft/tools/collector/src/bin/clickhouse-analytics-materializer.rsrust_hft/tools/collector/tests/clickhouse_analytics_materializer.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16b6fff85d
ℹ️ 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".
proerror77
force-pushed
the
codex/canonical-parquet-clickhouse-653
branch
2 times, most recently
from
August 9, 2026 03:22
b8e0503 to
37c2076
Compare
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 `@rust_hft/tools/collector/CLICKHOUSE_ANALYTICS.md`:
- Around line 22-29: Update the retry behavior description in
cex_analytics_partitions to qualify that retries are no-ops only when the
partition is complete. Explicitly retain that a retry with the same pending
manifest resumes materialization with deterministic row identities, while a
different manifest remains a conflict rejected before insertion.
In `@rust_hft/tools/collector/tests/clickhouse_analytics_materializer.rs`:
- Around line 282-294: Update
remote_write_requires_an_external_partition_claim_directory to expect
output.exists() after the command fails, preserving the assertion that stderr
contains “--claim-dir”; do not alter production ordering unless the intended
contract is to validate --claim-dir before publishing the local plan.
🪄 Autofix
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: 080316bf-0cfd-4e86-b466-b8140a85c9a1
📒 Files selected for processing (4)
rust_hft/tools/collector/CLICKHOUSE_ANALYTICS.mdrust_hft/tools/collector/Cargo.tomlrust_hft/tools/collector/src/bin/clickhouse-analytics-materializer.rsrust_hft/tools/collector/tests/clickhouse_analytics_materializer.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rust_hft/tools/collector/src/bin/clickhouse-analytics-materializer.rs
proerror77
force-pushed
the
codex/canonical-parquet-clickhouse-653
branch
5 times, most recently
from
August 9, 2026 04:53
ec79fc8 to
1fef1c3
Compare
proerror77
force-pushed
the
codex/canonical-parquet-clickhouse-653
branch
from
August 9, 2026 05:06
1fef1c3 to
1ed5191
Compare
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
1 participant
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.
Change contract
Materialize manifest-verified canonical Parquet partitions into immutable, lineage-rich ClickHouse analytics rows. The offline plan emits separate replay-event, PIT-feature, and backtest-result writers; optional remote writes require an external per-partition claim and publish complete rows only after typed inserts succeed.
Issue relationship
Closes #653
Out of scope
ClickHouse provisioning, deployment, backtest replay from ClickHouse, raw/canonical evidence replacement, live-runtime authorization, production cutover, ACK, OSS, and credential management are out of scope. No ClickHouse instance is required for validation.
Dependencies and merge order
#651 and #666 are already on
main. Final code is rebased onmain@a7927bbb22d0ee20613e8705272e097533a57c9b.Focused validation
cargo test --manifest-path rust_hft/Cargo.toml -p hft-collector --lockedpassed, including 10 materializer unit tests and 7 CLI integration tests; focused 10+7 materializer tests passed again after the final no-overlap rebase.cargo clippy --manifest-path rust_hft/Cargo.toml -p hft-collector --all-targets --features collector-binance --no-deps --locked -- -D warningspassed.rustfmt --edition 2021 --checkon the changed Rust files andgit diff --checkpassed.Rollout and rollback
No production rollout. This is an offline/optional analytics-plane binary and does not provision or contact ClickHouse unless an endpoint and shared claim directory are explicitly supplied. Rollback is reverting this one PR.
Scope exception
Final head
1ed5191939cd76b17076bf3be9556871937af4f2changes four scoped files and adds 2,018 non-generated lines, exceeding the 750-line threshold. Verified input admission, immutable/idempotent publication, and the three typed writer schemas form one fail-closed materialization contract; splitting them would expose partial lineage or conflict semantics. On 2026-08-09 the solo maintainer explicitly authorized this atomic exception and removed the independent-human-approval precondition. Existing independent CodeRabbit/Codex code-review and security workflow receipts have no unresolved P1/P2; all review threads are resolved. No fabricated GitHub approval is claimed.