feat(sources): add ClickHouse table provider - #157
Merged
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
abbccdda
commented
Jul 16, 2026
abbccdda
left a comment
Contributor
Author
There was a problem hiding this comment.
Review: ClickHouse table provider
Overall a solid, well-tested addition that follows the house patterns (env-based credentials, eager schema fetch, read-only enforced at config/executor/provider layers, the count(*) wrapper mirroring influxdb.rs). Findings are posted as inline comments, ordered by production impact (#1–#5). I verified the upstream claims against the vendored datafusion-table-providers 0.10.1 source.
If prioritizing: #1 (best-effort catalog) before merge; #2/#3 are small changes with security/correctness payoff; #4/#5 can be fast-follows.
Minor
- Doc inaccuracy:
clickhouse.rs:256(and the README) say schema is inferred viaDESCRIBE TABLE; upstream actually usesSELECT * … LIMIT 0through the ArrowStream format plus an engine lookup insystem.tables. Worth correcting since the mechanism determines which failures occur (e.g. the Kafka-engine case in #1). - No timeout around per-table schema fetch:
build_clickhouse_table_providerisn't wrapped inretry_with_timeout, so an endpoint that hangs after listing succeeds stalls startup indefinitely. Matches mysql (inherited gap), so not a blocker — but catalog mode multiplies exposure by table count. - No config-validation tests: the DynamoDB PR added
validate_accepts_dynamodb_read_write/ catalog-option tests inconfig.rs; ClickHouse has none —read_writerejection rides the genericUnsupportedWriteModepath untested for this type, and a test for #3 (databasein catalog mode) would have caught that gap. - Type-coverage gap in fixtures: CI tables only use UInt32/String/Float64/Bool/DateTime/Nullable(String). The types most likely to break ClickHouse→Arrow mapping —
LowCardinality(String)(dictionary encoding can differ between the LIMIT-0 schema fetch and real data batches, and upstream derives the stream schema from the first batch),DateTime64,Decimal,Enum8,UUID,Array— are untested. One kitchen-sink types fixture would harden this a lot. A catalog-mode test including a VIEW would also cover the view branch in upstreamget_schema.
What checks out
- Read-only stance enforced consistently: config validation,
JobExecutordestination rejection (with test), no write path in the provider. - Option validation fires before any network call (tested offline-safe).
- Upstream behaviors the docs rely on verified against vendored 0.10.1 source:
schemas()excludessystem/information_schema, and nonexistentallowed_schemasentries contribute nothing rather than erroring.
Verification (run locally against clickhouse/clickhouse-server:24.8)
- Unit + integration tests:
cargo test -p skardi clickhouse -- --include-ignored→ 20 passed, 0 failed (12 unit + all 8 live integration tests: register/scan, filter pushdown, count(*) empty projection, NULL handling, empty-table schema inference, numeric aggregation, catalog mode, plus the executor destination-rejection test). - README demo, table mode: all 5 pipelines (
query_user_by_id,list_all_users,products_by_category,user_order_summary,federated_stock_value) return exactly the documented rows. One cosmetic nit:federated_stock_valuereturnsstock_value: 16999.390000000003where the README shows16999.39— consider aROUND(..., 2)in the pipeline or noting float formatting. - README demo, catalog mode: both
catalog_demopipelines return the documented rows; catalog registered 4 tables. - Finding #1 repro:
CREATE TABLE mydb.kafka_events (...) ENGINE = Kafka→ catalog-mode server exits 1 at startup withCode: 620 Direct select is not allowed. - Finding #2 repro: context with
connection_string: http://skardi_user:skardi_pass@localhost:8123and nouser_env/pass_env→ embedded creds ignored, 3 retries ofdefault: Authentication failed, registration fails; warning + full connection string appear in server logs.
abbccdda
added a commit
that referenced
this pull request
Jul 16, 2026
- Catalog mode is now best-effort (mirrors DynamoDB): a table whose schema fetch fails (broken view, permissions gap) is skipped with a warning instead of failing server startup. Stream-like engine tables (Kafka/RabbitMQ/NATS/FileLog) and materialized-view inner tables (.inner* names) are filtered out up front. - Introspection is one batched system.tables query instead of one query per database, so many databases no longer blow the single 5s retry window. Adds a direct dep on the same `clickhouse` client crate the upstream provider already uses. - URL-embedded credentials are a hard config error instead of a warning: the pool ignores them (auth would fail confusingly late) and the connection string is logged and exposed by the data-sources API. Connection strings are validated before they are logged. - `database` joins `table`/`schema` in the catalog-mode conflicting options list; added ClickHouse config-validation tests (read_write rejection, database-in-catalog rejection). - count(*) empty-projection scans stream the narrowest fixed-width column instead of column 0; the no-aggregate-pushdown limitation is documented in the README. - Per-table schema inference is wrapped in retry_with_timeout so a hanging endpoint can't stall startup. - Docs: schema inference is a SELECT ... LIMIT 0 probe, not DESCRIBE TABLE; federated_stock_value rounds stock_value to match the README. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bakey
reviewed
Jul 17, 2026
bakey
reviewed
Jul 17, 2026
Adds a read-only ClickHouse data source backed by the clickhouse feature of datafusion-table-providers. Filters, projections, and limits are unparsed back to ClickHouse SQL and executed server-side. - Table mode (default) registers a single table; catalog mode registers every table across non-system databases as name.<database>.<table>, with an allowed_schemas allow-list - Credentials via user_env/pass_env options, never embedded in the URL - Access is read-only: ClickHouse mutations are async background rewrites, so read_write access mode and job destinations are rejected - CI: ClickHouse 24.8 service container, seeded fixture tables, and #[ignore]-gated integration tests - Docs: quick-start guide, demo contexts, and federated pipelines under docs/clickhouse/ Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
sum() over Float64 products is sensitive to summation order, so the pipeline intermittently returned 16999.390000000003 where the README documents 16999.39. Round to cents so the response is deterministic. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Catalog mode is now best-effort (mirrors DynamoDB): a table whose schema fetch fails (broken view, permissions gap) is skipped with a warning instead of failing server startup. Stream-like engine tables (Kafka/RabbitMQ/NATS/FileLog) and materialized-view inner tables (.inner* names) are filtered out up front. - Introspection is one batched system.tables query instead of one query per database, so many databases no longer blow the single 5s retry window. Adds a direct dep on the same `clickhouse` client crate the upstream provider already uses. - URL-embedded credentials are a hard config error instead of a warning: the pool ignores them (auth would fail confusingly late) and the connection string is logged and exposed by the data-sources API. Connection strings are validated before they are logged. - `database` joins `table`/`schema` in the catalog-mode conflicting options list; added ClickHouse config-validation tests (read_write rejection, database-in-catalog rejection). - count(*) empty-projection scans stream the narrowest fixed-width column instead of column 0; the no-aggregate-pushdown limitation is documented in the README. - Per-table schema inference is wrapped in retry_with_timeout so a hanging endpoint can't stall startup. - Docs: schema inference is a SELECT ... LIMIT 0 probe, not DESCRIBE TABLE; federated_stock_value rounds stock_value to match the README. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Reject any query string in the connection URL: ClickHouse's HTTP interface accepts ?user=&password= as auth, which would sail past the embedded-credentials check and leak through logs and the data-sources API. The pool ignores query parameters anyway, so all of them are now a hard config error. - Enforce the option contract at the provider boundary so the CLI and public API get the same checks as server config validation: unknown option keys (e.g. a misspelled pass_env that would silently connect as the default user), mode-mismatched options (table/database in catalog mode, allowed_schemas in table mode), and an allowed_schemas with no non-empty entry are all rejected before any network call. - Extract the duplicated count(*) empty-projection workaround into a shared CountSafeTable in providers/mod.rs; the InfluxDB path now also streams the narrowest fixed-width column instead of column 0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
abbccdda
force-pushed
the
clickhouse-provider
branch
from
July 22, 2026 01:17
ad2e81a to
f647aa4
Compare
The second-round review fix moved option validation into register_clickhouse_tables so the CLI gets the same checks as server config validation — but access_mode: read_write still slipped through the CLI, which never passed it to the provider. Follow the OpenConnector/DynamoDB pattern: the provider takes read_write and hard-errors on it, and both front-ends forward their access mode. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
abbccdda
force-pushed
the
clickhouse-provider
branch
from
July 22, 2026 01:34
f647aa4 to
4c34bc1
Compare
The clickhouse_source test helper in server config tests predates the open_connector field on DataSource, leaving the branch's test build uncompilable after syncing with main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bakey
approved these changes
Jul 22, 2026
…aims Verified the README end-to-end against a live clickhouse-server:24.8 and system.query_log; two claims didn't hold: - Plain DateTime arrives as UInt32 (epoch seconds) and Date as UInt16 via the ArrowStream format; only DateTime64 maps to an Arrow Timestamp. - list_all_users' LIMIT is not pushed down: its ORDER BY isn't pushed, so the LIMIT above it runs in Skardi too. A bare LIMIT does push down. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
abbccdda
enabled auto-merge (squash)
July 22, 2026 13:45
abbccdda
disabled auto-merge
July 22, 2026 14:12
abbccdda
added a commit
that referenced
this pull request
Jul 23, 2026
* docs: add ClickHouse to architecture diagram Add a ClickHouse source cell (columnar OLAP) to both the interactive HTML diagram and the static SVG rendered inline in the README. The Supported Data Sources table already lists ClickHouse (#157); this brings the diagrams in line. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: enlarge diagram frame to contain taller sources card The previous commit grew the sources card for the ClickHouse row but left the outer frame, background, and grid rects at their old height, so the bottom source rows (ClickHouse / files / object stores) spilled past the border. Grow the frame (676->722), background (700->746), and grid (674->720) to restore the 16px card-to-frame gap and 12px margin. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
bakey
added a commit
that referenced
this pull request
Jul 23, 2026
Merging main brought in the ClickHouse provider (#157), whose two register_source tests still destructured new_session_context() as a 2-tuple; this branch extended it to also return the Open Connector gateway map. Update the new call sites to the 3-tuple and pass the map. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bakey
added a commit
that referenced
this pull request
Jul 23, 2026
…ty (#165) * feat(sources): Open Connector UDTFs, security policy, and observability Milestone 4 of the Open Connector integration: the interactive SQL surface, per docs/superpowers/specs/2026-07-11-open-connector-integration-tasks.md. - open_connector_query(gateway, 'pack.table', resource_json[, alias]): runs a built-in source-pack table without a persistent YAML binding, compiling into the exact scan a bound table uses — same stable schema, filter allowlist, fingerprint gate, safety bounds, and the same per-gateway scan cache. Plans against registration-time discovery, so an undiscovered action is a targeted planning error (planning never performs network I/O). - open_connector_scan(gateway, action_id, input_json, row_path[, alias]): executes an explicitly allowlisted raw read action once (new PaginationStrategy::SinglePage; always live, no filter pushdown), with a deterministic row type derived at planning time from the discovered output schema (raw_schema.rs) — primitives typed, ["T","null"] unions nullable, everything else opaque JSON; indeterminate shapes fail with an error recommending a source pack. - Security, default-deny and pre-HTTP: discovery gains a read_only flag; raw actions require allowlist membership AND an explicit read-only classification (mutating vs unclassified rejected with distinct errors before any request); tests pin that YAML bindings cannot override pack action/row_path/pagination/columns. - Observability: scan completion/failure tracing events with gateway, binding, table, action, cache hit, pages, rows, and duration — never tokens, inputs, or bodies. - Engine refactor to enable dynamic schemas: OpenConnectorExec takes an owned ScanTarget instead of &'static SourcePackTable; RowConverter accepts owned ColumnSpecs. - Wiring: register_open_connector_tables publishes a GatewayHandle into a shared OpenConnectorGateways map (server OptimizerRegistry / CLI), mirroring the DatasetRegistry pattern; UDTFs registered on both front-ends. - Docs: docs/open-connector.md guide, README supported-sources entry, milestone checklist updated. Verification: 147 open_connector tests (UDTF/YAML parity, shared-cache replay with zero new requests, single-POST raw scan, pre-HTTP security rejections asserted via recorded gateway traffic, federated CSV join); full skardi/server/CLI suites green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(sources): emit Open Connector scan-completion event with the final batch A satisfied downstream LIMIT drops the scan stream without another poll (DataFusion's LimitStream clears its input on the batch that fills the fetch), so the completion event — previously emitted only from the early-return branch of the NEXT next_page call — never fired for LIMIT-satisfied scans. The docs' every-scan / exactly-once claims did not hold for the most common query shape. Log eagerly instead, with the final batch, wherever the scan is known complete (guarded by completion_logged): - live path: after the row counter includes the final batch, whenever `done` is set (covers LIMIT-satisfied AND short-final-page exhaustion, which had the same latent dependence on one more poll); - cache-replay path: when the replay queue drains (cached LIMIT queries replay under the same never-polled-again consumer). Logging after the row-count update keeps the event's `rows` field accurate — logging inside the LIMIT branch itself would under-count by the final batch. Regression tests drive ScanState directly against the mock gateway and assert completion_logged flips exactly on the final batch for all three shapes (LIMIT-satisfied, non-empty terminal page, cache replay), plus accurate row totals. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(sources): state the Open Connector failure-event contents accurately The scan-failure event's comment, docs/open-connector.md, and the milestone checklist claimed errors "carry JSON kinds only" / "response bodies are never logged". That is true for conversion and row-path failures, but ActionExecutionFailed deliberately quotes a bounded (<=512-char) snippet of the gateway's *error* response (the milestone-2 diagnostic for terminal provider failures, which can echo request identifiers such as owner/repo), and PaginationLoop carries the offending cursor. Align the three claims with reality instead of filtering the event: the same error display also reaches the SQL client and the front-ends' registration logs, so redacting one log site would not make the promise true — it would only strip the diagnostic where operators look first. The design spec's actual commitment (no tokens, credentials, authorization headers, or full sensitive inputs) was never violated and is now what the comment, guide, and checklist say. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(sources): pin Open Connector scan events with a tracing capture The completion/failure-event invariants were asserted only through ScanState's completion_logged flag — nothing verified that the events are actually emitted, exactly once, with the documented fields. Add a test-only tracing capture to testutil (a minimal Subscriber recording level/message/fields into a shared vec, installed as a thread-local default — #[tokio::test] bodies run single-threaded, so parallel tests stay isolated) and four event-level tests that consume the real execute() stream: - LIMIT-satisfied scan: one batch polled, stream dropped without a further poll (the LimitStream shape) → exactly one INFO completion event with gateway/binding/table/action, cache_hit=false, pages=1, rows=1, duration_ms; - empty scan: polled to None (and once past it) → exactly one completion with rows=0; - cache replay: two full scans → exactly two completions, the second cache_hit=true / pages=0 / rows=3 (the done-branch re-log is guarded, so draining the queue and polling to None stays at one); - terminal 5xx on execute → exactly one WARN failure event with the scan identity, pages_fetched=0, and the HTTP status in the error — and no completion event. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(sources): convert JSON null to SQL NULL in opaque Json columns The FieldType::Json arm stringified cells directly, so a present JSON null (Some(&Value::Null) — RowPath::extract returns the value when the key exists) became the 4-char string "null" instead of Arrow null, ignoring the column's nullability. Every other arm already routes present nulls through collect_cells (nullable -> Arrow null, non-nullable -> targeted per-column failure). This got hot with open_connector_scan: raw schemas type every object/array/wide-union field as nullable Json, and provider nulls (assignee: null, user: null) are ubiquitous — WHERE x IS NULL matched nothing while x = 'null' matched. Route the Json arm through collect_cells like the rest: JSON null is SQL NULL for nullable columns and a targeted ConversionFailed (found: "null") for non-nullable ones, instead of surfacing later as a batch-level nullability error. Tests: unit coverage for present-null vs absent-key (both Arrow null) and the non-nullable failure; an end-to-end raw-scan test pinning the SQL semantics (IS NULL matches the provider null, = 'null' does not). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(sources): pin the fetch-time semantics of pages_fetched A review suggested moving the increment after the post-fetch deadline check so a page that lands right at the deadline is not counted in the failure event. Declined: pages_fetched measures gateway traffic (requests actually made, rate-limit budget actually spent), not pages emitted — a failed scan emits nothing at all, so an emission reading of the field is meaningless there, and moving the increment would under-report real gateway load in timeout diagnostics while staying inconsistent with the extraction/conversion failure paths right below. Document the intent at the increment so the placement reads as a decision, not an accident. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(sources): document the registration-snapshot staleness window The raw-scan security gates (allowlist + read-only classification) read metadata discovered at registration; planning never re-contacts the gateway by design. An upstream action that turns mutating after registration therefore keeps passing the Skardi-side gate until the next restart or configuration reload. Spell that window out in the UDTF module docs and the security-model section of the guide — the same snapshot covers executability and contract fingerprints — and name Open Connector's own action policies as the live, independent boundary during it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(cli): thread UDTF gateways through the ClickHouse test call sites Merging main brought in the ClickHouse provider (#157), whose two register_source tests still destructured new_session_context() as a 2-tuple; this branch extended it to also return the Open Connector gateway map. Update the new call sites to the 3-tuple and pass the map. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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
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.
Summary
Adds ClickHouse as a read-only data source, backed by the
clickhousefeature ofdatafusion-table-providers. Filters, projections, andLIMITs are unparsed back to ClickHouse SQL and executed server-side, so scans stream only the rows a query actually needs — and ClickHouse tables federate with any other Skardi source in a single query.Details
databaseoption.name.<database>.<table>, filtered by an optionalallowed_schemasallow-list.user_env/pass_envoptions and are never embedded in the connection URL.ALTER TABLE ... UPDATE/DELETE) are asynchronous background rewrites and a mid-stream INSERT failure leaves partial parts visible, which doesn't match the semantics Skardi's write path promises.access_mode: read_writeand job destinations are rejected for ClickHouse sources.clickhouse/clickhouse-server:24.8service container with seeded fixture tables (including a NULL-column and an empty table for schema-inference coverage); integration tests are#[ignore]-gated and run via the existing--ignoredpass..inner*tables filtered up front, and introspection batched into a singlesystem.tablesquery.allowed_schemas, URL-embedded credentials, query strings, andaccess_mode: read_writeare all hard errors inregister_clickhouse_tablesitself, so the CLI and public API enforce the same contract as server config validation — and secrets can't reach logs or the data-sources API.docs/clickhouse/, plus a README source-matrix row.Test plan
DataSourceTyperoundtrip (cargo nextest run)#[ignore]-gated integration tests against the CI ClickHouse service container: registration, scans, filter/limit pushdown, NULL handling, empty-table schema inference, catalog modesubmit_rejects_clickhouse_destination_as_non_transactionalcovers the job-destination rejection🤖 Generated with Claude Code