Fix query(): insert FORMAT before a trailing SETTINGS clause - #972
Open
polyglotAI-bot wants to merge 2 commits into
Open
Fix query(): insert FORMAT before a trailing SETTINGS clause#972polyglotAI-bot wants to merge 2 commits into
polyglotAI-bot wants to merge 2 commits into
Conversation
client.query() appended `FORMAT <format>` to the very end of the user's SQL, so a query ending in a top-level `SETTINGS ...` clause was sent as `... SETTINGS ... FORMAT ...`. ClickHouse servers that require SETTINGS to be the trailing clause of a statement (e.g. DESCRIBE on < 24.x) reject that ordering; modern servers accept either order, masking the bug. formatQuery now detects a trailing top-level SETTINGS clause (ignoring string literals, comments, subqueries, and dollar-quoted heredocs) and inserts FORMAT before it, producing `... FORMAT <format> SETTINGS ...`. Fixes: #970
polyglotAI-bot
requested review from
mshustov and
peter-leonov-ch
as code owners
July 29, 2026 13:28
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes client.query() SQL assembly so the client inserts FORMAT <format> before a trailing top-level SETTINGS clause (instead of always appending FORMAT at the very end), restoring compatibility with ClickHouse servers/statement forms that require SETTINGS to be the final clause.
Changes:
- Updated
formatQuery()to detect a trailing top-levelSETTINGSclause and injectFORMATimmediately before it. - Added a unit test suite covering
FORMATplacement across strings/comments/heredocs/subqueries and semicolon stripping. - Added changelog entries to both Node.js and Web client packages for the user-visible fix.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/client-common/src/client.ts | Implements trailing top-level SETTINGS detection and adjusts FORMAT placement; updates query() JSDoc accordingly. |
| packages/client-common/tests/unit/format_query.test.ts | New table-driven unit tests asserting exact wire query strings for FORMAT placement scenarios. |
| packages/client-node/CHANGELOG.md | Documents the bug fix in the Node.js client changelog under # 1.24.0. |
| packages/client-web/CHANGELOG.md | Documents the bug fix in the Web client changelog under # 1.24.0. |
…substring alloc Close the codecov/patch gap on the formatQuery SETTINGS tokenizer by adding 9 targeted unit cases exercising its previously-uncovered branches: backslash- and doubled-quote string escapes, unterminated string, a bare `$` / trailing `$tag` that is not a dollar-quote opener, an unterminated dollar-quoted heredoc, a line comment whose scan resumes after the newline, an unterminated block comment, and an unbalanced closing bracket. Also address the Copilot review note on the scan hot path: make `settingsClauseRe` sticky (`/…/iy`) and probe it via `matchesSettingsClauseAt(query, i)` (sets `lastIndex = i`) instead of `settingsClauseRe.test(query.slice(i))`, so the depth-0 scan no longer allocates a substring per `s`/`S` probe — keeping it O(n). Behavior is unchanged; all 23 format_query cases pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #970.
client.query()builds the wire query viaformatQuery(), which unconditionally appendedFORMAT <format>to the end of the user-supplied SQL. When the query ends with a top-levelSETTINGS ...clause, the client emitted:ClickHouse servers whose parser requires
SETTINGSto be the trailing clause of a statement (e.g.DESCRIBE/DESC format(...)on servers older than 24.x) reject this. Modern servers (24.x+) accept either order, which is why the bug is masked against a current server — but the client is still producing the wrong ordering. The well-formed query is... FORMAT <format> SETTINGS ...(this same class brokeclickhouse-connect, issue #211).formatQuery()now detects a trailing top-levelSETTINGSclause and insertsFORMATright before it; when there is no such clause it appendsFORMATat the end exactly as before (byte-for-byte unchanged on the common path).Changes
packages/client-common/src/client.tsformatQuery(): if the (trimmed, semicolon-stripped) query ends with a trailing top-levelSETTINGSclause, emit... FORMAT <format> SETTINGS ...; otherwise appendFORMATat the end as before.trailingSettingsClauseIndex()scans the query with a small tokenizer that is aware of string literals ('/"/`with backslash and doubled-quote escapes), dollar-quoted heredocs ($$...$$,$tag$...$tag$), line comments (--,#), block comments (/* */), and bracket depth. ASETTINGSkeyword is treated as a clause only when it is at top level (bracket depth 0), outside strings/comments, a whole word, and immediately followed by a<name> = ...settings list — soSETTINGSinside a string/comment/subquery and asettingsidentifier/column are all left untouched.query()JSDoc to describe the new placement.client-commoncode is bundled into both clients, added a## Bug fixesentry under the unreleased# 1.24.0heading in bothpackages/client-node/CHANGELOG.mdandpackages/client-web/CHANGELOG.md.Test
packages/client-common/__tests__/unit/format_query.test.ts(new): a table-driven suite that captures the exact wire stringclient.query()sends (via a stub connection) and assertsFORMATplacement. It covers the fix (basic trailingSETTINGS, the issue'sDESC format(...)reproduction, subquery-with-outer-SETTINGS, block-comment-then-real-clause, trailing semicolon, no-space=) and — as contrast cases that must keep today's append-at-end behavior — noSETTINGS,SETTINGSinside a string literal, asettingsidentifier, a subquery's ownSETTINGS,--/#line comments, and a dollar-quoted heredoc containingSETTINGS-shaped text. The 7 injection cases fail onmainand pass with the fix; the contrast cases pass both ways. Expected orderings were verified against a live ClickHouse server (all HTTP 200).Verified in the devbox: full node unit suite green (419 passed),
typecheck,lint(--max-warnings=0), andprettierall clean. (Web unit tests run in a real browser via Playwright, which isn't available in the build sandbox; the shared code is exercised by the node suite through the identical symlinked source, andclient-webtypechecks.)Pre-PR validation gate
main, passes with the fix)formatQuery, the sole placeFORMATis appended)AGENTS.md(CHANGELOG in both client packages, prettier/lint/typecheck)Notes
SETTINGSclause are unaffected; queries that were failing on older servers now succeed.SETTINGSand its first setting name (e.g.SETTINGS /* c */ x = 1) is not recognized and falls back to the previous append-at-end behavior — no regression, just not newly fixed.