iframe-proxy

groeneai · 2026-06-16T15:08:11Z

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 LOGICAL_ERROR ("Inconsistent AST formatting") when a [i] subscript on a kql_array_sort_asc/kql_array_sort_desc result is followed by another operator, for example kql_array_sort_desc([...])[i].j.

Description

CI report (BuzzHouse, amd_debug, STID 1941-1bfa): https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=107210&sha=61447331cad5ae84cca31abe742548cfa907b3db&name_0=PR&name_1=BuzzHouse%20%28amd_debug%29

kql_array_sort_asc/kql_array_sort_desc return Tuple(Array, ...), so a [i] subscript on their result is tuple-element access and the parser rewrites the arrayElement operator to tupleElement. That rewrite lived only in Layer::mergeElement. When the subscript is immediately followed by another equal-or-lower-priority operator (.j, [k], +, =, ...), the arrayElement node is instead built by the operator-merge loop in ParserExpressionImpl::tryParseOperator, which had no rewrite, so a raw arrayElement(kql_array_sort_*, i) survived. The formatter emits that as [i], which re-parses (now with no trailing operator) through the rewrite as tupleElement, so the format-parse-format round-trip diverged and the consistency check in executeQueryImpl aborted the server with a LOGICAL_ERROR in debug and sanitizer builds.

Minimal reproducer (aborts the server on master, returns a clean type error with this fix):

SELECT kql_array_sort_desc([3, 1, 2])[0].4;

The rewrite is factored into Layer::foldKqlArraySortSubscript and applied at both arrayElement construction sites. It inspects the actual first argument of the just-built node instead of guessing from the operand stack, and keeps the operator-syntax guard so call-syntax arrayElement(kql_array_sort_*, ...) is preserved verbatim.

… by an operator (STID 1941-1bfa) kql_array_sort_asc/kql_array_sort_desc return Tuple(Array, ...), so a [i] subscript on their result is tuple-element access and the parser rewrites the arrayElement operator to tupleElement. That rewrite lived only in Layer::mergeElement. When the subscript is immediately followed by another equal-or-lower-priority operator (.j, [k], +, =, ...), the arrayElement node is instead built by the operator-merge loop in ParserExpressionImpl::tryParseOperator, which had no rewrite, so a raw arrayElement(kql_array_sort_*, i) survived. The formatter emits that as [i], which re-parses (now with no trailing operator) through the rewrite as tupleElement, so the format-parse-format round-trip diverged and the consistency check in executeQueryImpl aborted the server with a LOGICAL_ERROR in debug and sanitizer builds. The rewrite is factored into Layer::foldKqlArraySortSubscript and applied at both arrayElement construction sites. It inspects the actual first argument of the just-built node instead of guessing from the operand stack, and keeps the operator-syntax guard so call-syntax arrayElement(kql_array_sort_*, ...) is preserved verbatim. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

groeneai · 2026-06-16T15:08:37Z

Pre-PR validation gate

#	Question	Answer
a	Deterministic repro?	Yes. `SELECT kql_array_sort_desc([3, 1, 2])[0].4` (and any `kql_array_sort_*([...])[i] <op>` shape) aborts the server with the `Inconsistent AST formatting` `LOGICAL_ERROR` on every run on master HEAD; debug build, no randomization needed.
b	Root cause explained?	Yes. `kql_array_sort_*` returns `Tuple(Array, ...)`, so `[i]` on it is tuple-element access; the parser rewrites that `arrayElement` operator to `tupleElement`. The rewrite existed only in `Layer::mergeElement`. When the subscript is followed by another equal-or-lower-priority operator, the `arrayElement` is built instead by the operator-merge loop in `ParserExpressionImpl::tryParseOperator`, which had no rewrite, so a raw `arrayElement(kql, i)` survived. The formatter emits it as `[i]`, which re-parses (no trailing operator) through the rewrite as `tupleElement`, so format1 != format2 and `executeQueryImpl` aborts.
c	Fix matches root cause?	Yes. The rewrite is factored into `Layer::foldKqlArraySortSubscript` and applied at BOTH `arrayElement` construction sites, eliminating the asymmetry that produced the un-round-trippable node. No symptom guard, no assertion widening.
d	Test intent preserved / new tests added?	New regression test `04338_kql_array_sort_subscript_then_operator_1941_1bfa` added (round-trip stability for the new shapes, call-syntax preservation, KQL eval semantics). Existing kql tests (04004, 04052, 02366, 03784) still pass unchanged.
e	Both directions demonstrated?	Yes. Without the fix every reproducer aborts (exit 134 / connection-refused after server abort); with the fix all round-trip STABLE, the invalid shapes return `ILLEGAL_TYPE_OF_ARGUMENT` (Code 43) instead of aborting, and the server stays alive.
f	Fix is general, not a narrow patch?	Yes. The helper covers every `arrayElement`-building site (the only two in the parser), both `kql_array_sort_asc` and `kql_array_sort_desc`, the direct (`kql[i] <op>`) and chained (`kql[i][j] <op>`) cases, and preserves call-syntax `arrayElement(kql, ...)` via the `isOperator()` guard so user-written calls still round-trip.

Session id: cron:clickhouse-worker-slot-2:20260616-142500

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

cc @evillique @george-larionov -- could you review this? It fixes a debug-build server abort (Inconsistent AST formatting, STID 1941-1bfa) for kql_array_sort_*([...])[i] followed by another operator. The arrayElement->tupleElement subscript rewrite only ran in Layer::mergeElement, so the operator-merge loop in tryParseOperator left a non-round-trippable arrayElement(kql, i); the fix factors the rewrite into one helper applied at both construction sites.

clickhouse-gh · 2026-06-16T17:08:59Z

Workflow [PR], commit [bb54b60]

Summary: ❌

Performance Comparison: Performance dashboard

job_name	test_name	status	info	comment
Integration tests (amd_asan_ubsan, db disk, old analyzer, 4/6)		ERROR
	Timeout	FAIL	cidb

AI Review

Summary

This PR fixes the parser asymmetry for kql_array_sort_asc/kql_array_sort_desc subscript operators by applying the arrayElement to tupleElement fold at both AST construction sites. The focused regression test covers direct trailing operators, chained subscripts, call-syntax preservation, and executable semantics, and I did not find any unresolved correctness issue that warrants an inline review comment.

Final Verdict

Status: ✅ Approve. No required changes.

groeneai · 2026-06-17T04:39:50Z

CI summary for head 5a342b6. All three red checks are pre-existing failures unrelated to this parser-only change (KQL array_sort subscript fold). The new regression test 04338_kql_array_sort_subscript_then_operator_1941_1bfa passed (237 OK, 0 FAIL).

Stateless tests (amd_tsan, s3 storage, parallel, 2/2) -> 01710_projections: known flaky, 5 unrelated PRs and 0 master failures in 30d. https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=107634&sha=5a342b69fe28bf9c7cedb4b0a5ace9fa8160c469&name_0=PR
Stress test (arm_asan_ubsan, s3) -> "Block structure mismatch in UnionStep" (STID 0993-27f0): chronic trunk bug, present on master (2 hits) and several other PRs in 30d; tracked separately.
Performance Comparison (arm_release, master_head, 6/6): statistical noise (lazyMaterialization, set_index, short_circuit_nulls), not reproducible.

Finish Workflow is green; Mergeable Check is red only due to the non-PR-caused failures above.

alexey-milovidov · 2026-06-17T18:27:26Z

@groeneai, fix flaky test 01710_projections and fix this stress test failure: Block structure mismatch in UnionStep.

…ray-sort-subscript-operator-1941-1bfa

groeneai · 2026-06-17T18:50:52Z

@alexey-milovidov done.

01710_projections: the branch predated #107698 (merged 16:56Z), which pins index_granularity = 8192 so --random-merge-tree-settings can no longer inject a small value that splits the projection part across concurrent s3 read streams (the float accumulation-order diff). Branch updated to current master (bb54b60); CI is re-running on the new head.

Block structure mismatch in UnionStep (STID 0993-27f0): chronic trunk crash, not from this change. Present on master and several unrelated PRs over the last 30 days; it surfaces here only because the branch merges master. This PR touches only src/Parsers/ExpressionListParsers.cpp (no plan/optimizer code). Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=107634&sha=5a342b69fe28bf9c7cedb4b0a5ace9fa8160c469&name_0=PR&name_1=Stress%20test%20%28arm_asan_ubsan%2C%20s3%29 . Fix in flight: #107719.

clickhouse-gh · 2026-06-17T22:24:54Z

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	85.20%	85.30%	+0.10%
Functions	92.30%	92.30%	+0.00%
Branches	77.40%	77.50%	+0.10%

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

Full report · Diff report

groeneai · 2026-06-17T23:47:45Z

CI fully finished on bb54b60. The only red is Integration tests (amd_asan_ubsan, db disk, old analyzer, 4/6), and it is an infra batch-timeout, not a test failure: the job log ends with 1219 passed, 22 skipped, 13 warnings in 7236.66s (2:00:36) followed by session-timeout occurred during test execution. Zero pytest failures; the batch (heavy iceberg-spark + s3 suites under ASan) just hit the 2h session cap. This PR only touches src/Parsers/ExpressionListParsers.cpp, which cannot affect integration-test runtime.

Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=107634&sha=bb54b60df7766ca7f7801181737f140be90a362d&name_0=PR&name_1=Integration%20tests%20%28amd_asan_ubsan%2C%20db%20disk%2C%20old%20analyzer%2C%204%2F6%29

The two failures from the directive are green on this head: 01710_projections (picked up merged #107698) and the UnionStep stress test (chronic trunk STID 0993-27f0, fix in flight at #107719). Tracking the batch timeout separately as CI infra.

alexey-milovidov · 2026-06-22T03:38:22Z

@groeneai, provide a fix for timeout in integration tests: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=107634&sha=bb54b60df7766ca7f7801181737f140be90a362d&name_0=PR&name_1=Integration%20tests%20%28amd_asan_ubsan%2C%20db%20disk%2C%20old%20analyzer%2C%204%2F6%29

alexey-milovidov · 2026-06-22T03:39:43Z

@groeneai, we don't like KQL, because it is experimental and was implemented with low quality.

Send another PR that will remove kql_array_sort_asc, kql_array_sort_desc with all associated complications, including all traces of it inside code, tests, and docs.

groeneai · 2026-06-22T04:51:58Z

Done: #108101 removes kql_array_sort_asc/kql_array_sort_desc and the KQL array_sort_asc/array_sort_desc entirely (code, tests, docs, fuzzer dicts), including the subscript-rewrite special case this PR was fixing and the array_sort-only KQL alias handling.

That makes this PR moot: with the functions gone, the [i] subscript on their result no longer reaches the tupleElement rewrite, so the inconsistent-AST LOGICAL_ERROR cannot occur. #108101 closes #106850 directly. I will close this one once #108101 is approved; let me know if you would rather keep it open.

Algunenano · 2026-06-24T09:34:23Z

This was referenced Jun 16, 2026

Logical error: 'Inconsistent AST formatting between 'ExpressionList' and 'ExpressionList' in the query: (STID: 1941-1bfa) #105396

Closed

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

Closed

PedroTadim added the can be tested Allows running workflows for external contributors label Jun 16, 2026

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

groeneai mentioned this pull request Jun 17, 2026

Spawn background schedule pool threads lazily #105066

Merged

1 task

Merge remote-tracking branch 'origin/master' into groeneai/fix-kql-ar…

bb54b60

…ray-sort-subscript-operator-1941-1bfa

groeneai mentioned this pull request Jun 20, 2026

Fix empty TZ error #68921

Merged

21 tasks

groeneai mentioned this pull request Jun 22, 2026

Remove kql_array_sort_asc and kql_array_sort_desc #108101

Merged

Algunenano closed this Jun 24, 2026

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

groeneai commented Jun 16, 2026

Changelog category (leave one):

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

Description

Uh oh!

groeneai commented Jun 16, 2026

Pre-PR validation gate

Uh oh!

groeneai commented Jun 16, 2026

Uh oh!

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

Uh oh!

AI Review

Summary

Final Verdict

Uh oh!

groeneai commented Jun 17, 2026

Uh oh!

alexey-milovidov commented Jun 17, 2026

Uh oh!

groeneai commented Jun 17, 2026

Uh oh!

clickhouse-gh Bot commented Jun 17, 2026

LLVM Coverage Report

Uh oh!

groeneai commented Jun 17, 2026

Uh oh!

alexey-milovidov commented Jun 22, 2026

Uh oh!

alexey-milovidov commented Jun 22, 2026

Uh oh!

groeneai commented Jun 22, 2026

Uh oh!

Algunenano commented Jun 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

clickhouse-gh Bot commented Jun 16, 2026 •

edited

Loading