Skip to content

Commit 4e2a073

Browse files
anandgupta42claude
andcommitted
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) <noreply@anthropic.com>
1 parent 1aa0ece commit 4e2a073

10 files changed

Lines changed: 55 additions & 14 deletions

File tree

.claude/commands/add-database-driver.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -130,12 +130,12 @@ Work through all 9 phases from the checklist. Use parallel edits where possible.
130130

131131
```bash
132132
# Tests (from packages/opencode/)
133-
cd packages/opencode && bun test test/altimate/driver-normalize.test.ts test/altimate/connections.test.ts
133+
cd packages/opencode && bun test test/altimate/driver-normalize.test.ts test/altimate/connections.test.ts test/altimate/drivers-{database}-e2e.test.ts
134134

135-
# Typecheck
136-
bun turbo typecheck
135+
# Typecheck (from repo root)
136+
cd "$(git rev-parse --show-toplevel)" && bun turbo typecheck
137137

138-
# Marker check
138+
# Marker check (from repo root)
139139
bun run script/upstream/analyze.ts --markers --base main --strict
140140
```
141141

docs/docs/configure/warehouses.md

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -380,7 +380,6 @@ The following databases are not yet natively supported, but workarounds are avai
380380

381381
| Database | Workaround |
382382
|----------|------------|
383-
| ClickHouse | Use the bash tool with `clickhouse-client` or `curl` to query directly |
384383
| Cassandra | Use the bash tool with `cqlsh` to query directly |
385384
| CockroachDB | PostgreSQL-compatible — use `type: postgres` |
386385
| TimescaleDB | PostgreSQL extension — use `type: postgres` |
@@ -422,7 +421,7 @@ The `/discover` command can automatically detect warehouse connections from:
422421
| Source | Detection |
423422
|--------|-----------|
424423
| dbt profiles | Parses `~/.dbt/profiles.yml` |
425-
| Docker containers | Finds running PostgreSQL, MySQL, and SQL Server containers |
424+
| Docker containers | Finds running PostgreSQL, MySQL, SQL Server, and ClickHouse containers |
426425
| Environment variables | Scans for `SNOWFLAKE_ACCOUNT`, `PGHOST`, `DATABRICKS_HOST`, etc. |
427426

428427
See [Warehouse Tools](../data-engineering/tools/warehouse-tools.md) for the full list of environment variable signals.

docs/docs/drivers.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,7 @@ altimate-dbt init --project-root /path/to/dbt/project --python-path $(which pyth
136136
| Password | `host`, `port`, `service_name`, `user`, `password` |
137137

138138
### ClickHouse
139+
139140
| Method | Config Fields |
140141
|--------|--------------|
141142
| Password | `host`, `port`, `database`, `user`, `password` |

packages/drivers/ADDING_A_DRIVER.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ bun run script/upstream/analyze.ts --markers --base main --strict
178178

179179
## File Map
180180

181-
```
181+
```text
182182
packages/drivers/
183183
src/
184184
{database}.ts ← NEW: driver implementation

packages/drivers/src/clickhouse.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,10 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
6060
client = createClient(clientConfig)
6161
},
6262

63-
async execute(sql: string, limit?: number, _binds?: any[]): Promise<ConnectorResult> {
63+
async execute(sql: string, limit?: number, binds?: any[]): Promise<ConnectorResult> {
64+
if (binds && binds.length > 0) {
65+
throw new Error("ClickHouse driver does not support parameterized binds — use ClickHouse query parameters instead")
66+
}
6467
const effectiveLimit = limit === undefined ? 1000 : limit
6568
let query = sql
6669
// Only SELECT and WITH...SELECT support LIMIT — SHOW/DESCRIBE/EXPLAIN/EXISTS do not

packages/opencode/src/altimate/native/connections/credential-store.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,9 @@ const SENSITIVE_FIELDS = new Set([
3131
"ssl_key",
3232
"ssl_cert",
3333
"ssl_ca",
34+
"tls_key",
35+
"tls_cert",
36+
"tls_ca_cert",
3437
])
3538

3639
/** Cached keytar module (or null if unavailable). */

packages/opencode/src/altimate/native/finops/query-history.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -167,9 +167,11 @@ function buildHistoryQuery(
167167
return { sql: DATABRICKS_HISTORY_SQL, binds: [days, limit] }
168168
}
169169
if (whType === "clickhouse") {
170-
const sql = CLICKHOUSE_HISTORY_SQL.replace("{days:UInt32}", String(Math.floor(Number(days)))).replace(
170+
const clampedDays = Math.max(1, Math.min(Math.floor(Number(days)) || 30, 365))
171+
const clampedLimit = Math.max(1, Math.min(Math.floor(Number(limit)) || 100, 10000))
172+
const sql = CLICKHOUSE_HISTORY_SQL.replace("{days:UInt32}", String(clampedDays)).replace(
171173
"{limit:UInt32}",
172-
String(Math.floor(Number(limit))),
174+
String(clampedLimit),
173175
)
174176
return { sql, binds: [] }
175177
}

packages/opencode/src/altimate/tools/project-scan.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,9 @@ export async function detectEnvVars(): Promise<EnvVarConnection[]> {
293293
oracle: "oracle",
294294
duckdb: "duckdb",
295295
databricks: "databricks",
296+
clickhouse: "clickhouse",
297+
"clickhouse+http": "clickhouse",
298+
"clickhouse+https": "clickhouse",
296299
}
297300
const dbType = schemeTypeMap[scheme] ?? "postgres"
298301
// Only add if we don't already have this type detected from other env vars

packages/opencode/test/altimate/driver-normalize.test.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -902,6 +902,24 @@ describe("normalizeConfig — ClickHouse", () => {
902902
expect(result.ssl_ca).toBeUndefined()
903903
})
904904

905+
test("ca_cert → tls_ca_cert", () => {
906+
const result = normalizeConfig({
907+
type: "clickhouse",
908+
ca_cert: "/path/to/ca.pem",
909+
})
910+
expect(result.tls_ca_cert).toBe("/path/to/ca.pem")
911+
expect(result.ca_cert).toBeUndefined()
912+
})
913+
914+
test("ssl_cert → tls_cert", () => {
915+
const result = normalizeConfig({
916+
type: "clickhouse",
917+
ssl_cert: "/path/to/cert.pem",
918+
})
919+
expect(result.tls_cert).toBe("/path/to/cert.pem")
920+
expect(result.ssl_cert).toBeUndefined()
921+
})
922+
905923
test("tlsCert → tls_cert", () => {
906924
const result = normalizeConfig({
907925
type: "clickhouse",
@@ -919,4 +937,13 @@ describe("normalizeConfig — ClickHouse", () => {
919937
expect(result.tls_key).toBe("/path/to/key.pem")
920938
expect(result.tlsKey).toBeUndefined()
921939
})
940+
941+
test("ssl_key → tls_key", () => {
942+
const result = normalizeConfig({
943+
type: "clickhouse",
944+
ssl_key: "/path/to/key.pem",
945+
})
946+
expect(result.tls_key).toBe("/path/to/key.pem")
947+
expect(result.ssl_key).toBeUndefined()
948+
})
922949
})

packages/opencode/test/altimate/drivers-clickhouse-e2e.test.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,11 @@ function isDockerAvailable(): boolean {
2222
}
2323
}
2424

25-
function waitForPort(port: number, timeout = 30000): Promise<void> {
25+
function waitForPort(port: number, timeout = 30000, host = "127.0.0.1"): Promise<void> {
2626
return new Promise((resolve, reject) => {
2727
const start = Date.now()
2828
const attempt = () => {
29-
const sock = createConnection({ host: "127.0.0.1", port })
29+
const sock = createConnection({ host, port })
3030
sock.once("connect", () => {
3131
sock.destroy()
3232
resolve()
@@ -55,13 +55,16 @@ async function waitForDbReady(
5555
const start = Date.now()
5656
let lastErr: any
5757
while (Date.now() - start < timeout) {
58+
let connector: any
5859
try {
59-
const { connector, testQuery } = await connectFn()
60+
const result = await connectFn()
61+
connector = result.connector
6062
await connector.connect()
61-
await connector.execute(testQuery)
63+
await connector.execute(result.testQuery)
6264
return connector
6365
} catch (e: any) {
6466
lastErr = e
67+
try { connector?.disconnect?.() } catch {}
6568
await new Promise((r) => setTimeout(r, 2000))
6669
}
6770
}

0 commit comments

Comments
 (0)