iframe-proxy

groeneai · 2026-06-07T22:59:45Z

The bracket subscript on kql_array_sort_asc / kql_array_sort_desc was implemented by an in-place rename of a sibling arrayElement operand to tupleElement inside the operator-folding code in ParserExpression. The rewrite also fired against arrayElement nodes built via call syntax (arrayElement(kql_array_sort_*(...), i)), which destroyed the user-built AST and broke the format/parse/format round-trip check executed in debug builds.

The original AST stored arrayElement(arrayElement(kql_array_sort_*, i), j) because the inner was constructed by call syntax (is_operator = false). Formatting wrote arrayElement(kql_array_sort_*, i)[j], and the re-parse of that string went through the same code path and renamed the inner to tupleElement. The resulting AST had a different tree hash than the original and executeQueryImpl raised LOGICAL_ERROR Inconsistent AST formatting (STID: 1941-1bfa) across many BuzzHouse runs.

The fix restricts the inner rewrite to siblings whose is_operator flag is set. Operator-syntax kql_array_sort_*(...)[i][j] still folds to arrayElement(tupleElement(kql, i), j). Call-syntax shapes are kept verbatim and survive the round-trip; the type checker reports the semantic mismatch instead of an internal LOGICAL_ERROR.

Reproducer (debug build):

SELECT 1 WHERE (arrayElement(kql_array_sort_asc([(1, 2)]), 1)[2]).1 = 1;

Pre-fix raises LOGICAL_ERROR Inconsistent AST formatting (STID: 1941-1bfa). Post-fix raises ILLEGAL_TYPE_OF_ARGUMENT (the round-trip check passes; the call-syntax arrayElement applied to a Tuple is correctly rejected by type analysis).

Closes: #105396
Related: #91164

Latest CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=91164&sha=1102a6dbcb0f2cd3788e5c04bbdc1b1a78b76121&name_0=PR&name_1=BuzzHouse%20%28amd_debug%29

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):

Fix a server-side LOGICAL_ERROR Inconsistent AST formatting: ... (STID: 1941-1bfa) raised by the AST round-trip check in debug builds when an arrayElement(kql_array_sort_asc(...), i) or arrayElement(kql_array_sort_desc(...), i) was constructed via call syntax and a further [j] was applied. The parser no longer destructively renames a call-syntax sibling arrayElement, preserving the user-supplied shape across format/parse/format.

Documentation entry for user-facing changes

Documentation is unchanged

…ay_sort_* The bracket subscript on kql_array_sort_asc/desc was implemented in the operator-folding code by an in-place rename of a sibling arrayElement operand to tupleElement. The rewrite also fired against arrayElement nodes built via call syntax (arrayElement(kql_array_sort_*(...), i)), which destroyed the user-built shape and broke the format/parse/format round-trip check executed in debug builds. The original AST stored arrayElement(arrayElement(kql_array_sort_*, i), j) because the inner was constructed by call syntax (is_operator=false). Formatting wrote arrayElement(kql_array_sort_*, i)[j], and the re-parse of that string went through this same code path and renamed the inner to tupleElement. The resulting AST had a different tree hash than the original and executeQueryImpl raised LOGICAL_ERROR 'Inconsistent AST formatting (STID: 1941-1bfa)' across many BuzzHouse runs (issue ClickHouse#105396). Restrict the inner rewrite to siblings whose is_operator flag is set. Operator-syntax kql_array_sort_*(...)[i][j] still folds to arrayElement(tupleElement(kql, i), j), but call-syntax shapes are kept verbatim and survive the round-trip; the type checker reports the semantic mismatch instead of an internal LOGICAL_ERROR. Closes: ClickHouse#105396 Reproducer (debug build): SELECT 1 WHERE (arrayElement(kql_array_sort_asc([(1, 2)]), 1)[2]).1 = 1 Pre-fix raises LOGICAL_ERROR 'Inconsistent AST formatting (STID: 1941-1bfa)'. Post-fix raises ILLEGAL_TYPE_OF_ARGUMENT (the round-trip check passes; the call-syntax arrayElement applied to a Tuple is correctly rejected by type analysis). Latest CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=91164&sha=1102a6dbcb0f2cd3788e5c04bbdc1b1a78b76121&name_0=PR&name_1=BuzzHouse%20%28amd_debug%29 Related: ClickHouse#91164 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

groeneai · 2026-06-07T23:00:17Z

Pre-PR validation gate

#	Question	Answer
a	Deterministic repro?	Yes. `SELECT 1 WHERE (arrayElement(kql_array_sort_asc([(1, 2)]), 1)[2]).1 = 1` raises `LOGICAL_ERROR` `Inconsistent AST formatting (STID: 1941-1bfa)` on every debug-build run pre-fix; the new regression test `04325_stid_1941_1bfa_kql_array_sort_arrayelement_roundtrip.sql` exercises the same shape.
b	Root cause explained?	The operator-folding code in `mergeElement` rewrites a sibling `arrayElement(kql_array_sort_*, ...)` operand to `tupleElement` in place. When the inner `arrayElement` was built via call syntax (`is_operator = false`), the formatter still emits `arrayElement(...)`, but on re-parse the same code path rewrites the inner to `tupleElement`. The original AST and the re-parsed AST then differ in the inner function name, the tree hash check in `executeQueryImpl` fails, and the round-trip raises `LOGICAL_ERROR` `Inconsistent AST formatting`.
c	Fix matches root cause?	Yes. The fix gates the inner rewrite on `first_arg_as_node->isOperator()` so call-syntax `arrayElement` nodes are no longer mutated. Operator-syntax `kql_array_sort_*(...)[i][j]` still folds to `arrayElement(tupleElement(kql, i), j)` because the inner sibling there has `is_operator = true`.
d	Test intent preserved / new tests added?	New regression test `04325_stid_1941_1bfa_kql_array_sort_arrayelement_roundtrip.sql` covers: (1) operator-syntax `kql[i]` and `kql[i][j]` keep producing `tupleElement` and `arrayElement(tupleElement, ...)`, (2) call-syntax `arrayElement(kql, i)` and `arrayElement(arrayElement(kql, i), j)` round-trip unchanged, (3) mixed call+bracket `arrayElement(kql, i)[j]` round-trips and reports `ILLEGAL_TYPE_OF_ARGUMENT` instead of `LOGICAL_ERROR`. Existing tests `04004_kql_array_sort_const`, `04052_kql_array_sort_row_independence`, `02366_kql_func_dynamic`, `00343_array_element_generic`, `00292_parser_tuple_element`, `00267_tuple_array_access_operators_priority`, plus 23 other parser tests all pass.
e	Both directions demonstrated?	Yes. Pre-fix server (master HEAD): the reproducer query raises `Code 49 LOGICAL_ERROR Inconsistent AST formatting (STID: 1941-1bfa)`. Post-fix server (same query): raises `Code 43 ILLEGAL_TYPE_OF_ARGUMENT` (the round-trip check passes; the call-syntax `arrayElement` applied to a `Tuple` is correctly rejected by type analysis).
f	Fix is general, not a narrow patch?	Yes. The fix targets the destructive in-place rewrite at its origin (the sibling rewrite branch in `mergeElement`). No similar in-place AST-name mutations exist elsewhere in `ParserExpression` for `arrayElement` / `tupleElement` (verified by `grep -n 'first_arg_as_node\|->name = ' src/Parsers/ExpressionListParsers.cpp`). The first branch (renaming `cur_op.function_name`) is fine because it sets the operator state before any ASTFunction is built, not the name of an existing node.

Session id: cron:clickhouse-ci-task-worker:20260607-221100

groeneai · 2026-06-07T23:00:36Z

cc @evillique @alexey-milovidov — could you review this? It fixes the chronic STID 1941-1bfa AST round-trip failure where the kql_array_sort subscript hack in ParserExpression in-place renamed an existing call-syntax arrayElement to tupleElement, breaking the format/parse/format check in debug builds. The fix gates the rewrite on is_operator, preserving call-syntax shapes; operator-syntax kql[i][j] still folds correctly.

clickhouse-gh · 2026-06-07T23:02:02Z

Workflow [PR], commit [b76c782]

Summary: ❌

Performance Comparison: Performance dashboard

job_name	test_name	status	info
BuzzHouse (amd_debug)		FAIL
	Logical error: Join restriction violated (STID: 3398-4490)	FAIL	cidb
Performance Comparison (arm_release, master_head, 1/6)		FAIL	Performance dashboard
	entropy #4::old	FAIL	query history
	entropy #4::new	FAIL	query history
	entropy #5::old	FAIL	query history
	entropy #5::new	FAIL	query history
	entropy #8::old	FAIL	query history
	entropy #8::new	FAIL	query history
	fuse_sumcount #1::old	FAIL	query history
	fuse_sumcount #1::new	FAIL	query history
	logical_functions_small #9::old	FAIL	query history
	logical_functions_small #9::new	FAIL	query history
	10 more test cases not shown

AI Review

Summary

This PR narrows the kql_array_sort_asc / kql_array_sort_desc bracket-folding rewrite so only operator-syntax inner arrayElement nodes are renamed to tupleElement, preserving call-syntax ASTs and avoiding the debug format/parse/format LOGICAL_ERROR. I reviewed the current diff, surrounding parser and formatter paths, PR metadata, prior discussion, and the available Praktika report, and did not find an actionable correctness, safety, or coverage issue.

Final Verdict

Status: ✅ Approve

clickhouse-gh · 2026-06-08T02:17:49Z

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	84.50%	84.60%	+0.10%
Functions	92.30%	92.30%	+0.00%
Branches	77.20%	77.20%	+0.00%

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

Full report · Diff report

groeneai · 2026-06-08T03:37:07Z

CI Ledger — covered SHA: `b76c782`

CI fully finished. Two job-level failures; neither is PR-caused. Both surface chronic trunk issues with existing tracking.

job	classification	disposition
`BuzzHouse (amd_debug)` — `Logical error: 'Join restriction violated' (STID: 3398-4490)`	NOT PR-caused — pre-existing trunk bug	The failing query is `SELECT ifNull(count(DISTINCT ...)) ... CROSS JOIN ... INNER ALL JOIN (VALUES ...)` (no `kql_array_sort`, no `arrayElement`, no `tupleElement`). It hits `JoinOrderOptimizer::solveGreedy` (`joinOrder.cpp:642`) and is unrelated to the parser-side fix in this PR. STID 3398-4490 has a master sighting on 2026-06-05 and an unrelated PR #106211 sighting on 2026-05-31. Tracked: external fix PR #106066 by @vdimir is in flight (umbrella issue #106426).
`Performance Comparison (arm_release, master_head, 1/6)` — `2 too long, 7 slower, 3 unstable`	NOT PR-caused — chronic infra noise	All test rows on this lane are `success`; only the aggregator fires statistical-threshold breaches. The lane has been firing across many unrelated PRs in the last 7 days (#104976 ×4, #101796 ×4, #100397 ×2, #106337 ×2, #101783 ×2, #106569 ×2, #106414 ×2, plus 8 more). PR diff is parser-only; cannot affect runtime perf. Tracked under chronic family `2026-06-04-ci-chronic-performance-comparison-armr`.

All other 141 checks pass (Style, Fast, Builds, all FT shards, all stress, IT, Unit, Bugfix validation, Upgrade, Compatibility, Smoke).

cron:our-pr-ci-monitor:20260608-033000

groeneai · 2026-06-16T15:09:03Z

groeneai mentioned this pull request Jun 7, 2026

Parallelize dictGetKeys constant path and constant fold inverse dictionary lookup with dictGetKeys #91164

Merged

1 task

pufit added the can be tested Allows running workflows for external contributors label Jun 7, 2026

clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Jun 7, 2026

groeneai closed this Jun 16, 2026

Sunbelt Computer Software

PL/B Language Development and Support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix STID 1941-1bfa: stop mutating call-syntax arrayElement on kql_array_sort_*#106691

Fix STID 1941-1bfa: stop mutating call-syntax arrayElement on kql_array_sort_*#106691
groeneai wants to merge 1 commit into
ClickHouse:masterfrom
groeneai:groeneai/fix-stid-1941-1bfa-arrayelem-kql-roundtrip

groeneai commented Jun 7, 2026

Uh oh!

groeneai commented Jun 7, 2026

Uh oh!

groeneai commented Jun 7, 2026

Uh oh!

clickhouse-gh Bot commented Jun 7, 2026 •

edited

Loading

Uh oh!

clickhouse-gh Bot commented Jun 8, 2026

Uh oh!

groeneai commented Jun 8, 2026

Uh oh!

groeneai commented Jun 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

groeneai commented Jun 7, 2026

Changelog category (leave one):

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

Uh oh!

groeneai commented Jun 7, 2026

Pre-PR validation gate

Uh oh!

groeneai commented Jun 7, 2026

Uh oh!

clickhouse-gh Bot commented Jun 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review

Summary

Final Verdict

Uh oh!

clickhouse-gh Bot commented Jun 8, 2026

LLVM Coverage Report

Uh oh!

groeneai commented Jun 8, 2026

CI Ledger — covered SHA: b76c782

Uh oh!

groeneai commented Jun 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

clickhouse-gh Bot commented Jun 7, 2026 •

edited

Loading

CI Ledger — covered SHA: `b76c782`