Pre-PR validation gate (per TASK.md Phase 4 Step 7):

a) Deterministic repro? Yes. The minimal repro is SELECT {FUNC:Identifier}(-1) with --param_FUNC=abs. Pre-fix: server aborts with Logical error: 'Inconsistent AST formatting: the original AST: ... (STID: 1941-1bfa)' (signal 6) every time. Post-fix: returns Code: 62. ... Function name cannot be empty or a query parameter substitution. (SYNTAX_ERROR).

b) Root cause explained? Yes. ParserCompoundIdentifier with allow_query_parameter=true accepts {p:Identifier}.tbl.fn as a 3-part identifier with one parameter hole. Constructed via ASTIdentifier(std::vector<String> && name_parts_, bool special, ASTs && name_params) it stores the parameter into children but does NOT call resetFullName() (gated by name_params.empty()), so full_name is "". ASTIdentifier::name() returns this empty full_name. getFunctionLayer then builds a FunctionLayer(function_name=""), which emits an ASTFunction(name=""). ASTFunction::formatImplWithoutAlias at the if (!name.empty()) ostr << backQuoteIfNeed(name); site (the comment there: "Empty names are used rarely, to format queries with an extra pair of parentheses for external databases") writes only (args). The re-parser interprets (args) as a parenthesized expression via RoundBracketsLayer, producing a different AST. The tree-hash mismatch fires the STID 1941-1bfa exception in executeQueryImpl.

c) Fix matches root cause? Yes. The fix rejects the empty function_name directly in getFunctionLayer — at the exact site that decides whether to enter FunctionLayer mode. No special-casing in the formatter, no extra fields in ASTFunction, no parser rewrites elsewhere. 13 lines including comment.

d) Test intent preserved? No existing tests are modified. 01550_query_identifier_parameters (parameterized column / table / database names) and 02014_query_parameters (general {p:Type} substitution) still pass against the fix; legitimate parameter positions are not affected. A new regression test 04277_stid_1941_1bfa_param_subst_function_name.sh is added that asserts both the rejection (SYNTAX_ERROR) and that the legitimate uses still work.

e) Both directions demonstrated?

• WITHOUT the fix: built locally; running tests/clickhouse-test 04277 returned Having 1 errors! 0 tests passed. (server crashed in the middle of the test, exit 134).
• WITH the fix: tests/clickhouse-test --test-runs 20 -- 04277 returns 20 tests passed. 0 tests skipped. with full settings randomization on.

f) Fix is general? Yes. The check is placed once in getFunctionLayer and is reached from all three function_name_parser-feeding sites: ParserFunction::parseImpl, the table-function ViewLayer path, and the inner OPERAND state-machine site. No code path that creates an ASTFunction from a ParserFunctionName-parsed identifier bypasses it. The "trace upstream" question — why does an identifier with empty name() reach a function-call position? — is answered: ASTIdentifier legitimately leaves full_name empty when the identifier contains a parameter hole; the bug is that the caller then used that empty string as a function name without checking. The fix is at the call boundary, not on a defensive null check.

cc @evillique @george-larionov - could you review this? It is a parser-side fix for the chronic STID 1941-1bfa AST round-trip failure: rejects query parameter substitution as a function name ({F:Identifier}(args)) at parse time in getFunctionLayer, since such an identifier produces an ASTFunction with empty name that the formatter writes as just (args) and the round-trip check fails.

@groeneai rebase on top of the fresh master

@evillique rebased on current master in 5de9b39 (no conflicts). CI rerunning.

Workflow [PR], commit [5de9b39]

Summary: ✅

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR rejects query-parameter placeholders in function-name positions before they can create an empty-name ASTFunction, which addresses the STID 1941-1bfa AST round-trip failure for shapes such as {F:Identifier}(args) and {db:Identifier}.tbl.fn(args). The parser change is narrowly placed in getFunctionLayer, and the new test covers the minimal reproducer, the compound COLUMNS shape, the DESCRIBE TABLE wrapper, and nearby valid parameter substitutions. I did not find a code-level correctness issue.

PR Metadata

• 💡 The Changelog category should not be Bug Fix (user-visible misbehavior in an official stable release). The PR body says this is a debug-build AST round-trip failure, and the check in executeQueryImpl is under #ifndef NDEBUG; per ClickHouse changelog guidance, debug/sanitizer/fuzzer-only LOGICAL_ERROR fixes are not stable-release bug fixes. Suggested category: Improvement (or CI Fix or Improvement if maintainers want no changelog entry).
• 💡 If using Improvement, suggested entry: Reject query parameter substitutions in function-name positions, such as {F:Identifier}(args)or{db:Identifier}.tbl.fn(args), at parse time with a clear SYNTAX_ERROR instead of constructing an invalid empty-name function AST.

Final Verdict

Status: ⚠️ Request changes

Minimum required action: update the changelog category, and adjust the changelog entry if keeping it under Improvement.

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered by tests: 6/6 (100.00%) | Lost baseline coverage (was covered on master, now uncovered in this PR): 1 line(s) · Uncovered code

Full report · Diff report

@evillique closing instead of rebasing. Master already fixes this exact case.

PR #106635 (merged 2026-06-07, abc4092) rejects a query-parameter substitution in a function-name position at the source, in FunctionNameLayer::parseImpl via ASTIdentifier::isParam(), and adds chassert(!function_name.empty()) at the same getFunctionLayer line this PR edits. That chassert makes this PR's throw unreachable, which is exactly the throw-vs-chassert conflict on the rebase.

Their test 04319_query_parameter_as_function_name.sql covers all three reproducers here, including the verbatim DESCRIBE TABLE (SELECT {db:Identifier}.tbl.COLUMNS(...)) fuzzer case. No new master "Inconsistent AST formatting" failures for this subvariant since that merge.

Superseded by #106635, thanks @ vitlibar.