Escape the APPLY column transformer's function name and column-name prefix when formatting - #119851
Conversation
…refix when formatting
`ASTColumnsApplyTransformer::formatImpl` wrote both of its string members as raw bytes:
ostr << func_name;
ostr << ", '" << column_name_prefix << "')";
The parser reads them back as an identifier (`ParserIdentifier`) and as a string literal
(`ParserStringLiteral`), so any byte that is special to those grammars ends the token early or
is re-interpreted on the way in. Formatting is one half of a round-trip contract:
`executeQueryImpl` formats every query and parses it back under `#ifndef NDEBUG`, gated by no
setting, and raises `LOGICAL_ERROR` when the result does not match.
Three plain SELECTs therefore aborted an assertions-enabled build, all measured on a Debug
build of 305623c:
SELECT * APPLY (toString, 'x''y') FROM (SELECT 1 AS a)
-> "cannot parse query back", signal 6
SELECT * APPLY (toString, 'a\\b') FROM (SELECT 1 AS a)
-> the prefix is one backslash, spelled '\\' in SQL; emitted raw it re-parses as a<BS>b,
so "the original AST ... differs from the result of parsing back formatted AST",
signal 6
SELECT * APPLY `to String` FROM (SELECT 1 AS a)
-> "cannot parse query back", signal 6
The same formatter runs with no assertions, so release builds are affected through
`formatQuery` / `formatQuerySingleLine`, which returned SQL that either does not parse or
parses to a different query. The backslash case was not even idempotent: re-formatting mutated
the prefix again. A prefix holding a newline additionally broke `formatQuerySingleLine`'s
single-line contract by emitting a literal newline.
`quoteString` and `backQuoteIfNeed` are the exact inverses of those two parsers, and are what
every sibling in this area already uses: `ASTColumnsExceptTransformer::formatImpl` quotes its
pattern, `Replacement::formatImpl` back-quotes its name, and `ASTColumnsMatcher` quotes its
patterns. `backQuoteIfNeed` leaves an ordinary function name untouched and `quoteString("p_")`
is `'p_'`, so the output is byte-identical for every value that is not already broken and no
existing reference changes.
`appendColumnName` writes the same two members raw a few lines below and is deliberately left
alone: its output is a column name that is never parsed back, so quoting it would rename
user-visible columns for exactly the prefixes containing a quote or a backslash, which work
correctly in release builds today.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Covers the three shapes that broke the format/re-parse round trip (a single quote in the column-name prefix, a backslash in it, and a function name that needs back-quoting) plus a control asserting that an ordinary prefix and function name are formatted exactly as before. Each shape is observed twice: through `formatQuerySingleLine`, which shows the emitted text in every build type, and executed directly, which trips the `#ifndef NDEBUG` round-trip assertion in `executeQueryImpl` on the lanes built with assertions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Internal second-model reviewReviewed before publishing by an independent second-model review of the diff (two runs, 0 findings
Validation: the three shapes measured in both directions on a Debug build, 50/50 randomized runs of |
Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-impl-self-slot-57:20260913-191300 |
|
Workflow [PR], commit [daaf4e0] Summary: ✅
AI ReviewSummaryThis PR fixes Final Verdict✅ No findings in the current revision. LLVM Coverage ReportMeasured on commit daaf4e0.
Changed lines: Changed C/C++ lines covered: 4/4 (100.00%) · Uncovered code |
Build profile diff (arm_release)Comparing ✅ No significant changes. Binary sizes
The official master build is compiled with Object file sizes5 object files changed (+27.60 KiB total), 0 added.
716 more object files are built by the master warmup baseline only (it builds every object-file target, a pull request build only Compile time of recompiled translation units7 translation units recompiled, 8 s compile time in total, 7 of them have a recent master baseline. |
cc10270
Related: #109223
Related: #114205
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fixed AST formatting of the
APPLYcolumn transformer: a column-name prefix containing a single quote or a backslash, or a function name that needs back-quoting, was emitted without escaping, soformatQueryreturned a query that either could not be parsed or parsed back to a different query.Description
Flagged by the automated review on #109223 (comment), which starts routing the prefix through this printer. Pre-existing on master and on 25.8, 26.3, 26.6, 26.8.
ASTColumnsApplyTransformer::formatImplwrote both of its string members raw. The parser reads them back as an identifier and as a string literal, so a byte special to either grammar ends the token early or is re-interpreted. Measured on a Debug build of305623c72de45:SELECT * APPLY (toString, 'x''y') FROM (SELECT 1 AS a)Inconsistent AST formatting: ... cannot parse query back, signal 6SELECT * APPLY (toString, 'a\\b') FROM (SELECT 1 AS a)a<BS>b, so the AST differs, signal 6SELECT * APPLY `to String` FROM (SELECT 1 AS a)... cannot parse query back, signal 6The formatter needs no assertions, so release builds are affected through
formatQuery: it returned SQL that does not parse (rows 1 and 3) or parses to a different query (row 2, where re-formatting mutates the prefix again, soformatQueryis not idempotent).The fix is
backQuoteIfNeed(func_name)andquoteString(column_name_prefix), the exact inverses of those two parsers and what every sibling here uses (EXCEPTandASTColumnsMatcherquote their patterns,Replacementback-quotes its name). Ordinary values are emitted byte-identically and no existing reference changes.The review also asked to escape
appendColumnName. I deliberately did not: its output is a column name, never parsed back, so quoting it would rename user-visible columns for exactly the prefixes with a quote or a backslash, which work correctly today.#114205 rewrites the prefix line of this same function and will conflict textually; the diff is two expressions, so resolving it stays a one-line choice.
Workflow [PR]
Sync PR [sync-upstream/pr/119851]
Version info
26.9.1.1447(included in26.9and later)