Skip to content

fix: complete migration from string interpolation to parameterized query binds#432

Open
VJ-yadav wants to merge 2 commits intoAltimateAI:mainfrom
VJ-yadav:fix/complete-parameterized-binds-migration
Open

fix: complete migration from string interpolation to parameterized query binds#432
VJ-yadav wants to merge 2 commits intoAltimateAI:mainfrom
VJ-yadav:fix/complete-parameterized-binds-migration

Conversation

@VJ-yadav
Copy link

@VJ-yadav VJ-yadav commented Mar 24, 2026

Summary

Completes the migration started in #277. All remaining .replace("{placeholder}", ...) patterns are now replaced with parameterized ? placeholders and binds arrays across finops and schema modules.

Files changed:

  • credit-analyzer.ts — removed {warehouse_filter} placeholder
  • query-history.ts — removed {user_filter}, {warehouse_filter}, {limit} placeholders
  • role-access.ts — removed {role_filter}, {object_filter}, {grantee_filter}, {user_filter} placeholders
  • tags.ts — removed {tag_filter} placeholder
  • warehouse-advisor.ts — replaced 6x {days} string interpolation with ? binds, updated return type to { sql, binds }

Test Plan

  • bun test test/altimate/schema-finops-dbt.test.ts — 56 pass, 0 fail
  • turbo typecheck — all 5 packages pass
  • grep -r '.replace("{' packages/opencode/src/altimate/native/ — zero matches remaining

Checklist

  • Tests added/updated
  • Documentation updated (if needed)
  • CHANGELOG updated (if user-facing)

Fixes #290

Summary by CodeRabbit

  • Refactor
    • Improved SQL query construction for financial operations and schema management by refactoring from template placeholder replacement to direct SQL concatenation and parameterized bind values, enhancing code maintainability and consistency across query builders for Snowflake, BigQuery, PostgreSQL, and Databricks.

…ery binds

Replace all remaining .replace("{placeholder}", ...) patterns with
parameterized ? placeholders and binds arrays across finops and schema
modules. Also migrates warehouse-advisor.ts which was missed in AltimateAI#277.

Updates tests to verify binds are passed correctly.

Fixes AltimateAI#290
Copy link

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review.

Tip: disable this comment in your organization's Code Review settings.

@github-actions
Copy link

This PR doesn't fully meet our contributing guidelines and PR template.

What needs to be fixed:

  • PR description is missing required template sections. Please use the PR template.

Please edit this PR description to address the above within 2 hours, or it will be automatically closed.

If you believe this was flagged incorrectly, please let a maintainer know.

@coderabbitai
Copy link

coderabbitai bot commented Mar 24, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 6e314ea4-47e2-46b6-9600-d534be86ccd5

📥 Commits

Reviewing files that changed from the base of the PR and between a9760f5 and ca0d93c.

📒 Files selected for processing (2)
  • packages/opencode/src/altimate/native/finops/query-history.ts
  • packages/opencode/test/altimate/schema-finops-dbt.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/opencode/test/altimate/schema-finops-dbt.test.ts
  • packages/opencode/src/altimate/native/finops/query-history.ts

📝 Walkthrough

Walkthrough

SQL construction across FinOps and schema modules was migrated from placeholder replacement and inline interpolation to explicit clause concatenation and parameterized binds. Several builders now return { sql, binds }; callers were updated to pass binds to connector.execute(). Tests were adjusted to assert bind arrays and updated SQL shapes.

Changes

Cohort / File(s) Summary
Credit & Query History
packages/opencode/src/altimate/native/finops/credit-analyzer.ts, packages/opencode/src/altimate/native/finops/query-history.ts
Removed {warehouse_filter}/{user_filter} placeholders from templates. Builders now append filter clauses and always attach GROUP BY/ORDER BY/LIMIT fragments at runtime; Postgres history now renders LIMIT inline and returns binds: [] for that path.
Role Access / User Roles
packages/opencode/src/altimate/native/finops/role-access.ts
Eliminated placeholder .replace() usage ({role_filter}, {object_filter}, {grantee_filter}, {user_filter}); SQL strings are assembled by concatenating filters plus ORDER BY and LIMIT ?, preserving existing execute call patterns and binds ordering.
Warehouse Advisor (signature change)
packages/opencode/src/altimate/native/finops/warehouse-advisor.ts
Replaced {days} interpolation with ? placeholders. buildLoadSql/buildSizingSql signatures now return `{ sql: string; binds: any[] }
Tag References
packages/opencode/src/altimate/native/schema/tags.ts
Removed {tag_filter} placeholder from Snowflake template; getTags now concatenates tagFilter plus ORDER BY tag_name, object_name and LIMIT ? at runtime.
Tests
packages/opencode/test/altimate/schema-finops-dbt.test.ts
Assertions updated to validate { sql, binds } outputs and specific bind arrays (e.g., [], [-14], [14]), replacing checks that relied on interpolated numeric strings in SQL.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested labels

contributor

Poem

🐰
From holes of placeholders I hopped and dug,
Replaced each fragile string with a sturdy hug.
Binds in a row, queries tidy and neat,
Safer, cleaner SQL — a hop I repeat! 🥕

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning Changes address the core requirement [#290] to replace escapeSqlString and .replace() patterns with parameterized binds across credit-analyzer, query-history, role-access, warehouse-advisor, and tags; however, schema-sync.ts and sql-escape.ts deletion were not addressed. Address the remaining file (schema-sync.ts) and delete packages/drivers/src/sql-escape.ts as required by acceptance criteria in issue #290.
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and concisely summarizes the main change: completing the migration from string interpolation to parameterized query binds across multiple files.
Description check ✅ Passed The description follows the template with Summary, Test Plan, and Checklist sections; provides clear file-by-file changes and test verification, though CHANGELOG was not updated.
Out of Scope Changes check ✅ Passed All changes are directly aligned with the migration objective: removing placeholder-based replacements and adding parameterized binds to SQL construction in finops and schema modules.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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/src/altimate/native/finops/query-history.ts`:
- Line 58: The Postgres path is receiving a SQL string with a literal "LIMIT ?"
because getQueryHistory() is appending POSTGRES_HISTORY_SQL plus [limit] while
the Postgres driver (packages/drivers/src/postgres.ts) ignores _binds; fix by
either (A) rendering the numeric limit into POSTGRES_HISTORY_SQL in
getQueryHistory() so the SQL contains "LIMIT <number>" (keep
POSTGRES_HISTORY_SQL updated) or (B) implement bind support in the Postgres
driver (handle _binds in the code in packages/drivers/src/postgres.ts so it
replaces parameter placeholders with bound values) — pick one approach and apply
it consistently so getQueryHistory(), POSTGRES_HISTORY_SQL and the Postgres
driver agree on binds vs rendered values.
🪄 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: 8d51b164-0724-4938-88e9-1a07fcf5fb4d

📥 Commits

Reviewing files that changed from the base of the PR and between 544903f and a9760f5.

📒 Files selected for processing (6)
  • packages/opencode/src/altimate/native/finops/credit-analyzer.ts
  • packages/opencode/src/altimate/native/finops/query-history.ts
  • packages/opencode/src/altimate/native/finops/role-access.ts
  • packages/opencode/src/altimate/native/finops/warehouse-advisor.ts
  • packages/opencode/src/altimate/native/schema/tags.ts
  • packages/opencode/test/altimate/schema-finops-dbt.test.ts

…pport

The Postgres driver ignores the binds parameter, so LIMIT ? would be
sent as literal text and fail. Render LIMIT inline for Postgres while
other warehouses use parameterized binds.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: complete migration from escapeSqlString to parameterized query binds

1 participant