Skip to content

fix: respect backticks when splitting column names in PrepareBatch#1829

Open
Laotree wants to merge 4 commits intoClickHouse:mainfrom
Laotree:fix/backtick-column-name-parsing
Open

fix: respect backticks when splitting column names in PrepareBatch#1829
Laotree wants to merge 4 commits intoClickHouse:mainfrom
Laotree:fix/backtick-column-name-parsing

Conversation

@Laotree
Copy link
Copy Markdown
Contributor

@Laotree Laotree commented Apr 14, 2026

Fixes issue #1827 where PrepareBatch would incorrectly split column names on comma without respecting backticks or double quotes. For example:

INSERT INTO my_table (col1, `my_weird,col2`, col3)

Would incorrectly be parsed as ['col1', 'my_weird', 'col2', 'col3'] instead of ['col1', 'my_weird,col2', 'col3'].

This adds a splitColumnsRespectingQuotes function that properly handles commas inside backtick-quoted and double-quoted column names.

Summary

Checklist

Delete items not relevant to your PR:

  • Unit and integration tests covering the common scenarios were added

Fixes issue where PrepareBatch would incorrectly split column names on
comma without respecting backticks or double quotes. For example:

    INSERT INTO my_table (col1, `my_weird,col2`, col3)

Would incorrectly be parsed as ['col1', 'my_weird', 'col2', 'col3']
instead of ['col1', 'my_weird,col2', 'col3'].

This adds a splitColumnsRespectingQuotes function that properly handles
commas inside backtick-quoted and double-quoted column names.
@github-actions
Copy link
Copy Markdown

Summary

This PR fixes a real parsing bug (#1827) in extractNormalizedInsertQueryAndColumns: the naive strings.Split(columns, ",") broke whenever a column name contained a comma inside backtick or double-quote delimiters (e.g. `my_weird,col2`). The replacement, splitColumnsRespectingQuotes, is a straightforward character-scan that toggles quote-mode on backtick and double-quote boundaries and only splits on commas outside any quoting context. The logic is correct and the approach is idiomatic Go.

Should fix

Missing regression test in tests/issues/.
Per the project's workflow rules, bug fixes require a regression test at tests/issues/issue_1827_test.go. The unit tests added to batch_test.go are a good sanity check for the parsing helper, but they don't exercise PrepareBatch end-to-end against a real ClickHouse instance with a quoted column name containing a comma. Without an integration test, a future refactor of the batch pipeline could silently regress this. A minimal test that creates a table with such a column name, calls PrepareBatch, Appends a row, and Sends would be sufficient.

Nits

  • nit: splitColumnsRespectingQuotes doesn't handle escaped/doubled quote characters inside a quoted identifier (e.g. `mycol `` → column literally named `` mycol ``). ClickHouse allows this per the identifier syntax docs. The old strings.Split path didn't handle it either, so this isn't a regression, but it's worth a `// Note: escaped quotes (doubled backtick/double-quote) are not supported` comment on the function so the next reader doesn't wonder.
  • nit: The old loop had a comment linking to the ClickHouse identifier syntax docs (// refers to https://clickhouse.com/docs/en/sql-reference/syntax#identifiers). That comment was dropped when the loop was rewritten. Worth keeping it near the strings.ReplaceAll / strings.Trim call so the stripping logic remains self-explanatory.

Verdict

Approve — the fix is correct, the helper function is clean, and the unit tests cover both quoting styles. The missing tests/issues/issue_1827_test.go is the only meaningful gap; please add it before merging to stay consistent with the project's regression test convention.

@Laotree
Copy link
Copy Markdown
Contributor Author

Laotree commented Apr 15, 2026

The unit test cases have already been added to batch_test.go. Writing separate test cases in issue_1827_test.go would be redundant.

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.

2 participants