From af47f65e86927ff40432115b8f432082a22ee227 Mon Sep 17 00:00:00 2001 From: Vijay Yadav Date: Sat, 28 Mar 2026 17:45:03 -0400 Subject: [PATCH] fix: migrate warehouse-advisor {days} interpolation to parameterized binds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../altimate/native/finops/query-history.ts | 8 +++- .../native/finops/warehouse-advisor.ts | 38 +++++++++---------- .../test/altimate/schema-finops-dbt.test.ts | 19 ++++++---- 3 files changed, 38 insertions(+), 27 deletions(-) diff --git a/packages/opencode/src/altimate/native/finops/query-history.ts b/packages/opencode/src/altimate/native/finops/query-history.ts index e2ed0eab31..a0046db514 100644 --- a/packages/opencode/src/altimate/native/finops/query-history.ts +++ b/packages/opencode/src/altimate/native/finops/query-history.ts @@ -158,7 +158,13 @@ function buildHistoryQuery( } } if (whType === "postgres" || whType === "postgresql") { - return { sql: POSTGRES_HISTORY_SQL.replace("{limit}", String(Math.floor(Number(limit)))), binds: [] } + // The Postgres connector (packages/drivers/src/postgres.ts) does not yet + // pass binds through to pg — its execute() ignores the _binds parameter. + // Render LIMIT inline with Math.floor to prevent injection. + return { + sql: POSTGRES_HISTORY_SQL.replace("{limit}", String(Math.floor(Number(limit)))), + binds: [], + } } if (whType === "bigquery") { return { sql: BIGQUERY_HISTORY_SQL, binds: [days, limit] } diff --git a/packages/opencode/src/altimate/native/finops/warehouse-advisor.ts b/packages/opencode/src/altimate/native/finops/warehouse-advisor.ts index dc6c1166a5..e2bf4e84f3 100644 --- a/packages/opencode/src/altimate/native/finops/warehouse-advisor.ts +++ b/packages/opencode/src/altimate/native/finops/warehouse-advisor.ts @@ -22,7 +22,7 @@ SELECT MAX(avg_queued_load) as peak_queue_load, COUNT(*) as sample_count FROM SNOWFLAKE.ACCOUNT_USAGE.WAREHOUSE_LOAD_HISTORY -WHERE start_time >= DATEADD('day', -{days}, CURRENT_TIMESTAMP()) +WHERE start_time >= DATEADD('day', ?, CURRENT_TIMESTAMP()) GROUP BY warehouse_name ORDER BY avg_queue_load DESC ` @@ -36,7 +36,7 @@ SELECT AVG(bytes_scanned) as avg_bytes_scanned, SUM(credits_used_cloud_services) as total_credits FROM SNOWFLAKE.ACCOUNT_USAGE.QUERY_HISTORY -WHERE start_time >= DATEADD('day', -{days}, CURRENT_TIMESTAMP()) +WHERE start_time >= DATEADD('day', ?, CURRENT_TIMESTAMP()) AND execution_status = 'SUCCESS' GROUP BY warehouse_name ORDER BY total_credits DESC @@ -59,7 +59,7 @@ SELECT MAX(period_slot_ms / 1000.0) as peak_queue_load, COUNT(*) as sample_count FROM \`region-US.INFORMATION_SCHEMA.JOBS_TIMELINE\` -WHERE period_start >= TIMESTAMP_SUB(CURRENT_TIMESTAMP(), INTERVAL {days} DAY) +WHERE period_start >= TIMESTAMP_SUB(CURRENT_TIMESTAMP(), INTERVAL ? DAY) GROUP BY reservation_id ORDER BY avg_concurrency DESC ` @@ -74,7 +74,7 @@ SELECT AVG(total_bytes_billed) as avg_bytes_scanned, SUM(total_bytes_billed) / 1099511627776.0 * 5.0 as total_credits FROM \`region-US.INFORMATION_SCHEMA.JOBS\` -WHERE creation_time >= TIMESTAMP_SUB(CURRENT_TIMESTAMP(), INTERVAL {days} DAY) +WHERE creation_time >= TIMESTAMP_SUB(CURRENT_TIMESTAMP(), INTERVAL ? DAY) AND job_type = 'QUERY' AND state = 'DONE' GROUP BY reservation_id @@ -94,7 +94,7 @@ SELECT MAX(num_queued_queries) as peak_queue_load, COUNT(*) as sample_count FROM system.compute.warehouse_events -WHERE event_time >= DATE_SUB(CURRENT_DATE(), {days}) +WHERE event_time >= DATE_SUB(CURRENT_DATE(), ?) GROUP BY warehouse_id ORDER BY avg_queue_load DESC ` @@ -109,7 +109,7 @@ SELECT AVG(read_bytes) as avg_bytes_scanned, 0 as total_credits FROM system.query.history -WHERE start_time >= DATE_SUB(CURRENT_DATE(), {days}) +WHERE start_time >= DATE_SUB(CURRENT_DATE(), ?) AND status = 'FINISHED' GROUP BY warehouse_id ORDER BY query_count DESC @@ -127,17 +127,17 @@ function getWhType(warehouse: string): string { return wh?.type || "unknown" } -function buildLoadSql(whType: string, days: number): string | null { - if (whType === "snowflake") return SNOWFLAKE_LOAD_SQL.replace("{days}", String(days)) - if (whType === "bigquery") return BIGQUERY_LOAD_SQL.replace("{days}", String(days)) - if (whType === "databricks") return DATABRICKS_LOAD_SQL.replace(/{days}/g, String(days)) +function buildLoadSql(whType: string, days: number): { sql: string; binds: any[] } | null { + if (whType === "snowflake") return { sql: SNOWFLAKE_LOAD_SQL, binds: [-days] } + if (whType === "bigquery") return { sql: BIGQUERY_LOAD_SQL, binds: [days] } + if (whType === "databricks") return { sql: DATABRICKS_LOAD_SQL, binds: [days] } return null } -function buildSizingSql(whType: string, days: number): string | null { - if (whType === "snowflake") return SNOWFLAKE_SIZING_SQL.replace("{days}", String(days)) - if (whType === "bigquery") return BIGQUERY_SIZING_SQL.replace("{days}", String(days)) - if (whType === "databricks") return DATABRICKS_SIZING_SQL.replace(/{days}/g, String(days)) +function buildSizingSql(whType: string, days: number): { sql: string; binds: any[] } | null { + if (whType === "snowflake") return { sql: SNOWFLAKE_SIZING_SQL, binds: [-days] } + if (whType === "bigquery") return { sql: BIGQUERY_SIZING_SQL, binds: [days] } + if (whType === "databricks") return { sql: DATABRICKS_SIZING_SQL, binds: [days] } return null } @@ -218,10 +218,10 @@ export async function adviseWarehouse(params: WarehouseAdvisorParams): Promise { test("builds PostgreSQL history SQL", () => { const built = HistoryTemplates.buildHistoryQuery("postgres", 7, 50) expect(built?.sql).toContain("pg_stat_statements") - expect(built?.sql).toContain("50") // postgres still uses string interpolation + // Postgres connector does not yet pass binds — LIMIT rendered inline + expect(built?.sql).toContain("LIMIT 50") + expect(built?.binds).toEqual([]) }) test("returns null for DuckDB (no query history)", () => { @@ -209,18 +211,21 @@ describe("FinOps: SQL template generation", () => { describe("warehouse-advisor", () => { test("builds Snowflake load SQL", () => { - const sql = AdvisorTemplates.buildLoadSql("snowflake", 14) - expect(sql).toContain("WAREHOUSE_LOAD_HISTORY") + const built = AdvisorTemplates.buildLoadSql("snowflake", 14) + expect(built?.sql).toContain("WAREHOUSE_LOAD_HISTORY") + expect(built?.binds).toEqual([-14]) }) test("builds Snowflake sizing SQL", () => { - const sql = AdvisorTemplates.buildSizingSql("snowflake", 14) - expect(sql).toContain("PERCENTILE_CONT") + const built = AdvisorTemplates.buildSizingSql("snowflake", 14) + expect(built?.sql).toContain("PERCENTILE_CONT") + expect(built?.binds).toEqual([-14]) }) test("builds BigQuery load SQL", () => { - const sql = AdvisorTemplates.buildLoadSql("bigquery", 14) - expect(sql).toContain("JOBS_TIMELINE") + const built = AdvisorTemplates.buildLoadSql("bigquery", 14) + expect(built?.sql).toContain("JOBS_TIMELINE") + expect(built?.binds).toEqual([14]) }) test("returns null for unsupported types", () => {