Workflow [PR], commit [68b5feb]

Summary: ✅

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR parallelizes the constant path of dictGetKeys and adds an analyzer rewrite that folds eligible dictGet(...)=const predicates into direct key filters. The latest head fixes the signed-zero/vector consistency and non-coordinator-layout issues from the previous review, but two performance/safety concerns remain around doing parallel dictionary scans inside scalar function execution and doing an unbounded analysis-time scan before falling back.

Findings

⚠️ Majors

• src/Analyzer/Passes/InverseDictionaryLookupPass.cpp:428 The constant-fold path still executes dictGetKeys completely before it knows whether the result can be folded under the 1 MiB constant-size guard. For large-hit values this scans and materializes the dictionary during analysis, discards the result when resolveConstantExpression refuses to produce a ConstantNode, and then falls back to the IN (SELECT ... FROM dictionary(...)) rewrite that scans again at execution. This makes the large-hit case strictly more expensive than the old rewrite and can add avoidable analysis-time memory spikes.
  Suggested fix: make the fold bounded, for example by scanning with a row/byte budget and returning "too large" before materializing the full array, or skip the dictGetKeys fold when the pass cannot prove the result is small.

• src/Functions/FunctionDictGetKeys.cpp:397 [dismissed by author -- https://github.com/Parallelize dictGetKeys constant path and constant fold inverse dictionary lookup with dictGetKeys #91164#discussion_r3327058284] The scalar function path still starts a nested PipelineExecutor with num_threads derived from max_threads. Since executeImpl can run concurrently in several outer query pipeline streams and useDefaultImplementationForConstants is false, a query such as SELECT dictGetKeys('d', 'a', 'x') FROM big_table can run multiple inner dictionary scans in parallel, each requesting up to max_threads workers. The sink-based implementation removed the explicit ThreadPool, but it did not cap the nested executor against the outer query's thread budget.
  Suggested fix: keep the per-block scalar path single-threaded, cache/share the constant result per query, or reserve/share an explicit thread budget so total query parallelism remains bounded by max_threads.

Final Verdict

Status: ⚠️ Request changes

Minimum required actions: bound or skip the large-result analysis-time fold, and cap/share the nested constant-path parallelism so dictGetKeys cannot exceed the query thread budget when called from an outer pipeline.

For big dictionary:

AMD:

ARM:

Merged master (the branch was 318 commits behind and red) and verified the two affected translation units (InverseDictionaryLookupPass.cpp, FunctionDictGetKeys.cpp) still compile cleanly.

The only CI failure is BuzzHouse (amd_debug): Logical error: 'Inconsistent AST formatting: the original AST: (STID: 1941-1bfa). This is unrelated to this PR — it is a parser/formatter roundtrip inconsistency on a fuzzer-generated query that contains no dictGet/dictGetKeys (it involves kql_array_sort_asc, tupleElement, nested numeric/emoji identifiers like t0d0.c0.c2.2.3). It is already tracked with the exact same STID: 1941-1bfa in #105396.

@groeneai, could you take a look at #105396 and provide a fix in a separate PR if one isn't already in progress? 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

Picked up the BuzzHouse failure (STID 1941-1bfa, issue #105396). The crash on this PR is a sub-variant of the same chronic round-trip family that PR #104991 (CODEC/BACKUP_NAME, merged) and PR #105839 (query parameter substitution, open) cover.

This sub-variant is parser-side: the kql_array_sort subscript hack in ParserExpression was rewriting a sibling arrayElement(kql_array_sort_*, ...) operand to tupleElement in place. When the inner arrayElement was built via call syntax (is_operator = false), the destructive rewrite broke the format/parse/format round-trip check in debug builds and raised LOGICAL_ERROR Inconsistent AST formatting (STID: 1941-1bfa).

Fix is in PR #106691 — gates the rewrite on is_operator. Operator-syntax kql[i][j] still folds to arrayElement(tupleElement(kql, i), j). Call-syntax shapes round-trip cleanly, regression test 04325_stid_1941_1bfa_kql_array_sort_arrayelement_roundtrip covers both shapes plus the BuzzHouse-failing pattern.

Once #106691 is merged the BuzzHouse run for #91164 should be unblocked. I will keep monitoring this PR's CI.

Merged master to refresh the base (merge-base was 2026-06-07) and trigger a clean CI run.

The only CI red was the AST fuzzer logical error Column identifier A is already registered (STID 4697-4326), which is unrelated to this PR: the same error hits master (pull_request_number = 0) and dozens of other PRs every day in CIDB. It is already tracked by #106649 and fixes are in progress in #106025 and #106414.

@groeneai, this is the known mutations IN-subquery analyzer regression — no separate action needed here, just flagging that the red on this PR is that pre-existing issue.

Merged master (the branch was ~1180 commits behind and red) and pushed fixes for the red CI and the access-gating finding.

Red Fast test — references after operators-as-functions

The 6 failing dictGetKeys / inverse-dictionary tests (03701, 03702, 03703, 03713, 04201, 04276) failed only because master PR #94681 made FunctionNode::toASTImpl always format operators as function calls in EXPLAIN SYNTAX / toAST (a IN b → in(a, b), a < b → less(a, b), a AND b → and(a, b), a NOT LIKE b → notLike(a, b)). Regenerated the references; verified the change is purely cosmetic plan formatting — every query result and data row is byte-identical, only operator rendering inside EXPLAIN plans changed. The full 15-test inverse_dictionary_lookup + dict_get_keys stateless suite passes locally.

Access-gating finding (InverseDictionaryLookupPass.cpp:414) — fixed

Moved isCreateTemporaryTableGranted() below the constant-fold path so it gates only the fallback IN (SELECT ... FROM dictionary(...)) rewrite. A user with dictGet access but without CREATE TEMPORARY TABLE now reaches the dictGetKeys constant fold (key = const / key IN [..] / 0). Added 04401_inverse_dictionary_lookup_rbac_constant_fold, which proves via EXPLAIN that the folded plan contains neither dictGet nor a dictionary() subquery for such a user, that dictionary() is still ACCESS_DENIED, and that results match the unoptimized path.

Remaining AI-review items — left for review

• Nested parallel executor in the constant path (FunctionDictGetKeys.cpp): the inline thread @novikd opened is resolved — the original ThreadPool was replaced with stream-attached sinks, and dictGetKeys is isDeterministic() = false, so the parallel scan only runs once at analysis time for the pass-driven fold; the inner executor runs with use_concurrency_control, so its CPU slots come from the query's shared pool. This is the PR's core parallelization, so I left it as the resolved thread stands. @novikd, please confirm whether you want any additional capping here.
• Bounded large-hit fold (InverseDictionaryLookupPass.cpp:437): this remains a deliberate trade-off (analysis-time execution is bounded by max_memory_usage; the cost is wasted work only in the rare large-hit case, in exchange for the common-case win). A bounded dictionary-scan helper would be a larger, separate change; leaving the thread open for a decision.
• The changelog entry could mention both surfaces (constant dictGetKeys and the inverse dictGet equality fold).

Addressed the AI review Request changes verdict (deterministic dictGetKeys constant-path order) and refreshed the branch.

Fix (d7288222579) — dictGetKeys constant-path ordering determinism:

• DictionarySource now tags every chunk it produces with its sequential block number (a DictionaryBlockNumber chunk info). Block n always covers the key range [n * max_block_size, (n + 1) * max_block_size).
• executeConstPath gathers the matched chunks from all per-stream sinks and merges them in ascending block-number order, reproducing the single-stream scan order. The result is now deterministic and identical to the vector path (dictGetKeys(d, a, 'x') vs dictGetKeys(d, a, materialize('x'))) regardless of how blocks are spread across reading streams. The parallel scan/filter is preserved.
• New test 04413_dict_get_keys_const_path_order_consistency forces many one-row blocks (max_block_size = 1) and max_threads = 8 so the constant path is genuinely parallel, then pins its result (order included) to the vector path for values matching several keys.

Also merged current master into the branch (clean, no conflicts) to refresh CI.

The remaining unresolved thread on InverseDictionaryLookupPass.cpp (bounded dictGetKeys fold to avoid the large-hit double-scan) is left as the design follow-up @alexey-milovidov flagged for himself; it is not part of the verdict's required actions.

@groeneai, the only red on the previous run was AST fuzzer (amd_debug) failing with Logical error: Block structure mismatch in A stream: different columns (STID 0993-27f0), which is the known flake tracked in #108142 (matched across unrelated PRs and master, not specific to this change). Could you confirm/track? It should re-run on the freshly triggered CI.

@alexey-milovidov Confirmed: 0993-27f0 is the chronic trunk flake (tracked in #108142), not caused by this PR.

CIDB over the last 30 days: 10 master hits plus 104 distinct unrelated PRs (131 total events), from 2026-06-06 to 2026-06-30 05:02 UTC, across AST fuzzer (amd_debug / arm_asan_ubsan) and Stress test (amd_msan / arm_asan_ubsan / serverfuzz). On this PR it appeared once: AST fuzzer (amd_debug) on 756da4d. The logical error is in the UnionStep / IntersectOrExceptStep block-structure path, unrelated to the dictGetKeys and inverse-dictionary-lookup changes here.

Master example, same check: https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=cd3c529cd3a8002caa1a4ba585a39da636836784&name_0=MasterCI&name_1=AST%20fuzzer%20%28amd_debug%29&name_1=AST%20fuzzer%20%28amd_debug%29

In-flight fix: #107719. Expect it to clear on the re-run.

Addressed the AI review ❌ Block verdict — both required actions are now fixed in 68b5feb7320 (built and tested locally).

1. Blocker — non-coordinator dictionary layouts (FunctionDictGetKeys.cpp:114).
The constant path asserted DictionaryBlockNumber with chassert and then dereferenced it unconditionally, so a release build (where chassert does not evaluate) would dereference a null chunk info for a valid dictGetKeys(dict, attr, const) call on layouts that read in a single stream (DIRECT, COMPLEX_KEY_DIRECT, POLYGON, REGEXP_TREE, ...). The block number is now optional — read only when present — and the block-number merge sort runs only for multi-stream reads (num_streams > 1), which come exclusively from DictionarySourceCoordinator and always attach the tag. Single-stream layouts keep the sink's insertion (scan) order.
New regression 04491_dict_get_keys_const_path_non_coordinator_layout exercises DIRECT / COMPLEX_KEY_DIRECT; it does not crash on a RelWithDebInfo build (where chassert is a no-op) and the constant path agrees with the vector path for every value.

2. Major — vector-path floating zero (FunctionDictGetKeys.cpp:657).
ColumnVector<T>::updateHashWithValue hashes the stored bytes, so 0.0 and -0.0 hash differently even though SQL equals(0.0, -0.0) is true. The hash is a mandatory prefilter before equals, so dictGetKeys(d, 'f', CAST(-0.0 AS Float64)) and its materialize(...) form could disagree. Floating-point attributes (optionally Nullable) now normalize signed zero in the bucket / cache hash on both the query-value side and the dictionary side, so SQL-equal values always share a bucket and the equals confirmation decides the match.
New regression 04490_dict_get_keys_float_signed_zero_consistency pins the constant and vector paths together for Float32 / Float64 with both zero signs stored in the dictionary, using explicit literal-vs-materialize(literal) comparisons.

Locally re-ran the full dict_get_keys + inverse_dictionary_lookup stateless suite (13 .sql tests) — all pass; the two new tests pass too.

The only remaining unresolved thread (InverseDictionaryLookupPass.cpp — bounded large-hit fold to avoid the double-scan) is the design follow-up @alexey-milovidov flagged for himself; it is not part of the verdict's required actions.

@groeneai, the sole CI red is Stress test (amd_msan) → Hung check failed, possible deadlock found, the chronic master-wide flake tracked in #107941. The hung stack is AST-fuzzer query analysis (executeASTFuzzerQueries → InterpreterSelectQueryAnalyzer) with zero dictGet / dictGetKeys frames, so it is unrelated to this change. Could you confirm/track? It should re-run on the freshly triggered CI.

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered: 318/332 (95.78%) · Uncovered code

Full report · Diff report

@alexey-milovidov Confirmed: the Stress test (amd_msan) Hung check failed, possible deadlock found on d7288222579 is the chronic master-wide flake (#107941), not caused by this PR. The hung stack is the server-side AST fuzzer in-flight query analysis (executeASTFuzzerQueries -> InterpreterSelectQueryAnalyzer) with zero dictGet / dictGetKeys frames.

CIDB (30d): the Stress test (amd_msan) hung-check hit 100 distinct unrelated PRs plus 15 master runs (2026-06-03 to 2026-06-30); across all stress variants it is 611 distinct PRs plus 445 master events. Trunk-chronic, not PR-specific.

In-flight fixes for this AST-fuzzer hang class: #107640 (make the server-side AST fuzzer respect KILL/timeout/shutdown, merged) and #108212 (limit and cancel in-flight server-side AST fuzzer queries, merged 2026-06-28).

The freshly triggered CI on the new head 68b5feb7320 is already fully green (0 reds across 159 checks); the amd_msan Stress re-run passed. Tracking under #107941.

Report (failed run): https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=91164&sha=d7288222579cb7a427e4f37f1ca62f8f8c8491b8&name_0=PR&name_1=Stress%20test%20%28amd_msan%29