Fix PostgreSQL visual query follow-ups

[DhruvShah-Dev]

Summary

• route generateTableList through the same PostgreSQL table reference formatter used by FROM generation
• format HAVING column references with the existing generated-column formatter
• quote PostgreSQL SELECT aliases when they need quoting
• apply PostgreSQL identifier formatting for both postgres and postgresql driver ids

Testing

• pnpm.cmd exec vitest run tests/utils/visualQuery.test.ts --pool=threads --maxWorkers=1
• pnpm.cmd exec vitest run tests/utils/visualQuery.test.ts tests/utils/identifiers.test.ts --pool=threads --maxWorkers=1
• pnpm.cmd typecheck
• pnpm.cmd lint

[kilo-code-bot]

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (4 files)

• src/utils/identifiers.ts
• src/utils/visualQuery.ts
• tests/utils/identifiers.test.ts
• tests/utils/visualQuery.test.ts

Previous Review Summary (commit e0c4eaf)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit e0c4eaf)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (4 files)

• src/utils/identifiers.ts
• src/utils/visualQuery.ts
• tests/utils/identifiers.test.ts
• tests/utils/visualQuery.test.ts

---

_{Reviewed by glm-5.2 · Input: 70K · Output: 10.3K · Cached: 160.5K}

[debba]

Hi @DhruvShah-Dev can you fix merge conflicts?

[DhruvShah-Dev]

Resolved the merge conflicts with current main and pushed commit 53a60ac. Verified with pnpm.cmd exec vitest run tests/utils/visualQuery.test.ts (66 passed).

[debba]

Hey @DhruvShah-Dev, thanks for working on these PostgreSQL follow-ups. I tested the PR locally and found a blocker around HAVING generation.

generateHavingClause now sends every aggregate condition through formatGeneratedColumnRef. This breaks aggregate expressions that are already supported by the existing tests:

• COUNT(*) becomes "COUNT(*)"
• SUM(t1.amount) becomes SUM(t1."amount)"

PostgreSQL rejects both forms.

There is also a new test that expects this query fragment:

SELECT COUNT(t1."AccountId") AS "Total Count"
FROM "AccountEventLog" t1
HAVING t1."AccountId" > 0

I ran it against PostgreSQL 16, and it fails because t1.AccountId is neither grouped nor used inside an aggregate function. It should probably generate something like HAVING COUNT(t1."AccountId") > 0, depending on how we want the condition model to work.

Could you update the HAVING formatting so it preserves aggregate expressions and add PostgreSQL test cases for COUNT(*) and SUM(t1.amount)?

Everything else looks good. I ran the full frontend suite with 3,715 passing tests, plus typecheck and lint. CI is also green.

[aesslinger]

Thanks for this, @DhruvShah-Dev — the HAVING-quoting and SELECT-alias-quoting fixes here are catching real, separate bugs (mixed-case/reserved-word columns and aliases render unquoted today, even for the plain builtin postgres driver). Nice find.

Heads up on one overlap to be aware of before this merges: postgres-plugin-migration (fixing #614) also touches shouldQuoteIdentifiers in src/utils/identifiers.ts, and the two changes will conflict.

• This PR's fix: driverStr === "postgres" || driverStr === "postgresql" — a hardcoded id check.
• Hardcoded driver === "postgres" checks in core break the standalone PostgreSQL plugin #614's fix: (capabilities.sql_dialect ?? "postgres") === "postgres" — checks the resolved driver's declared SQL dialect instead of its id string.

The difference matters beyond just merge mechanics: the hardcoded-id version only works because the current PostgreSQL plugin happens to be registered as "postgresql". Any other postgres-compatible driver registered under a different id (a forked plugin, a differently-named build, etc.) would hit the exact same silent-unquoting bug this PR is fixing, just under a new id string. The sql_dialect-based check in #614 covers any driver that declares itself postgres-compatible, regardless of its id.

Confirmed via a throwaway test-merge that the actual conflict is small and contained to shouldQuoteIdentifiers's body/signature — the HAVING/alias-quoting functions this PR adds (formatHavingColumnRef, formatAggregateArgument, formatAlias) are net-new and don't overlap with anything in #614; they'll just need their driver params widened to match whichever DriverArg type wins out, so they inherit the capability-driven check for free.

@debba — flagging for you to decide how to resolve (merge order, which version of shouldQuoteIdentifiers to keep, etc.) since it touches both PRs.

[debba]

Thanks for the detailed analysis @aesslinger, and for running the test-merge to size up the conflict.

I'd go ahead and merge #588 first. The HAVING and alias quoting functions it adds are net-new and fix real bugs users can hit today, so I'd rather preserve that additive work and land it now instead of holding it behind the bigger migration PR.

@aesslinger since you already scoped the overlap, can you take care of resolving the conflict on #577 after this lands? The sql_dialect-based check is the right long-term version of shouldQuoteIdentifiers, so when you rebase, keep the capability-driven logic and just make sure the string fallback also covers "postgresql" so the fix from this PR isn't lost. Widening the driver params of the new helpers can happen in the same pass.

@DhruvShah-Dev thanks again for the fix, merging shortly.