Skip to content

fix: preserve SETTINGS clause in PrepareBatch after column list#1923

Open
polyglotAI-bot wants to merge 2 commits into
mainfrom
polyglot/fix-preparebatch-settings-clause
Open

fix: preserve SETTINGS clause in PrepareBatch after column list#1923
polyglotAI-bot wants to merge 2 commits into
mainfrom
polyglot/fix-preparebatch-settings-clause

Conversation

@polyglotAI-bot

Copy link
Copy Markdown
Collaborator

Description

Fixes #1919.

PrepareBatch normalizes the INSERT query through extractNormalizedInsertQueryAndColumns (batch.go). The normalization regex captures INSERT INTO <table>[(columns)] and stops there, so a SETTINGS ... clause placed after the column list — e.g. INSERT INTO t (a, b) SETTINGS async_insert=1 — was silently dropped from the query sent to the server. In the no-column-list form (INSERT INTO t SETTINGS async_insert=1) the settings text instead leaked into the extracted table name, which breaks the HTTP path's DESCRIBE TABLE <tableName>.

The fix captures a trailing SETTINGS clause and preserves it in the normalized query, stripping it from the query before the table name and columns are extracted so it no longer leaks into either. The clause is recognized only when the SETTINGS keyword is followed by an actual name = value assignment, so a table or column merely named settings is not misparsed as a settings clause.

Changes

  • batch.go: add extractInsertSettingsMatch and capture/re-inject the SETTINGS clause in extractNormalizedInsertQueryAndColumns; build the normalized query as INSERT INTO <table>[(cols)] [SETTINGS ...] FORMAT Native.
  • batch_test.go: extend the table-driven TestExtractNormalizedInsertQueryAndColumns with cases for SETTINGS after the column list (incl. trailing VALUES, multiple settings, no column list, lowercase keyword, explicit FORMAT) plus contrast cases proving a table/column named settings is left untouched.
  • tests/issues/1919_test.go: end-to-end regression test over native and HTTP — a valid inline SETTINGS clause round-trips, and an unknown setting must surface as an error (proving the clause actually reaches the server instead of being dropped).

Test

go test -run TestExtractNormalizedInsertQueryAndColumns . and Test1919 both fail on main and pass on this branch. The unit cases assert the exact normalized query / table name / columns; Test1919 inserts with a valid SETTINGS clause after the column list and then asserts an unknown setting produces an error on both protocols (before the fix the setting was dropped and no error was raised). Existing root-package tests and TestBatch* integration tests still pass with no changes to existing tests.

Pre-PR validation gate

  • Deterministic repro confirmed (unit + integration fail on main, pass here)
  • Root cause documented above
  • Fix targets the root cause (recognize + preserve the SETTINGS clause)
  • Test fails without fix, passes with fix
  • No existing tests broken / weakened
  • Convention compliance verified per AGENTS.md / CONTRIBUTING.md (regression test in tests/issues/NNNN_test.go, t.Cleanup for connections, table-driven cases, gofmt clean)
  • No CHANGELOG.md edit — CONTRIBUTING.md states it is generated automatically during release
  • No public API change

extractNormalizedInsertQueryAndColumns normalized the INSERT query with a
regex that stopped after the optional column list, so a SETTINGS clause
placed after the columns (e.g. "INSERT INTO t (a, b) SETTINGS async_insert=1")
was silently dropped from the query sent to the server. In the no-column-list
form it instead leaked into the extracted table name, breaking the HTTP
DESCRIBE TABLE path.

Capture a trailing SETTINGS clause and preserve it in the normalized query,
stripping it before the table name and columns are extracted. The clause is
recognized only when the SETTINGS keyword is followed by a "name = value"
assignment, so a table or column merely named "settings" is not misparsed.

Fixes: #1919
@github-actions

Copy link
Copy Markdown

🤖 Claude review

Fixes #1919: PrepareBatch silently dropped a SETTINGS clause placed after the column list (and, in the no-column-list form, leaked the settings text into the extracted table name, breaking the HTTP DESCRIBE TABLE). The fix captures a trailing SETTINGS clause, strips it before table/column extraction, and re-injects it into the normalized query. Verdict: approve.

The implementation is correct and the regex is appropriately conservative — the SETTINGS\s+\w+\s*= guard requires a real name = value assignment, so a table or column merely named settings is not misparsed (traced through the (col1, settings) and INSERT INTO settings cases). Ordering is sound: truncateValues requires whitespace after VALUES, so a trailing bare VALUES correctly survives to be handled by the settings regex's optional (?:\s+VALUES)?.

Test coverage is strong:

  • Unit test extends TestExtractNormalizedInsertQueryAndColumns with the column-list, no-column-list, trailing VALUES, explicit FORMAT, multiple-settings, and lowercase cases, plus contrast cases proving a settings-named table/column is untouched.
  • Test1919 is an end-to-end regression over native and HTTP that both round-trips a valid clause and asserts an unknown setting surfaces as an error — proving the clause actually reaches the server rather than being dropped.

Blind spots:

  • Integration test exercises only the native driver.Conn on both protocols, not the database/sql surface. This is low-risk: the std path routes through the same prepareBatchextractNormalizedInsertQueryAndColumns, and that pure function is directly unit-tested.
  • Could not run the integration test against a live ClickHouse; correctness of the round-trip and the unknown-setting error text is taken on the test's assertions.
  • A SETTINGS string value literally containing FORMAT or a trailing VALUES token could be mis-truncated, but this is a pre-existing fragility of the truncateFormat/truncateValues regexes, not introduced here.

Verdict: ✅ Approve

Inline comments are attached to the relevant lines. This summary updates in place on re-review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes a PrepareBatch INSERT normalization bug where an inline SETTINGS ... clause (especially after a column list) could be dropped or misparsed, affecting both native and HTTP batch paths.

Changes:

  • Extend INSERT normalization to detect and preserve a trailing SETTINGS name=value... clause while preventing it from leaking into the extracted table name/columns.
  • Expand unit coverage for INSERT normalization around SETTINGS placement/casing/FORMAT interactions and “settings” identifier contrast cases.
  • Add an end-to-end regression test (native + HTTP) asserting inline SETTINGS reaches the server (unknown setting must error).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
batch.go Captures and re-injects a trailing SETTINGS clause into the normalized INSERT query.
batch_test.go Adds table-driven unit cases covering SETTINGS after column lists (and related contrasts).
tests/issues/1919_test.go Adds an integration regression test for issue #1919 across native and HTTP protocols.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread batch.go Outdated
Comment thread batch_test.go
…atch

A trailing `;` statement terminator on the inline SETTINGS form (e.g.
`INSERT INTO t (a, b) SETTINGS async_insert=1;`) was folded into the
captured settings clause by the lazy `.+?`, producing a malformed
normalized query `... SETTINGS async_insert=1; FORMAT Native` that the
server rejects with a syntax error. Match trailing terminators and
whitespace with `[\s;]*$` outside the capture group so they are dropped,
mirroring how the no-SETTINGS path already strips a trailing `;`. A `;`
inside a quoted setting value is preserved (only end-of-query terminators
are consumed).

Addresses review feedback on #1919.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[backfill: ClickHouse/clickhouse-go] PrepareBatch silently drops SETTINGS clause when placed after column list

3 participants