Skip to content

Commit af47f65

Browse files
VJ-yadavanandgupta42
authored andcommitted
fix: migrate warehouse-advisor {days} interpolation to parameterized binds
Replaces `{days}` string interpolation in warehouse-advisor SQL templates with `?` positional binds across Snowflake, BigQuery, and Databricks warehouse-load and warehouse-sizing queries. `buildLoadSql` and `buildSizingSql` now return `{ sql, binds }` and the call sites in `adviseWarehouse` pass binds through to `connector.execute()`. Postgres `{limit}` in query-history is kept as a rendered value with `Math.floor(Number(limit))` because the Postgres connector's execute() does not yet forward the `_binds` parameter. Filter placeholder interpolations elsewhere in finops/schema are left as-is per review feedback — tracked separately. - `warehouse-advisor.ts`: 6× `{days}` → `?`, return types updated - `query-history.ts`: comment clarifies why Postgres stays inline - `schema-finops-dbt.test.ts`: updated for new `{ sql, binds }` shape Continues the migration started in #277.
1 parent 0203b6a commit af47f65

File tree

3 files changed

+38
-27
lines changed

3 files changed

+38
-27
lines changed

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,13 @@ function buildHistoryQuery(
158158
}
159159
}
160160
if (whType === "postgres" || whType === "postgresql") {
161-
return { sql: POSTGRES_HISTORY_SQL.replace("{limit}", String(Math.floor(Number(limit)))), binds: [] }
161+
// The Postgres connector (packages/drivers/src/postgres.ts) does not yet
162+
// pass binds through to pg — its execute() ignores the _binds parameter.
163+
// Render LIMIT inline with Math.floor to prevent injection.
164+
return {
165+
sql: POSTGRES_HISTORY_SQL.replace("{limit}", String(Math.floor(Number(limit)))),
166+
binds: [],
167+
}
162168
}
163169
if (whType === "bigquery") {
164170
return { sql: BIGQUERY_HISTORY_SQL, binds: [days, limit] }

packages/opencode/src/altimate/native/finops/warehouse-advisor.ts

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ SELECT
2222
MAX(avg_queued_load) as peak_queue_load,
2323
COUNT(*) as sample_count
2424
FROM SNOWFLAKE.ACCOUNT_USAGE.WAREHOUSE_LOAD_HISTORY
25-
WHERE start_time >= DATEADD('day', -{days}, CURRENT_TIMESTAMP())
25+
WHERE start_time >= DATEADD('day', ?, CURRENT_TIMESTAMP())
2626
GROUP BY warehouse_name
2727
ORDER BY avg_queue_load DESC
2828
`
@@ -36,7 +36,7 @@ SELECT
3636
AVG(bytes_scanned) as avg_bytes_scanned,
3737
SUM(credits_used_cloud_services) as total_credits
3838
FROM SNOWFLAKE.ACCOUNT_USAGE.QUERY_HISTORY
39-
WHERE start_time >= DATEADD('day', -{days}, CURRENT_TIMESTAMP())
39+
WHERE start_time >= DATEADD('day', ?, CURRENT_TIMESTAMP())
4040
AND execution_status = 'SUCCESS'
4141
GROUP BY warehouse_name
4242
ORDER BY total_credits DESC
@@ -59,7 +59,7 @@ SELECT
5959
MAX(period_slot_ms / 1000.0) as peak_queue_load,
6060
COUNT(*) as sample_count
6161
FROM \`region-US.INFORMATION_SCHEMA.JOBS_TIMELINE\`
62-
WHERE period_start >= TIMESTAMP_SUB(CURRENT_TIMESTAMP(), INTERVAL {days} DAY)
62+
WHERE period_start >= TIMESTAMP_SUB(CURRENT_TIMESTAMP(), INTERVAL ? DAY)
6363
GROUP BY reservation_id
6464
ORDER BY avg_concurrency DESC
6565
`
@@ -74,7 +74,7 @@ SELECT
7474
AVG(total_bytes_billed) as avg_bytes_scanned,
7575
SUM(total_bytes_billed) / 1099511627776.0 * 5.0 as total_credits
7676
FROM \`region-US.INFORMATION_SCHEMA.JOBS\`
77-
WHERE creation_time >= TIMESTAMP_SUB(CURRENT_TIMESTAMP(), INTERVAL {days} DAY)
77+
WHERE creation_time >= TIMESTAMP_SUB(CURRENT_TIMESTAMP(), INTERVAL ? DAY)
7878
AND job_type = 'QUERY'
7979
AND state = 'DONE'
8080
GROUP BY reservation_id
@@ -94,7 +94,7 @@ SELECT
9494
MAX(num_queued_queries) as peak_queue_load,
9595
COUNT(*) as sample_count
9696
FROM system.compute.warehouse_events
97-
WHERE event_time >= DATE_SUB(CURRENT_DATE(), {days})
97+
WHERE event_time >= DATE_SUB(CURRENT_DATE(), ?)
9898
GROUP BY warehouse_id
9999
ORDER BY avg_queue_load DESC
100100
`
@@ -109,7 +109,7 @@ SELECT
109109
AVG(read_bytes) as avg_bytes_scanned,
110110
0 as total_credits
111111
FROM system.query.history
112-
WHERE start_time >= DATE_SUB(CURRENT_DATE(), {days})
112+
WHERE start_time >= DATE_SUB(CURRENT_DATE(), ?)
113113
AND status = 'FINISHED'
114114
GROUP BY warehouse_id
115115
ORDER BY query_count DESC
@@ -127,17 +127,17 @@ function getWhType(warehouse: string): string {
127127
return wh?.type || "unknown"
128128
}
129129

130-
function buildLoadSql(whType: string, days: number): string | null {
131-
if (whType === "snowflake") return SNOWFLAKE_LOAD_SQL.replace("{days}", String(days))
132-
if (whType === "bigquery") return BIGQUERY_LOAD_SQL.replace("{days}", String(days))
133-
if (whType === "databricks") return DATABRICKS_LOAD_SQL.replace(/{days}/g, String(days))
130+
function buildLoadSql(whType: string, days: number): { sql: string; binds: any[] } | null {
131+
if (whType === "snowflake") return { sql: SNOWFLAKE_LOAD_SQL, binds: [-days] }
132+
if (whType === "bigquery") return { sql: BIGQUERY_LOAD_SQL, binds: [days] }
133+
if (whType === "databricks") return { sql: DATABRICKS_LOAD_SQL, binds: [days] }
134134
return null
135135
}
136136

137-
function buildSizingSql(whType: string, days: number): string | null {
138-
if (whType === "snowflake") return SNOWFLAKE_SIZING_SQL.replace("{days}", String(days))
139-
if (whType === "bigquery") return BIGQUERY_SIZING_SQL.replace("{days}", String(days))
140-
if (whType === "databricks") return DATABRICKS_SIZING_SQL.replace(/{days}/g, String(days))
137+
function buildSizingSql(whType: string, days: number): { sql: string; binds: any[] } | null {
138+
if (whType === "snowflake") return { sql: SNOWFLAKE_SIZING_SQL, binds: [-days] }
139+
if (whType === "bigquery") return { sql: BIGQUERY_SIZING_SQL, binds: [days] }
140+
if (whType === "databricks") return { sql: DATABRICKS_SIZING_SQL, binds: [days] }
141141
return null
142142
}
143143

@@ -218,10 +218,10 @@ export async function adviseWarehouse(params: WarehouseAdvisorParams): Promise<W
218218
const whType = getWhType(params.warehouse)
219219
const days = params.days ?? 14
220220

221-
const loadSql = buildLoadSql(whType, days)
222-
const sizingSql = buildSizingSql(whType, days)
221+
const loadBuilt = buildLoadSql(whType, days)
222+
const sizingBuilt = buildSizingSql(whType, days)
223223

224-
if (!loadSql || !sizingSql) {
224+
if (!loadBuilt || !sizingBuilt) {
225225
return {
226226
success: false,
227227
warehouse_load: [],
@@ -237,8 +237,8 @@ export async function adviseWarehouse(params: WarehouseAdvisorParams): Promise<W
237237

238238
// Run load and sizing queries in parallel
239239
const [loadResult, sizingResult] = await Promise.all([
240-
connector.execute(loadSql, 1000),
241-
connector.execute(sizingSql, 1000),
240+
connector.execute(loadBuilt.sql, 1000, loadBuilt.binds),
241+
connector.execute(sizingBuilt.sql, 1000, sizingBuilt.binds),
242242
])
243243

244244
// Build warehouse name → size map from SHOW WAREHOUSES (Snowflake only).

packages/opencode/test/altimate/schema-finops-dbt.test.ts

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,9 @@ describe("FinOps: SQL template generation", () => {
141141
test("builds PostgreSQL history SQL", () => {
142142
const built = HistoryTemplates.buildHistoryQuery("postgres", 7, 50)
143143
expect(built?.sql).toContain("pg_stat_statements")
144-
expect(built?.sql).toContain("50") // postgres still uses string interpolation
144+
// Postgres connector does not yet pass binds — LIMIT rendered inline
145+
expect(built?.sql).toContain("LIMIT 50")
146+
expect(built?.binds).toEqual([])
145147
})
146148

147149
test("returns null for DuckDB (no query history)", () => {
@@ -209,18 +211,21 @@ describe("FinOps: SQL template generation", () => {
209211

210212
describe("warehouse-advisor", () => {
211213
test("builds Snowflake load SQL", () => {
212-
const sql = AdvisorTemplates.buildLoadSql("snowflake", 14)
213-
expect(sql).toContain("WAREHOUSE_LOAD_HISTORY")
214+
const built = AdvisorTemplates.buildLoadSql("snowflake", 14)
215+
expect(built?.sql).toContain("WAREHOUSE_LOAD_HISTORY")
216+
expect(built?.binds).toEqual([-14])
214217
})
215218

216219
test("builds Snowflake sizing SQL", () => {
217-
const sql = AdvisorTemplates.buildSizingSql("snowflake", 14)
218-
expect(sql).toContain("PERCENTILE_CONT")
220+
const built = AdvisorTemplates.buildSizingSql("snowflake", 14)
221+
expect(built?.sql).toContain("PERCENTILE_CONT")
222+
expect(built?.binds).toEqual([-14])
219223
})
220224

221225
test("builds BigQuery load SQL", () => {
222-
const sql = AdvisorTemplates.buildLoadSql("bigquery", 14)
223-
expect(sql).toContain("JOBS_TIMELINE")
226+
const built = AdvisorTemplates.buildLoadSql("bigquery", 14)
227+
expect(built?.sql).toContain("JOBS_TIMELINE")
228+
expect(built?.binds).toEqual([14])
224229
})
225230

226231
test("returns null for unsupported types", () => {

0 commit comments

Comments
 (0)