feat(collector): materialize ClickHouse analytics contracts - #667
feat(collector): materialize ClickHouse analytics contracts#667proerror77 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds a ClickHouse analytics materializer for replay, PIT feature, and backtest-result artifacts. It validates manifests and Parquet inputs, creates immutable plans, preserves lineage, handles 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 |
![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 (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
| For multimodal PIT manifests, pass `--venue` and `--market` explicitly. A | ||
| backtest-result manifest uses `backtest-result-metadata-v1` and must include a | ||
| SHA-256 `source_revision`; the result JSON is stored as metadata, never as a | ||
| replay event tape. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the content-addressed filename rule for result manifests.
result_plan requires the manifest filename to equal <manifest-sha256>.result-manifest.json and fails closed otherwise. The document states the opposite exemption for replay manifests but is silent for result manifests. Add the requirement so operators do not pass an arbitrary path.
🤖 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 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.
| #[arg(long, default_value = "")] | ||
| password: String, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
clap 4 arg attribute env hide_env_values
💡 Result:
In clap v4, the hide_env_values functionality is available via the Arg::hide_env_values builder method [1]. When using the derive API, this is accessed through the #[arg(hide_env_values = true)] attribute [2][3]. Key details regarding hide_env_values: 1. Purpose: This setting prevents the current value of an associated environment variable from being displayed in the generated --help message [1][4]. This is a security feature intended to prevent sensitive information (like API keys or secrets) from being inadvertently exposed [1][4]. 2. Implementation (Builder API): You can apply it to an Arg instance using:.env("MY_ENV_VAR").hide_env_values(true) [1] 3. Implementation (Derive API): When using #[derive(Parser)], you apply it as an attribute on your struct fields [3]: #[arg(long, env = "MY_ENV_VAR", hide_env_values = true)] field: String, Note that you must explicitly set it to true (i.e., hide_env_values = true), as clap derive attributes for boolean settings generally require an explicit value rather than being implicitly enabled by presence alone [2]. 4. Requirements: The env feature must be enabled in your Cargo.toml for environment variable support to function [5][6]. This approach replaces the older ArgSettings::HideEnvValues used in earlier versions of clap, which has been deprecated [7][8].
Citations:
- 1: https://docs.rs/clap/4.1.8/clap/struct.Arg.html
- 2: derive error for missing
= valuefor attributes is confusing clap-rs/clap#3726 - 3: https://blog.hashhackers.com/blog/rust-cli-clap/
- 4: https://docs.rs/clap-v3/latest/clap_v3/enum.ArgSettings.html
- 5: https://docs.rs/clap/latest/clap/_features/index.html
- 6: https://rust.code-maven.com/clap/clap-and-environment-variables
- 7: https://docs.rs/clap/~3.1/enum.ArgSettings.html
- 8: HideEnvValues is deprecated, but there doesn't seem to be a derive-based alternative clap-rs/clap#3730
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate file and relevant lines =="
fd -a 'clickhouse-analytics-materializer.rs' . | sed 's#^\./##'
file="$(fd 'clickhouse-analytics-materializer.rs' . | head -n1)"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '1,120p' "$file" | cat -n
fi
echo
echo "== Find clap version/features in Cargo manifests =="
rg -n 'name = "tools-collector"|name = "collector"|clap|collect' Cargo.toml Cargo.lock rust_hft/tools/collector -g 'Cargo.toml' -g 'Cargo.lock' | head -200Repository: proerror77/monday
Length of output: 5900
Do not accept the ClickHouse password as a plain CLI argument.
Command-line arguments are readable by other local users through the process list and can be written to shell history or logs. Read the password from an environment variable or a file instead.
🔒 Proposed fix
- #[arg(long, default_value = "")]
- password: String,
+ /// Read the ClickHouse password from this environment variable.
+ #[arg(long, env = "CLICKHOUSE_PASSWORD", default_value = "", hide_env_values = true)]
+ password: String,Add the env feature to the collector’s clap dependency until the crate has managed credentials handling.
🤖 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 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.
| if manifest.artifact_path.is_relative() | ||
| && manifest | ||
| .artifact_path | ||
| .components() | ||
| .any(|component| matches!(component, std::path::Component::ParentDir)) | ||
| { | ||
| bail!("PIT feature artifact path escapes its manifest directory"); | ||
| } | ||
| let artifact = manifest_path | ||
| .parent() | ||
| .context("PIT feature manifest has no parent")? | ||
| .join(&manifest.artifact_path); | ||
| manifest.artifact_path = artifact; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
An absolute artifact_path bypasses the PIT containment check.
The guard runs only when manifest.artifact_path.is_relative(). If the manifest declares an absolute path, no check runs, and Path::join discards the manifest parent and uses the absolute path directly. The materializer then reads a file outside the manifest directory. validate_replay_manifest (Line 395) and result_plan (Line 322) both reject absolute paths, so this kind is the outlier.
🔒 Proposed fix
- if manifest.artifact_path.is_relative()
- && manifest
- .artifact_path
- .components()
- .any(|component| matches!(component, std::path::Component::ParentDir))
+ if manifest.artifact_path.is_absolute()
+ || manifest
+ .artifact_path
+ .components()
+ .any(|component| matches!(component, std::path::Component::ParentDir))
{
bail!("PIT feature artifact path escapes its manifest directory");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if manifest.artifact_path.is_relative() | |
| && manifest | |
| .artifact_path | |
| .components() | |
| .any(|component| matches!(component, std::path::Component::ParentDir)) | |
| { | |
| bail!("PIT feature artifact path escapes its manifest directory"); | |
| } | |
| let artifact = manifest_path | |
| .parent() | |
| .context("PIT feature manifest has no parent")? | |
| .join(&manifest.artifact_path); | |
| manifest.artifact_path = artifact; | |
| if manifest.artifact_path.is_absolute() | |
| || manifest | |
| .artifact_path | |
| .components() | |
| .any(|component| matches!(component, std::path::Component::ParentDir)) | |
| { | |
| bail!("PIT feature artifact path escapes its manifest directory"); | |
| } | |
| let artifact = manifest_path | |
| .parent() | |
| .context("PIT feature manifest has no parent")? | |
| .join(&manifest.artifact_path); | |
| manifest.artifact_path = artifact; |
🤖 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 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.
| let partition_identity = | ||
| format!("{schema_version}:{venue}:{market}:{symbol}:{start_time_us}:{end_time_us}"); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
partition_identity can be ambiguous when a component contains :.
The identity is a :-joined string and it is the immutability key for the registry and for conflict detection. venue and symbol are only checked for non-emptiness, and --venue is caller supplied. Two different partitions can then produce the same identity string. Reject : in venue and symbol, or use a hash of the tuple.
🤖 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 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.
| for row in existing | ||
| .lines() | ||
| .filter_map(|line| serde_json::from_str::<Value>(line).ok()) | ||
| { | ||
| let Some(existing_hash) = row | ||
| .get("manifest_sha256") | ||
| .and_then(Value::as_str) | ||
| .map(str::to_string) | ||
| else { | ||
| continue; | ||
| }; | ||
| if existing_hash != plan.lineage.manifest_sha256 { | ||
| bail!("ClickHouse partition identity conflicts with existing manifest"); | ||
| } | ||
| match row.get("materialization_state").and_then(Value::as_str) { | ||
| Some("complete") => return Ok("idempotent-hit".to_string()), | ||
| Some("pending") => { | ||
| bail!("ClickHouse partition identity has an incomplete materialization") | ||
| } | ||
| _ => bail!("ClickHouse partition registry state is unsupported"), | ||
| } | ||
| } | ||
|
|
||
| let mut pending = plan.rows[0].1.clone(); | ||
| pending["materialization_state"] = "pending".into(); | ||
| pending["materialization_version"] = 1.into(); | ||
| insert_rows( | ||
| &client, | ||
| &query_url, | ||
| database, | ||
| user, | ||
| password, | ||
| "cex_analytics_partitions", | ||
| serde_json::to_vec(&pending)?, | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The pending claim is not atomic, so two concurrent runs can both claim one identity.
The code reads the registry, then inserts the pending row. Between those two steps another process can run the same read. cex_analytics_partitions is a ReplacingMergeTree, which deduplicates by key at merge time and enforces no uniqueness at insert time. Two runs with different manifest_sha256 values for one partition_identity can therefore both pass the conflict check and both write data rows. That defeats the "prevent conflicting content" requirement.
Options: gate the claim with a lightweight lock table plus a read-back verification of the winning manifest_sha256 before data inserts, or serialize materialization per identity outside this binary.
Also document the recovery path for a stuck pending row. After a failed data insert the claim stays, and every later retry fails closed with no documented way to clear it.
🤖 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 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.
| fn sql_literal(value: &str) -> String { | ||
| format!("'{}'", value.replace('\'', "''")) | ||
| } | ||
|
|
||
| fn identity_query(database: &str, partition_identity: &str) -> String { | ||
| format!( | ||
| "SELECT manifest_sha256, materialization_state, materialization_version FROM {database}.cex_analytics_partitions FINAL WHERE partition_identity = {} LIMIT 1 FORMAT JSONEachRow", | ||
| sql_literal(partition_identity) | ||
| ) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
ClickHouse string literal backslash escaping HTTP param_ query parameters
💡 Result:
In ClickHouse, HTTP query parameters (prefixed with param_) are parsed using an escaped text format to allow for the unambiguous representation of special characters, including NULLs (encoded as \N) [1][2][3]. Key rules for handling backslashes and special characters in these parameters include: 1. Escape Sequences: ClickHouse interprets backslashes as the start of escape sequences (e.g., \t for tab, \n for newline, \N for NULL) [1][2][4]. 2. Encoding Requirements: To include a literal backslash or a special character in a URL query parameter, you must ensure it is correctly encoded for the URL and then escaped for the ClickHouse parser [1][2]. - For example, to pass a literal tab character (\t), it must be provided as the string \t in the parameter value. - Because this string contains a backslash, it must be URL-encoded, resulting in %5C%09 (where %5C is the URL encoding for \ and %09 is for the tab) [1][2]. 3. Consistency Challenges: Users have reported that inconsistent interpretation of backslashes can occur in some versions [5]. Specifically, a single backslash followed by certain characters may trigger parsing errors (CANNOT_PARSE_ESCAPE_SEQUENCE), while double backslashes (\) are sometimes interpreted as a literal backslash [5]. To ensure reliable data transmission, applications should implement robust serialization that escapes special characters (backslash, tab, newline, carriage return) with a preceding backslash before passing them to the ClickHouse HTTP interface [4].
Citations:
- 1: https://github.com/ClickHouse/clickhouse-docs/blob/main/docs/integrations/interfaces/http.md
- 2: https://clickhouse.com/docs/concepts/features/interfaces/http
- 3: Changed serialization of query parameters ClickHouse/ClickHouse#8517
- 4: https://github.com/buildkite/clickhouse-sql/blob/main/lib/clickhouse/http_client/parameter_serializer.rb
- 5: Backslashes in a string query parameter seem to be interpreted inconsistently ClickHouse/ClickHouse#69656
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant Rust file and nearby definitions/usages.
file="rust_hft/tools/collector/src/bin/clickhouse-analytics-materializer.rs"
if [ ! -f "$file" ]; then
echo "missing $file"
fd -a 'clickhouse-analytics-materializer\.rs$' .
exit 1
fi
echo "== file stats =="
wc -l "$file"
echo "== relevant sql_literal and identity_query =="
sed -n '760,825p' "$file" | cat -n
echo "== partition_identity construction and validation sites =="
rg -n "partition_identity|sql_identifier|sql_literal|identity_query|--venue|parameter|param_|to_ascii_uppercase|market|symbol|venue" "$file" -C 3
echo "== diff stat/name status =="
git diff --stat HEAD~1..HEAD 2>/dev/null || git diff --stat 2>/dev/null || trueRepository: proerror77/monday
Length of output: 13056
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="rust_hft/tools/collector/src/bin/clickhouse-analytics-materializer.rs"
echo "== clickhouse docs mentions in repo =="
rg -n "ClickHouse|clickhouse|cex_analytics_partitions|partition_identity|sql_literal|bind|param_|JSONEachRow" rust_hft/tools/collector -C 2
echo "== tests around sql_literal and identity =="
sed -n '860,875p' "$file" | cat -n
echo "== deterministic Rust string transformation probe =="
python3 - <<'PY'
samples = ["binance:spot:BTCUSDT:1:2", "a\\b", "a\\'", "\\q", "a\\\\b", "a\\\\'"]
for s in samples:
quoted = "'" + s.replace("'", "''") + "'"
print(f"input={repr(s)} quoted={repr(quoted)}")
print("partition_identity with trailing backslash contains no quote escape:")
s = "pit:v1\\:binance\\:spot\\:BTCUSDT\\:1\\:2"
quoted = "'" + s.replace("'", "''") + "'"
print(repr(quoted), "' inside quoted:", "'" in quoted)
PYRepository: proerror77/monday
Length of output: 50375
🌐 Web query:
ClickHouse documentation String quoted literal backslash escape
💡 Result:
In ClickHouse, string literals must be enclosed in single quotes (') [1][2]. Double quotes are not supported for string literals [1][2]. To include special characters or quotes within a string, ClickHouse supports two primary escaping methods: 1. Backslash Escaping: You can use a backslash () followed by a supported escape sequence [1][2]. 2. Single Quote Doubling: The single quote character (') can be escaped by using two consecutive single quotes ('') [1][2]. Supported Backslash Escape Sequences When a backslash is used, it interprets the following character according to the table below [1][2]. If a backslash precedes a character not listed in this table, it loses its special meaning and is interpreted literally (e.g., \w is treated as \w, not a special escape sequence) [1][2]. | Supported Escape | Description | | ----------------- | ------------------------------------------------------------- | | \xHH | 8-bit character (hex digits) | | \N | Reserved (does nothing) | | \a | Alert | | \b | Backspace | | \e | Escape character | | \f | Form feed | | \n | Line feed | | \r | Carriage return | | \t | Horizontal tab | | \v | Vertical tab | | \0 | Null character | | \ | Backslash | | ' | Single quote | | " | Double quote | | ` | Backtick | | / | Forward slash | | = | Equal sign | Important Notes - Mandatory Escaping: You must escape at least the single quote (') and the backslash () characters using either their respective escape sequences (' or \ and \ respectively) or by doubling the quote ('') [1][2]. - LIKE Operator Context: When using the LIKE operator, there is an additional layer of escaping required because LIKE uses \ as an escape character for its own metacharacters (%, _, and ). Consequently, to match a literal backslash in a LIKE pattern, you often need to write four backslashes (\\) [3]. - Heredoc: For scenarios where you wish to avoid extensive escaping, ClickHouse also supports heredoc syntax ($tag$content$tag$), where no escaping is required [4].
Citations:
- 1: https://clickhouse.com/docs/reference/syntax
- 2: https://github.com/ClickHouse/ClickHouse/blob/master/docs/en/sql-reference/syntax.md
- 3: https://clickhouse.com/docs/reference/functions/regular-functions/string-search-functions
- 4: https://help.aliyun.com/en/clickhouse/developer-reference/syntax
Escape backslashes in ClickHouse string literals
sql_literal doubles ', but ClickHouse also treats \ as an escape character inside single-quoted literals. A partition_identity containing a backslash before the closing quote can change the parsed string. Escape \ as \\ and \' before '\'', or use ClickHouse HTTP bound parameters.
🤖 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 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.
![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: 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".
| let mut pending = plan.rows[0].1.clone(); | ||
| pending["materialization_state"] = "pending".into(); | ||
| pending["materialization_version"] = 1.into(); | ||
| insert_rows( |
There was a problem hiding this comment.
When two materializers for different manifests sharing one partition identity run concurrently, both can complete the preceding SELECT before either reaches this unconditional pending-row insert. The documented ReplacingMergeTree only resolves versions during reads; it does not make this check-and-insert sequence exclusive, so both writers can insert typed rows and publish version-2 registry rows, leaving mixed analytics data and a nondeterministically selected manifest. Use an atomic/external claim mechanism or serialize writers and verify ownership before inserting data.
AGENTS.md reference: AGENTS.md:L38-L39
Useful? React with 👍 / 👎.
| output.write_all(bytes)?; | ||
| output.sync_all()?; | ||
| drop(output); | ||
| match fs::rename(&temporary, path) { |
There was a problem hiding this comment.
On Unix, fs::rename replaces an existing destination, so if two processes pass the earlier path.exists() check with different plan bytes, the later rename silently overwrites the first plan and returns Ok(()); the conflict branches never run. This violates the immutable-output contract under concurrent publication. Use a no-replace primitive such as the crate's existing hard-link publication pattern, then compare the winning file on an already-exists error.
AGENTS.md reference: AGENTS.md:L40-L41
Useful? React with 👍 / 👎.
| let hash = sha256_bytes(&bytes); | ||
| valid_sha256(&hash, "manifest sha256")?; | ||
| Ok((path, bytes, hash)) |
There was a problem hiding this comment.
For replay and PIT inputs, the CLI receives only a manifest path, and this function computes a hash from those same untrusted bytes without comparing it to an expected digest, signed envelope, content-addressed filename, or verified cache marker; checking that the newly computed SHA string is syntactically valid is tautological. A fabricated but structurally valid manifest and matching artifact can therefore claim arbitrary source-segment revisions and be published as verified analytics lineage. Require an independently supplied/verified manifest identity while still allowing relocated cache filenames.
AGENTS.md reference: AGENTS.md:L38-L39
Useful? React with 👍 / 👎.
| { | ||
| bail!("canonical Parquet schema does not match its manifest"); | ||
| } | ||
| let mut rows = Vec::with_capacity(manifest.rows); |
There was a problem hiding this comment.
For realistic multi-million-row LOB partitions, this allocation retains the entire decompressed event tape, after which replay_plan duplicates it into a Vec<Value>, encode_plan creates another full byte buffer, and remote sending constructs another grouped buffer. Peak memory therefore scales to several times the uncompressed partition size and can OOM before any output or ClickHouse insert occurs. Validate and encode/send rows through bounded streaming or staged files rather than materializing every representation simultaneously.
Useful? React with 👍 / 👎.
| manifest.artifact_sha256 | ||
| ); | ||
| } | ||
| let rows = read_replay_rows(&artifact, &manifest)?; |
There was a problem hiding this comment.
If a replay artifact is replaced after sha256_file closes it but before this call reopens the pathname, the rows can come from different bytes than the SHA recorded in lineage; result_plan repeats the same hash-then-fs::read pattern. This permits cache corruption or a concurrent replacement to pass verification and publish falsely attributed analytics. Open once and hash plus parse that same handle or retained byte snapshot.
AGENTS.md reference: AGENTS.md:L38-L39
Useful? React with 👍 / 👎.
| let partition_identity = | ||
| format!("{schema_version}:{venue}:{market}:{symbol}:{start_time_us}:{end_time_us}"); |
There was a problem hiding this comment.
When two valid backtests over the same venue, symbol, and time window produce different strategy results, both receive the same partition identity because the key omits the manifest, artifact, source revision, and run identity, so the second result is rejected as a conflict. The same happens for PIT matrices covering the same window but using different feature sets or label horizons. This prevents the analytics plane from retaining multiple experiments over one market interval; include a dataset-specific feature/run identity in these partition keys.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| fn sql_literal(value: &str) -> String { | ||
| format!("'{}'", value.replace('\'', "''")) |
There was a problem hiding this comment.
For PIT and result inputs, venue and symbol values are only checked for non-emptiness, and the PIT --venue override is similarly unrestricted. An identity containing a backslash before a quote is not safely escaped by merely doubling quotes because ClickHouse also interprets backslash escapes, allowing the literal to terminate early and the identity SELECT predicate to be altered. Use ClickHouse HTTP query parameters or comprehensively validate/encode identity components instead of constructing this SQL literal.
AGENTS.md reference: AGENTS.md:L38-L39
Useful? React with 👍 / 👎.
1 participant
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 claim a pending registry identity and publish complete rows only after all typed inserts succeed.
Issue relationship
Closes #653
Out of scope
ClickHouse provisioning, deployment, backtest replay from ClickHouse, raw/canonical evidence replacement, live-runtime authorization, and production cutover are out of scope. No ClickHouse instance is required for validation.
Dependencies and merge order
#651 and #666 are already on
main(main@9b8b36b6f6cfe6d805e7cd6e172b677a3ddd4ac3).Focused validation
cargo test --manifest-path rust_hft/Cargo.toml -p hft-collector --bin clickhouse-analytics-materializer --test clickhouse_analytics_materializer --locked(4 unit + 2 integration passed)cargo clippy --manifest-path rust_hft/Cargo.toml -p hft-collector --all-targets --features collector-binance --no-deps --locked -- -D warningspassedcargo test --manifest-path rust_hft/Cargo.toml -p hft-collector --locked(292 passed, 2 ignored; all collector binaries/tests passed)rustfmt --edition 2021on the changed Rust files,git diff 9b8b36b6f6cfe6d805e7cd6e172b677a3ddd4ac3...HEAD --check, and.github/scripts/agent-worktree-preflight.shpassedRollout and rollback
No production rollout. The materializer is an offline/optional analytics-plane binary; rollback is removing this one PR. Any remote ClickHouse target must already satisfy the documented
ReplacingMergeTree(materialization_version)plusFINALregistry contract; incompatible state fails closed.Scope exception
One-time atomic exception approved by
/rootfor commit16b6fff85d15fb98423d791ce4dc522e7fce3796: four scoped files and 1,225 additions exceed the 750-line guideline because issue #653 is one inseparable materialization contract (verified replay input, immutable/idempotent registry, and three distinct writer schemas). Splitting would ship partial writers/lineage semantics and enlarge the trust surface. Remaining helper duplication is a low-risk follow-up only when another consumer needs shared contract code.Summary by CodeRabbit
New Features
Documentation
Tests