feat: add ClickHouse warehouse driver#574
Merged
anandgupta42 merged 6 commits intomainfrom Mar 30, 2026
Merged
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
📝 WalkthroughWalkthroughThis PR adds first-class ClickHouse support: a new ClickHouse driver implementation, full integration across registry/discovery/dbt/finops, E2E test suites for multiple ClickHouse versions, CI wiring, peer-dependency/publish updates, normalization and credential handling, and comprehensive documentation and contributor checklists. Changes
Sequence Diagram(s)sequenceDiagram
participant Altimate as Altimate App
participant Registry as Connection Registry
participant DriverMod as `@altimateai/drivers/clickhouse`
participant ClickHouse as ClickHouse Server
Altimate->>Registry: request connector(type="clickhouse", config)
Registry->>DriverMod: dynamic import
DriverMod-->>Registry: connect(config) -> Connector
Registry-->>Altimate: Connector
Altimate->>DriverMod: Connector.connect()
DriverMod->>ClickHouse: HTTP/HTTPS requests (JSONEachRow, system.* queries)
ClickHouse-->>DriverMod: results/metadata
DriverMod-->>Altimate: execute/listSchemas/listTables/describeTable responses
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Add first-class ClickHouse support as the 12th database driver: **Driver (`packages/drivers/src/clickhouse.ts`):** - Official `@clickhouse/client` over HTTP(S) - Supports ClickHouse server 23.3+ (all non-EOL versions) - Password, connection string, and TLS/mTLS auth - ClickHouse Cloud and self-hosted compatible - Parameterized queries for SQL injection prevention - DML-aware LIMIT injection (won't break `WITH...INSERT`) **Integration (23 touchpoints):** - Registry: `DRIVER_MAP`, import switch, `PASSWORD_DRIVERS` - Discovery: Docker containers, env vars (`CLICKHOUSE_HOST`/`CLICKHOUSE_URL`), dbt profiles (`ADAPTER_TYPE_MAP`), dbt lineage dialect - FinOps: `system.query_log` query history template - Normalization: aliases for `connectionString`, `requestTimeout`, TLS fields - Publish: `@clickhouse/client` in `peerDependencies` **Tests:** - 30+ E2E tests across 5 suites (latest, LTS 23.8, 24.3, 24.8, connection string) - 14 config normalization tests for all ClickHouse aliases - MergeTree variants, materialized views, Nullable columns, Array/Map/IPv4 types **Documentation:** - Full config section in `warehouses.md` (standard, Cloud, connection string) - Support matrix entry in `drivers.md` with auth methods - Dedicated guide (`guides/clickhouse.md`): MergeTree optimization, materialized view pipelines, dialect translation, LowCardinality tips, dbt integration - Updated README, getting-started, warehouse-tools docs **Engineering:** - `packages/drivers/ADDING_A_DRIVER.md` — 23-point checklist for adding future drivers - `.claude/commands/add-database-driver.md` — Claude skill to automate the process Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
anandgupta42
force-pushed
the
feat/add-clickhouse-driver
branch
from
350e956 to
e63d6c6
Compare
- `execute()` now uses `client.command()` for INSERT/CREATE/DROP/ALTER queries instead of `client.query()` with JSONEachRow format, which caused parse errors on INSERT VALUES - Add `CLICKHOUSE_DEFAULT_ACCESS_MANAGEMENT=1` to all LTS Docker containers (required for passwordless default user) - Fix UInt64 assertion to handle both string and number JSON encoding Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Add `clickhouse/clickhouse-server:latest` as a GitHub Actions service - Add test step running `drivers-clickhouse-e2e.test.ts` with CI env vars - Add test file to change detection paths for the `drivers` filter Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…ilures) Ran 167 adversarial tests against real ClickHouse Docker containers covering SQL injection, unicode, NULLs, LIMIT edge cases, exotic types, error handling, large data, MergeTree variants, views, system tables, concurrent operations, and return value edge cases. **Bugs found and fixed:** 1. **DESCRIBE/EXISTS get LIMIT appended** — `isSelectLike` regex matched DESCRIBE/EXISTS but ClickHouse doesn't support LIMIT on these statements. Fix: narrowed `supportsLimit` to only `SELECT` and `WITH` queries. 2. **`limit=0` returns 0 rows** — truncation check `rows.length > 0` was always true, causing `slice(0, 0)` to return empty array. Fix: guard with `effectiveLimit > 0 &&` before truncation check. 3. **`limit=0` treated as `limit=1000`** — `0 ?? 1000` returns 0 (correct) but `limit === undefined ? 1000 : limit` properly distinguishes "not provided" from "explicitly zero". Changed from `??` to explicit check. **Regression tests added (5 tests in main E2E suite):** - DESCRIBE TABLE without LIMIT error - EXISTS TABLE without LIMIT error - limit=0 returns all rows without truncation - INSERT uses `client.command()` not `client.query()` - WITH...INSERT does not get LIMIT appended Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
LTS 24.3 and 24.8 test suites incorrectly used `CH_USE_CI` (tied to `TEST_CLICKHOUSE_HOST`) which made them skip Docker startup in CI but still try to connect on ports 18125/18126 — causing 60s timeouts. Give each LTS suite its own `_USE_CI` flag (`CH_243_USE_CI`, `CH_248_USE_CI`) so they properly fall through to Docker-start-or-skip behavior, matching how LTS 23.8 already works with `CH_LTS_USE_CI`. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
packages/opencode/test/altimate/driver-normalize.test.ts (1)
887-922: Add coverage for the remaining ClickHouse TLS aliases (ca_cert,ssl_cert).
CLICKHOUSE_ALIASESincludes both, but this suite currently doesn’t assert them. Adding these two cases will make alias coverage complete and protect against regressions.✅ Suggested additional tests
+ test("ca_cert → tls_ca_cert", () => { + const result = normalizeConfig({ + type: "clickhouse", + ca_cert: "/path/to/ca.pem", + }) + expect(result.tls_ca_cert).toBe("/path/to/ca.pem") + expect(result.ca_cert).toBeUndefined() + }) + + test("ssl_cert → tls_cert", () => { + const result = normalizeConfig({ + type: "clickhouse", + ssl_cert: "/path/to/cert.pem", + }) + expect(result.tls_cert).toBe("/path/to/cert.pem") + expect(result.ssl_cert).toBeUndefined() + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/altimate/driver-normalize.test.ts` around lines 887 - 922, Add two tests to fully cover ClickHouse TLS aliases: create a test named "ca_cert → tls_ca_cert" that calls normalizeConfig({ type: "clickhouse", ca_cert: "/path/to/ca.pem" }) and assert result.tls_ca_cert === "/path/to/ca.pem" and result.ca_cert is undefined; and create a test named "ssl_cert → tls_cert" that calls normalizeConfig({ type: "clickhouse", ssl_cert: "/path/to/cert.pem" }) and assert result.tls_cert === "/path/to/cert.pem" and result.ssl_cert is undefined; reference normalizeConfig and the new test names when locating where to add them in the suite.packages/opencode/src/altimate/native/dbt/lineage.ts (1)
103-113: Consider de-duplicating adapter→dialect mapping to avoid drift.
detectDialect()now has its own map including ClickHouse, whilepackages/opencode/src/altimate/native/connections/dbt-profiles.tsmaintains another adapter map. A shared constant would reduce future mismatch risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/native/dbt/lineage.ts` around lines 103 - 113, detectDialect() duplicates an adapter→dialect map that also exists in the dbt-profiles adapter map; extract a single shared constant (e.g., export ADAPTER_TO_DIALECT or ADAPTER_DIALECT_MAP) into a common module and replace the local dialectMap in detectDialect() with an import of that constant, then update the other module to import the same constant instead of maintaining its own map so both detectDialect() and the dbt-profiles adapter mapping reference the single source of truth.packages/opencode/src/altimate/tools/warehouse-add.ts (1)
14-29: Document MongoDB in the canonical field list too.Line 36 now advertises
mongodbas a supportedconfig.type, but this description block still omits it. Since this text is what the tool planner sees, the mismatch makes MongoDB additions less reliable than the runtime behavior.Also applies to: 36-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/tools/warehouse-add.ts` around lines 14 - 29, The config description string on the z.record(...) describe(...) (symbol: config) is missing canonical MongoDB fields even though mongodb is advertised as a supported config.type; update that long description to include a "mongodb" bullet with canonical fields (e.g., uri/connection_string, host, port, database, user, password, authSource, replicaSet, tls/tlsCAFile/tlsCertificateKeyFile, ssl, replicaSet) and add a short MongoDB auth/connection example so the planner and runtime are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/commands/add-database-driver.md:
- Around line 129-142: The test block currently runs "cd packages/opencode &&
bun test test/altimate/driver-normalize.test.ts
test/altimate/connections.test.ts" which leaves the shell in packages/opencode
so the subsequent "bun turbo typecheck" and "bun run script/upstream/analyze.ts
--markers --base main --strict" run from the wrong directory and the E2E suite
isn't executed; change the test invocation to run in a subshell (e.g., wrap the
bun test invocation in parentheses) and include
"test/altimate/drivers-{database}-e2e.test.ts" in the bun test arguments so the
new driver’s Docker tests execute, and ensure "bun turbo typecheck" and "bun run
script/upstream/analyze.ts --markers --base main --strict" are executed from the
repo root (i.e., not affected by the cd).
In `@docs/docs/configure/warehouses.md`:
- Around line 291-350: The docs still contain stale references that mark
ClickHouse as unsupported and omit it from Docker auto-discovery; locate the
ClickHouse section and any mentions of “unsupported” or “not supported”
referring to ClickHouse (search for the "ClickHouse" heading and the Docker
auto-discovery paragraph) and remove or update those lines to reflect ClickHouse
is supported, and add ClickHouse to the Docker auto-discovery list/description
where other supported warehouses are enumerated (also update the other two
occurrences mentioned in the review). Ensure the wording matches the new
supported phrasing used in the ClickHouse section (e.g., reference supported
versions and cloud options).
In `@docs/docs/drivers.md`:
- Around line 138-144: Add blank lines before and after the ClickHouse table
block to satisfy markdownlint MD058: ensure there's an empty line between the
"### ClickHouse" header and the table start, and another empty line after the
table end; edit the block containing the "### ClickHouse" header and the
subsequent table (`| Method | Config Fields |` ... `| TLS/HTTPS | ... |`) so the
table is separated by blank lines from surrounding text.
In `@packages/drivers/ADDING_A_DRIVER.md`:
- Line 181: Add a language tag to the fenced code block at the shown snippet in
ADDING_A_DRIVER.md (the file-map block starting with "packages/drivers/") to
satisfy markdownlint MD040; update the opening triple-backticks to include a
language like text (e.g., change ``` to ```text) so the block is explicitly
marked as plain text.
In `@packages/drivers/src/clickhouse.ts`:
- Line 63: The execute(sql: string, limit?: number, _binds?: any[]) function
currently accepts but ignores _binds and then runs raw SQL (the raw query
execution block), which can silently bypass parameterization; update execute to
fail fast when binds are provided or implement proper parameter mapping—e.g., at
the start of execute() check if _binds is non-empty and throw a clear error like
"binds not supported for ClickHouse execute" (or alternatively map _binds into
ClickHouse parameters and pass them to the client query API), and ensure the raw
SQL execution code path (the block that currently executes the text query) is
only used when no binds are supplied.
In `@packages/drivers/src/normalize.ts`:
- Around line 90-93: The config normalization maps ssl_* → tls_* (see
normalizeConfig), but saveConnection uses SENSITIVE_FIELDS to strip secrets and
currently lacks the normalized keys; update the SENSITIVE_FIELDS array (in
credential-store.ts where SENSITIVE_FIELDS is defined) to include "tls_ca_cert",
"tls_cert", and "tls_key" so that saveConnection will recognize and strip the
normalized tls_* fields after normalizeConfig runs.
In `@packages/opencode/src/altimate/native/finops/query-history.ts`:
- Around line 169-175: The ClickHouse branch is embedding unvalidated days and
limit into CLICKHOUSE_HISTORY_SQL; clamp and sanitize days and limit before
substitution to prevent NaN/Infinity/negative values producing invalid UInt32
SQL. Convert inputs with Number(...), treat non-finite values as 0, clamp to the
UInt32 range (0 to 4294967295), and use Math.floor on the sanitized value before
replacing the "{days:UInt32}" and "{limit:UInt32}" tokens in the
CLICKHOUSE_HISTORY_SQL string inside the whType === "clickhouse" block so the
returned sql always contains valid UInt32 integers.
In `@packages/opencode/src/altimate/tools/project-scan.ts`:
- Around line 224-235: The new ClickHouse entry (type: "clickhouse") was added
but the generic DATABASE_URL classifier still maps unknown schemes to Postgres,
so DATABASE_URL=clickhouse://... will be misclassified; update the DATABASE_URL
parsing logic in project-scan.ts to recognize the "clickhouse" scheme (and any
common aliases like "clickhouse+http"/"clickhouse+native" if used) and return
the "clickhouse" type instead of defaulting to "postgres", making the classifier
consistent with the configMap for the clickhouse entry.
In `@packages/opencode/test/altimate/drivers-clickhouse-e2e.test.ts`:
- Around line 25-30: The waitForPort helper currently hardcodes the host to
"127.0.0.1" (see function waitForPort and its use of createConnection), which
breaks tests when suites configure a different host; change waitForPort
signature to accept an optional host parameter (defaulting to "127.0.0.1") and
use that host when calling createConnection, then update all call sites (the
tests around lines referenced in the comment) to pass their configured host
instead of relying on localhost; apply the same change to the other versioned
suites that copy this helper so all readiness checks use the suite-configured
host.
- Around line 51-69: The waitForDbReady function leaks connectors on failed
attempts: after calling connectFn() you must ensure any connector returned is
closed when the attempt fails; modify waitForDbReady (around the
connectFn/connector usage) to call and await a safe close/disconnect on the
returned connector (e.g., await connector.close() or connector.disconnect() if
present) inside the catch (or a per-attempt finally) and guard the call with a
typeof/exists check and its own try/catch so close errors don’t mask the
original error; keep using lastErr for the final thrown message.
---
Nitpick comments:
In `@packages/opencode/src/altimate/native/dbt/lineage.ts`:
- Around line 103-113: detectDialect() duplicates an adapter→dialect map that
also exists in the dbt-profiles adapter map; extract a single shared constant
(e.g., export ADAPTER_TO_DIALECT or ADAPTER_DIALECT_MAP) into a common module
and replace the local dialectMap in detectDialect() with an import of that
constant, then update the other module to import the same constant instead of
maintaining its own map so both detectDialect() and the dbt-profiles adapter
mapping reference the single source of truth.
In `@packages/opencode/src/altimate/tools/warehouse-add.ts`:
- Around line 14-29: The config description string on the z.record(...)
describe(...) (symbol: config) is missing canonical MongoDB fields even though
mongodb is advertised as a supported config.type; update that long description
to include a "mongodb" bullet with canonical fields (e.g.,
uri/connection_string, host, port, database, user, password, authSource,
replicaSet, tls/tlsCAFile/tlsCertificateKeyFile, ssl, replicaSet) and add a
short MongoDB auth/connection example so the planner and runtime are consistent.
In `@packages/opencode/test/altimate/driver-normalize.test.ts`:
- Around line 887-922: Add two tests to fully cover ClickHouse TLS aliases:
create a test named "ca_cert → tls_ca_cert" that calls normalizeConfig({ type:
"clickhouse", ca_cert: "/path/to/ca.pem" }) and assert result.tls_ca_cert ===
"/path/to/ca.pem" and result.ca_cert is undefined; and create a test named
"ssl_cert → tls_cert" that calls normalizeConfig({ type: "clickhouse", ssl_cert:
"/path/to/cert.pem" }) and assert result.tls_cert === "/path/to/cert.pem" and
result.ssl_cert is undefined; reference normalizeConfig and the new test names
when locating where to add them in the suite.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5211562f-d2d1-4cd7-8599-181354eeec9b
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.claude/commands/add-database-driver.md.github/meta/commit.txt.github/workflows/ci.ymlREADME.mddocs/docs/configure/warehouses.mddocs/docs/data-engineering/guides/clickhouse.mddocs/docs/data-engineering/guides/index.mddocs/docs/data-engineering/tools/warehouse-tools.mddocs/docs/drivers.mddocs/docs/getting-started/index.mddocs/mkdocs.ymlpackages/drivers/ADDING_A_DRIVER.mdpackages/drivers/package.jsonpackages/drivers/src/clickhouse.tspackages/drivers/src/index.tspackages/drivers/src/normalize.tspackages/opencode/.github/meta/commit.txtpackages/opencode/script/publish.tspackages/opencode/src/altimate/native/connections/dbt-profiles.tspackages/opencode/src/altimate/native/connections/docker-discovery.tspackages/opencode/src/altimate/native/connections/registry.tspackages/opencode/src/altimate/native/dbt/lineage.tspackages/opencode/src/altimate/native/finops/query-history.tspackages/opencode/src/altimate/tools/project-scan.tspackages/opencode/src/altimate/tools/warehouse-add.tspackages/opencode/test/altimate/driver-normalize.test.tspackages/opencode/test/altimate/drivers-clickhouse-e2e.test.ts
Comment on lines
+138
to
+144
| ### ClickHouse | ||
| | Method | Config Fields | | ||
| |--------|--------------| | ||
| | Password | `host`, `port`, `database`, `user`, `password` | | ||
| | Connection String | `connection_string: "http://user:pass@host:8123"` | | ||
| | TLS/HTTPS | `protocol: "https"`, `tls_ca_cert`, `tls_cert`, `tls_key` | | ||
|
|
There was a problem hiding this comment.
Add blank lines around the ClickHouse auth table to satisfy markdownlint.
MD058 is triggered here; add spacing before/after the table block.
🧹 Minimal lint fix
### ClickHouse
+
| Method | Config Fields |
|--------|--------------|
| Password | `host`, `port`, `database`, `user`, `password` |
| Connection String | `connection_string: "http://user:pass@host:8123"` |
| TLS/HTTPS | `protocol: "https"`, `tls_ca_cert`, `tls_cert`, `tls_key` |
+
ClickHouse driver supports server versions 23.3+ (all non-EOL releases). Uses the official `@clickhouse/client` package over HTTP(S). Compatible with ClickHouse Cloud, self-hosted, and Altinity.Cloud. Query history available via `system.query_log`.📝 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.
Suggested change
| ### ClickHouse | |
| | Method | Config Fields | | |
| |--------|--------------| | |
| | Password | `host`, `port`, `database`, `user`, `password` | | |
| | Connection String | `connection_string: "http://user:pass@host:8123"` | | |
| | TLS/HTTPS | `protocol: "https"`, `tls_ca_cert`, `tls_cert`, `tls_key` | | |
| ### ClickHouse | |
| | Method | Config Fields | | |
| |--------|--------------| | |
| | Password | `host`, `port`, `database`, `user`, `password` | | |
| | Connection String | `connection_string: "http://user:pass@host:8123"` | | |
| | TLS/HTTPS | `protocol: "https"`, `tls_ca_cert`, `tls_cert`, `tls_key` | | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 139-139: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/docs/drivers.md` around lines 138 - 144, Add blank lines before and
after the ClickHouse table block to satisfy markdownlint MD058: ensure there's
an empty line between the "### ClickHouse" header and the table start, and
another empty line after the table end; edit the block containing the "###
ClickHouse" header and the subsequent table (`| Method | Config Fields |` ... `|
TLS/HTTPS | ... |`) so the table is separated by blank lines from surrounding
text.
- Remove stale ClickHouse entry from "Unsupported Databases" doc section - Add ClickHouse to Docker auto-discovery description in docs - Add blank line around ClickHouse auth table for markdownlint MD058 - Add `text` language tag to fenced code block for markdownlint MD040 - Fail fast when `binds` passed to ClickHouse `execute()` instead of ignoring - Add `tls_key`, `tls_cert`, `tls_ca_cert` to SENSITIVE_FIELDS in credential store - Clamp `days`/`limit` values in ClickHouse query history SQL builder - Add `clickhouse`, `clickhouse+http`, `clickhouse+https` to DATABASE_URL scheme map - Make `waitForPort` accept configurable host in E2E tests - Close failed connectors during `waitForDbReady` retries in E2E tests - Add missing TLS alias tests: `ca_cert`, `ssl_cert`, `ssl_key` Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.claude/commands/add-database-driver.md (2)
27-38: Add language specifier to code block.The research template code block lacks a language identifier, which triggers markdownlint MD040.
📝 Suggested fix
Present findings to the user before proceeding: -``` +```markdown ## Research: {Database}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/commands/add-database-driver.md around lines 27 - 38, Update the fenced code block in .claude/commands/add-database-driver.md (the "## Research: {Database}" template) to include a language specifier (e.g., change the opening ``` to ```markdown) so the block is marked as markdown and markdownlint MD040 is resolved; ensure only the opening fence is modified and content remains unchanged.
151-172: Add language specifier to summary template code block.Same MD040 issue — the summary template needs a language identifier.
📝 Suggested fix
Present final summary: -``` +```markdown ## {Database} Driver Added🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/commands/add-database-driver.md around lines 151 - 172, The Markdown code block used as the summary template in .claude/commands/add-database-driver.md is missing a language specifier which triggers MD040; update the opening fence from ``` to ```markdown so the template block (the "## {Database} Driver Added" section) is explicitly marked as markdown. Ensure the triple-backtick fence that begins the template is changed to include "markdown" and leave the rest of the block unchanged.packages/opencode/test/altimate/drivers-clickhouse-e2e.test.ts (1)
115-115: Pass configured host to waitForPort for consistency.The
waitForPortfunction now accepts a host parameter, but call sites don't pass the configured host. WhenCH_HOSTis set to a non-localhost value viaTEST_CLICKHOUSE_HOST, the port check still targets127.0.0.1.🔧 Suggested fix
- await waitForPort(CH_PORT, 60000) + await waitForPort(CH_PORT, 60000, CH_HOST)Similar updates needed at lines 436, 542, 620, 703 for their respective host variables.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/altimate/drivers-clickhouse-e2e.test.ts` at line 115, Calls to waitForPort (e.g., await waitForPort(CH_PORT, 60000)) are missing the host argument so they always check 127.0.0.1; update each call to pass the configured host variable (the value coming from TEST_CLICKHOUSE_HOST / CH_HOST) as the second parameter, e.g., change waitForPort(CH_PORT, 60000) to waitForPort(CH_PORT, CH_HOST, 60000) and make the same change for the other occurrences referenced in the review (the calls at the other test locations should pass their respective host variables as the second argument to waitForPort).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/test/altimate/drivers-clickhouse-e2e.test.ts`:
- Around line 65-68: In the catch block where the test currently attempts
connector?.disconnect?.(), replace that call with the Connector's actual cleanup
method connector?.close?.() so the cleanup runs; update the catch surrounding
code that references disconnect to call close() on the connector variable
(preserving the optional chaining and the existing empty catch), ensuring the
Connector interface's close() is invoked for proper teardown.
---
Nitpick comments:
In @.claude/commands/add-database-driver.md:
- Around line 27-38: Update the fenced code block in
.claude/commands/add-database-driver.md (the "## Research: {Database}" template)
to include a language specifier (e.g., change the opening ``` to ```markdown) so
the block is marked as markdown and markdownlint MD040 is resolved; ensure only
the opening fence is modified and content remains unchanged.
- Around line 151-172: The Markdown code block used as the summary template in
.claude/commands/add-database-driver.md is missing a language specifier which
triggers MD040; update the opening fence from ``` to ```markdown so the template
block (the "## {Database} Driver Added" section) is explicitly marked as
markdown. Ensure the triple-backtick fence that begins the template is changed
to include "markdown" and leave the rest of the block unchanged.
In `@packages/opencode/test/altimate/drivers-clickhouse-e2e.test.ts`:
- Line 115: Calls to waitForPort (e.g., await waitForPort(CH_PORT, 60000)) are
missing the host argument so they always check 127.0.0.1; update each call to
pass the configured host variable (the value coming from TEST_CLICKHOUSE_HOST /
CH_HOST) as the second parameter, e.g., change waitForPort(CH_PORT, 60000) to
waitForPort(CH_PORT, CH_HOST, 60000) and make the same change for the other
occurrences referenced in the review (the calls at the other test locations
should pass their respective host variables as the second argument to
waitForPort).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 41a8cf32-ea3e-44a6-9caa-e31e70d99eba
📒 Files selected for processing (10)
.claude/commands/add-database-driver.mddocs/docs/configure/warehouses.mddocs/docs/drivers.mdpackages/drivers/ADDING_A_DRIVER.mdpackages/drivers/src/clickhouse.tspackages/opencode/src/altimate/native/connections/credential-store.tspackages/opencode/src/altimate/native/finops/query-history.tspackages/opencode/src/altimate/tools/project-scan.tspackages/opencode/test/altimate/driver-normalize.test.tspackages/opencode/test/altimate/drivers-clickhouse-e2e.test.ts
✅ Files skipped from review due to trivial changes (4)
- docs/docs/drivers.md
- packages/opencode/test/altimate/driver-normalize.test.ts
- packages/drivers/ADDING_A_DRIVER.md
- packages/opencode/src/altimate/tools/project-scan.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/drivers/src/clickhouse.ts
Comment on lines
+65
to
+68
| } catch (e: any) { | ||
| lastErr = e | ||
| try { connector?.disconnect?.() } catch {} | ||
| await new Promise((r) => setTimeout(r, 2000)) |
There was a problem hiding this comment.
Use close() instead of disconnect() for connector cleanup.
The Connector interface (from packages/drivers/src/types.ts) defines close(), not disconnect(). The optional chaining connector?.disconnect?.() will silently do nothing since the method doesn't exist.
🔧 Suggested fix
} catch (e: any) {
lastErr = e
- try { connector?.disconnect?.() } catch {}
+ try { await connector?.close?.() } catch {}
await new Promise((r) => setTimeout(r, 2000))
}📝 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.
Suggested change
| } catch (e: any) { | |
| lastErr = e | |
| try { connector?.disconnect?.() } catch {} | |
| await new Promise((r) => setTimeout(r, 2000)) | |
| } catch (e: any) { | |
| lastErr = e | |
| try { await connector?.close?.() } catch {} | |
| await new Promise((r) => setTimeout(r, 2000)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/altimate/drivers-clickhouse-e2e.test.ts` around lines
65 - 68, In the catch block where the test currently attempts
connector?.disconnect?.(), replace that call with the Connector's actual cleanup
method connector?.close?.() so the cleanup runs; update the catch surrounding
code that references disconnect to call close() on the connector variable
(preserving the optional chaining and the existing empty catch), ensuring the
Connector interface's close() is invoked for proper teardown.
kulvirgit
pushed a commit
that referenced
this pull request
Mar 30, 2026
* feat: add ClickHouse warehouse driver with full integration Add first-class ClickHouse support as the 12th database driver: **Driver (`packages/drivers/src/clickhouse.ts`):** - Official `@clickhouse/client` over HTTP(S) - Supports ClickHouse server 23.3+ (all non-EOL versions) - Password, connection string, and TLS/mTLS auth - ClickHouse Cloud and self-hosted compatible - Parameterized queries for SQL injection prevention - DML-aware LIMIT injection (won't break `WITH...INSERT`) **Integration (23 touchpoints):** - Registry: `DRIVER_MAP`, import switch, `PASSWORD_DRIVERS` - Discovery: Docker containers, env vars (`CLICKHOUSE_HOST`/`CLICKHOUSE_URL`), dbt profiles (`ADAPTER_TYPE_MAP`), dbt lineage dialect - FinOps: `system.query_log` query history template - Normalization: aliases for `connectionString`, `requestTimeout`, TLS fields - Publish: `@clickhouse/client` in `peerDependencies` **Tests:** - 30+ E2E tests across 5 suites (latest, LTS 23.8, 24.3, 24.8, connection string) - 14 config normalization tests for all ClickHouse aliases - MergeTree variants, materialized views, Nullable columns, Array/Map/IPv4 types **Documentation:** - Full config section in `warehouses.md` (standard, Cloud, connection string) - Support matrix entry in `drivers.md` with auth methods - Dedicated guide (`guides/clickhouse.md`): MergeTree optimization, materialized view pipelines, dialect translation, LowCardinality tips, dbt integration - Updated README, getting-started, warehouse-tools docs **Engineering:** - `packages/drivers/ADDING_A_DRIVER.md` — 23-point checklist for adding future drivers - `.claude/commands/add-database-driver.md` — Claude skill to automate the process Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix: use `client.command()` for ClickHouse DDL/DML, fix E2E test auth - `execute()` now uses `client.command()` for INSERT/CREATE/DROP/ALTER queries instead of `client.query()` with JSONEachRow format, which caused parse errors on INSERT VALUES - Add `CLICKHOUSE_DEFAULT_ACCESS_MANAGEMENT=1` to all LTS Docker containers (required for passwordless default user) - Fix UInt64 assertion to handle both string and number JSON encoding Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * ci: add ClickHouse E2E tests to driver-e2e CI job - Add `clickhouse/clickhouse-server:latest` as a GitHub Actions service - Add test step running `drivers-clickhouse-e2e.test.ts` with CI env vars - Add test file to change detection paths for the `drivers` filter Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix: 3 driver bugs found by adversarial testing (167 tests, 3 real failures) Ran 167 adversarial tests against real ClickHouse Docker containers covering SQL injection, unicode, NULLs, LIMIT edge cases, exotic types, error handling, large data, MergeTree variants, views, system tables, concurrent operations, and return value edge cases. **Bugs found and fixed:** 1. **DESCRIBE/EXISTS get LIMIT appended** — `isSelectLike` regex matched DESCRIBE/EXISTS but ClickHouse doesn't support LIMIT on these statements. Fix: narrowed `supportsLimit` to only `SELECT` and `WITH` queries. 2. **`limit=0` returns 0 rows** — truncation check `rows.length > 0` was always true, causing `slice(0, 0)` to return empty array. Fix: guard with `effectiveLimit > 0 &&` before truncation check. 3. **`limit=0` treated as `limit=1000`** — `0 ?? 1000` returns 0 (correct) but `limit === undefined ? 1000 : limit` properly distinguishes "not provided" from "explicitly zero". Changed from `??` to explicit check. **Regression tests added (5 tests in main E2E suite):** - DESCRIBE TABLE without LIMIT error - EXISTS TABLE without LIMIT error - limit=0 returns all rows without truncation - INSERT uses `client.command()` not `client.query()` - WITH...INSERT does not get LIMIT appended Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix: address CodeRabbit review findings for ClickHouse driver PR - Remove stale ClickHouse entry from "Unsupported Databases" doc section - Add ClickHouse to Docker auto-discovery description in docs - Add blank line around ClickHouse auth table for markdownlint MD058 - Add `text` language tag to fenced code block for markdownlint MD040 - Fail fast when `binds` passed to ClickHouse `execute()` instead of ignoring - Add `tls_key`, `tls_cert`, `tls_ca_cert` to SENSITIVE_FIELDS in credential store - Clamp `days`/`limit` values in ClickHouse query history SQL builder - Add `clickhouse`, `clickhouse+http`, `clickhouse+https` to DATABASE_URL scheme map - Make `waitForPort` accept configurable host in E2E tests - Close failed connectors during `waitForDbReady` retries in E2E tests - Add missing TLS alias tests: `ca_cert`, `ssl_cert`, `ssl_key` Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> --------- Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
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.
What does this PR do?
Adds first-class ClickHouse support as the 12th database driver in Altimate Code. Full integration across all 23 codebase touchpoints — driver, registry, config normalization, Docker discovery, environment variable detection, dbt profile mapping, dbt lineage dialect, query history (finops), publish peer dependencies, tool descriptions, tests, and documentation.
Key highlights:
@clickhouse/clientover HTTP(S), supporting ClickHouse server 23.3+ (all non-EOL versions)WITH...INSERTCTEs)system.query_logquery history template for finopspackages/drivers/ADDING_A_DRIVER.md(23-point checklist for future drivers)/add-database-driverto automate adding future database driversType of change
Issue for this PR
Closes #573
How did you verify your code works?
Checklist
Summary by CodeRabbit
New Features
Documentation
Tests
Chores