Skip to content

Escape the APPLY column transformer's function name and column-name prefix when formatting - #119851

Merged
PedroTadim merged 2 commits into
ClickHouse:masterfrom
groeneai:fix-apply-transformer-format-escaping
Sep 14, 2026
Merged

Escape the APPLY column transformer's function name and column-name prefix when formatting#119851
PedroTadim merged 2 commits into
ClickHouse:masterfrom
groeneai:fix-apply-transformer-format-escaping

Conversation

@groeneai

@groeneai groeneai commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator

Related: #109223
Related: #114205

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fixed AST formatting of the APPLY column transformer: a column-name prefix containing a single quote or a backslash, or a function name that needs back-quoting, was emitted without escaping, so formatQuery returned 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::formatImpl wrote 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 of 305623c72de45:

query before
SELECT * APPLY (toString, 'x''y') FROM (SELECT 1 AS a) Inconsistent AST formatting: ... cannot parse query back, signal 6
SELECT * APPLY (toString, 'a\\b') FROM (SELECT 1 AS a) the one-backslash prefix is emitted raw and re-parses as a<BS>b, so the AST differs, signal 6
SELECT * APPLY `to String` FROM (SELECT 1 AS a) ... cannot parse query back, signal 6

The 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, so formatQuery is not idempotent).

The fix is backQuoteIfNeed(func_name) and quoteString(column_name_prefix), the exact inverses of those two parsers and what every sibling here uses (EXCEPT and ASTColumnsMatcher quote their patterns, Replacement back-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

  • Merged into: 26.9.1.1447 (included in 26.9 and later)

groeneai and others added 2 commits September 13, 2026 20:42
…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>
@groeneai groeneai added can be tested Allows running workflows for external contributors groeneai-origin-self PR origin: self-found defect (impact-bar validated) labels Sep 13, 2026
@groeneai

Copy link
Copy Markdown
Collaborator Author
Internal second-model review

Reviewed before publishing by an independent second-model review of the diff (two runs, 0 findings
each) and by my own cold pass. Every finding was in my own prose, none in the code or the test: an
under-escaped backslash example ('a\b' is the backspace escape, so the measured input is 'a\\b'),
an overstated idempotence claim, and an imprecise note about the overlap with #114205. All are
corrected here.

⚠️ appendColumnName writes the same two members raw and is deliberately left alone, for the reason
given in the description.

Validation: the three shapes measured in both directions on a Debug build, 50/50 randomized runs of
the new test, and a sweep of all 43 stateless tests that use APPLY with zero reference churn.

@clickhouse-gh clickhouse-gh Bot closed this Sep 13, 2026
@clickhouse-gh clickhouse-gh Bot reopened this Sep 13, 2026
@clickhouse-gh clickhouse-gh Bot reopened this Sep 13, 2026
@groeneai

Copy link
Copy Markdown
Collaborator Author
Pre-PR validation gate (click to expand)
# Question Answer
a Deterministic repro? Yes, three fixed queries, no randomization or timing. On a Debug build of 305623c72de45 (Build ID c2ffd92d), SELECT * APPLY (toString, 'x''y') FROM (SELECT 1 AS a), the same with a literal backslash in the prefix, and SELECT * APPLY `to String` FROM (SELECT 1 AS a) each exit 134 on every run.
b Root cause explained? ASTColumnsApplyTransformer::formatImpl emitted func_name and column_name_prefix as raw bytes, but the parser reads them back with ParserIdentifier and ParserStringLiteral, so the emitter must be their inverse. A ' or a needs-quoting identifier breaks the re-parse; a \ is re-read as an escape introducer and yields a different value. executeQueryImpl formats and re-parses every query under #ifndef NDEBUG, gated by no setting, so each becomes a LOGICAL_ERROR; with no assertions the same formatter backs formatQuery.
c Fix matches root cause? Yes. The defect is in the emitter and the emitter is fixed, with the two helpers that are the exact inverses of those parsers. No bound widened, no tag added, no downstream guard, no data reduction.
d Test intent preserved / new tests added? New 05211_apply_transformer_format_escaping. Six statements: the three shapes plus a control, each shape seen twice (through formatQuerySingleLine, visible in every build type, and executed, which trips the round-trip assertion). No existing test weakened; no existing reference changed.
e Both directions demonstrated? Yes, Build-ID verified. Base c2ffd92d: exit 134 / 134 / 134. Fix d02018cb: exit 0 / 0 / 46 (UNKNOWN_FUNCTION). Test: 50 randomized runs, 50 passed.
f Fix is general across code paths? Both string members of the function are fixed, not just the reported one: func_name aborts on a plain SELECT too and is the structural sibling of the reported defect, so it ships here. appendColumnName is deliberately unchanged: its output is a column name that is never parsed back, and quoting it would rename user-visible columns for exactly the prefixes containing a quote or a backslash. Column names measured byte-identical before and after. The sibling transformers (EXCEPT, REPLACE, ASTColumnsMatcher) already escape.
g Fix generalizes across inputs (params/datatypes/wrappers)? No column types are involved, so there is no Nullable / LowCardinality / Array / Const matrix; the invariant is textual. The real input dimension is which bytes are special to the two grammars, and all are covered: newline now \n (it was a literal newline, breaking the single-line contract), tab \t, a lone quote '\'', a`b as `a\`b`. Also correct with parameters present (APPLY (quantile(0.5), 'q')) and on the lambda branch. Formatting is now idempotent for all three shapes; before, the backslash case was not: the first re-format mutated the prefix, and only the pass after that was stable, at the wrong value.
h Backward compatible? (maintainer-approved exception only) Yes. No setting, no default, no format version, so no SettingsChangesHistory.cpp entry. Ordinary prefixes and function names are emitted byte-identically, confirmed by the control arm and by zero reference churn over the 43 stateless tests using APPLY. A mixed-version cluster is strictly better off: an older shard receives text it can parse instead of text it cannot.
i Invariants and contracts preserved? The invariant is restored, not weakened: every string member this formatImpl emits is written so the formatted text parses back to an equal AST. It holds on all paths, including the lambda branch and the empty-prefix path where the changed line is not reached. No lock, allocation, block-structure or concurrency contract is involved.

Session id: cron:clickhouse-impl-self-slot-57:20260913-191300

@clickhouse-gh

clickhouse-gh Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [daaf4e0]

Summary:


AI Review

Summary

This PR fixes ASTColumnsApplyTransformer::formatImpl so APPLY function names and column-name prefixes are formatted with escaping that round-trips through the existing parser. I reviewed the current diff and touched parser file, checked the prior PR discussion, and looked at the current CI state; I did not find a remaining correctness, compatibility, or test-coverage issue that warrants an inline review comment.

Final Verdict

✅ No findings in the current revision.

LLVM Coverage Report

Measured on commit daaf4e0.

Metric Baseline Current Δ
Lines 89.00% 89.00% +0.00%
Functions 91.80% 91.90% +0.10%
Branches 81.40% 81.40% +0.00%

Changed lines: Changed C/C++ lines covered: 4/4 (100.00%) · Uncovered code

Full report · Diff report

@clickhouse-gh clickhouse-gh Bot added pr-bugfix Pull request with bugfix, not backported by default comp-sql-syntax SQL/grammar parsing, AST nodes, syntax-level features. labels Sep 13, 2026
@clickhouse-gh

clickhouse-gh Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Build profile diff (arm_release)

Comparing daaf4e0f7 with master 4e8f2df43 (stripped binary size, per-symbol sizes and ThinLTO time; compile times per translation unit against the most recent warmup build that recompiled it).

✅ No significant changes.

Binary sizes

programs/clickhouse-stripped: smaller than the master baseline by the known offset between the two builds, so the difference is not shown. A delta that differs from the offset by more than 50% of it is shown, in either direction.

The official master build is compiled with -g and a pull request build is not, and XRay counts debug instructions towards its instrumentation threshold, so master instruments thousands of functions more and its binary is ~0.4% larger no matter what the pull request does.

Object file sizes

5 object files changed (+27.60 KiB total), 0 added.

Object file Master PR Δ
src/CMakeFiles/dbms.dir/Analyzer/ValidationUtils.cpp.o 157.19 KiB 184.33 KiB +27.14 KiB (+17.27%)

716 more object files are built by the master warmup baseline only (it builds every object-file target, a pull request build only clickhouse-bundle) and not compared.

Compile time of recompiled translation units

7 translation units recompiled, 8 s compile time in total, 7 of them have a recent master baseline.

Job report

@PedroTadim PedroTadim self-assigned this Sep 14, 2026
@PedroTadim
PedroTadim added this pull request to the merge queue Sep 14, 2026
Merged via the queue into ClickHouse:master with commit cc10270 Sep 14, 2026
176 of 348 checks passed
@robot-clickhouse robot-clickhouse added the pr-synced-to-cloud The PR is synced to the cloud repo label Sep 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

can be tested Allows running workflows for external contributors comp-sql-syntax SQL/grammar parsing, AST nodes, syntax-level features. groeneai-origin-self PR origin: self-found defect (impact-bar validated) pr-bugfix Pull request with bugfix, not backported by default pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants