Skip sharding key IN rewrite when the sharding key contains a set

[groeneai]

Related: #107619

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 the Not-ready Set is passed as the second argument for function 'in' logical error that could be thrown when a Distributed table's sharding key expression itself contained an IN/subquery (a set) and optimize_skip_unused_shards_rewrite_in was enabled.

Description

Found by the AST fuzzer (STID 0250-4e52, tracked in #107619). Reported on the arm_debug Stress test of PR #101039; per @ alexey-milovidov the failure is unrelated to that PR, so this is a separate fix.

Reproducer (aborts in debug/sanitizer builds):

SELECT * FROM remoteSecure('127.{1..8}', view(SELECT number AS id FROM numbers(0)),
    bitAnd(1025, murmurHash3_32(minus(id, (2 IN (SELECT 1)))))) WHERE id IN (2, 3) LIMIT 444
SETTINGS optimize_skip_unused_shards = 1, optimize_skip_unused_shards_rewrite_in = 1,
    allow_nondeterministic_optimize_skip_unused_shards = 1;

CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=101039&sha=8b127de6068682ddc50262e918ab1a7dd3a0a307&name_0=PR&name_1=Stress%20test%20%28arm_debug%29

Root cause: OptimizeShardingKeyRewriteIn prunes shards by executing the standalone sharding-key ExpressionActions on constant values (shardContains -> executeFunctionOnField). The sharding-key expression is built directly from the sharding-key AST, so an IN/subquery in the sharding key becomes a FunctionIn whose set is never populated during planning. Executing it hits FunctionIn::executeImpl with an unbuilt set and throws the Not-ready Set logical error (OptimizeShardingKeyRewriteInVisitor.cpp:36 -> Functions/in.cpp:116).

Fix: skip the rewrite when the sharding-key expression contains a set, and query all shards instead. This is the same safe fallback already used for non-deterministic sharding keys, and the optimization was never valid for a set-containing key anyway (it cannot be constant-folded per value). Both the analyzer and old-analyzer call sites are guarded.

Distinct from the other in-flight Not-ready Set fixes (#109722, #102192), which address the buildOrderedSetInplace / QueryPlan path; neither touches OptimizeShardingKeyRewriteIn.

[groeneai]

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes. clickhouse-local (fake 2-shard cluster) EXPLAIN SELECT count() FROM dist WHERE dummy IN (0,1) where the sharding key is bitAnd(dummy + (0 IN (SELECT 1)), 1), with optimize_skip_unused_shards[_rewrite_in]=1. Aborts every time on master HEAD (exit 134), also reproduced with the exact fuzzer query on a real server.
b	Root cause explained?	The sharding-key ExpressionActions is built standalone from the sharding-key AST. OptimizeShardingKeyRewriteIn::shardContains executes it on constant Fields to prune shards. When the sharding key contains an IN/subquery, that becomes a FunctionIn whose set is never built during planning, so execution hits FunctionIn::executeImpl with an unbuilt set and throws Not-ready Set (in.cpp:116, via OptimizeShardingKeyRewriteInVisitor.cpp:36).
c	Fix matches root cause?	Yes. Skip the rewrite when the sharding-key expression contains a set (scan the DAG for a Set-typed node); query all shards, the same safe fallback already used for non-deterministic keys.
d	Test intent preserved / new test added?	New regression test 04545_shardkey_rewrite_in_subquery_set.sh (modelled on the #104478 empty-tuple test 04243). No existing test weakened.
e	Demonstrated both directions?	Yes. Pristine master: abort (exit 134). With fix: EXPLAIN exit 0, server stays alive, test outputs OK. Verified via clickhouse-test.
f	General, not a narrow patch?	Both invocation sites (analyzer + old-analyzer) are guarded via one shared helper. OptimizeShardingKeyRewriteIn is the only consumer of the standalone-execute path.
g	Generalizes across inputs?	The guard scans the whole sharding DAG for any Set-typed node, so it catches a set anywhere in the expression (nested inside functions, tuple-IN, NOT IN, globalIn, subquery-IN), not just the one fuzzer shape.
h	Backward compatible?	Yes. Only disables an optimization for a query class that previously aborted; for such keys the optimization was never valid. No setting default, on-disk/wire/replication format, or new-validation change. Result correctness is unaffected (all shards queried).
i	Invariants preserved?	Yes. OptimizeShardingKeyRewriteIn's contract (only prune shards it can prove exclude the value) is upheld: when it cannot evaluate the key it now queries all shards, matching the existing non-deterministic-key fallback.

Session id: cron:clickhouse-worker-slot-1:20260714-220600

[groeneai]

cc @azat @devcrafter — could you review? This skips the OptimizeShardingKeyRewriteIn shard-pruning rewrite when the Distributed sharding key itself contains an IN/subquery (a set), which otherwise executes the standalone sharding-key expression against an unbuilt set and throws the Not-ready Set logical error (#107619, STID 0250-4e52). All shards are queried in that case, matching the non-deterministic-key fallback.

[clickhouse-gh]

Workflow [PR], commit [8440d3b]

Summary: ❌

job_name	test_name	status	info	comment
Finish Workflow		FAIL
	python3 ./ci/jobs/scripts/workflow_hooks/new_tests_check.py	FAIL
Config Workflow		ERROR
Dockers Build (amd)		DROPPED
Dockers Build (arm)		DROPPED
Dockers Build (multiplatform manifest)		DROPPED
Style check		DROPPED
Code Review		DROPPED
Docs check		DROPPED
Docs check (Mintlify)		DROPPED
Fast test		DROPPED

[groeneai]

Both addressed in e6b9b24.

1. The guard now bails only for an unready set. The set-typed node is a ColumnConst(ColumnSet); tuple/storage sets are materialized eagerly (FutureSet::get() non-null) and are kept, only subquery-backed sets (get() null) trigger the fallback. Renamed to shardingKeyExpressionContainsNotReadySet, so pruning still applies to keys like bitAnd(dummy + (0 IN (1, 2)), 1).

2. Guarded the legacy path too. Moved the helper into OptimizeShardingKeyRewriteInVisitor and added the same early return to StorageDistributed::skipUnusedShards and skipUnusedShardsWithAnalyzer before they call evaluateExpressionOverConstantCondition.

Verified locally: with allow_experimental_analyzer = 0 and the subquery-set key, master HEAD aborts during planning (exit 134), the fix reaches shard connection instead (no abort). Regression test extended to cover the old-analyzer path and the ready tuple-set case.

[groeneai]

CI finish ledger — 2e65b38

Every failure below has an owner. Only CH Inc sync is exempt.

Check / test	Reason	Owner / fixing PR
Stateless tests (amd_asan_ubsan, distributed plan, parallel) — 158 unrelated tests	host-OOM ((total) memory limit exceeded, server-global OvercommitTracker; fleet-wide 01:00-04:45 UTC 2026-07-15, hit master + 15+ PRs, none in this PR's diff)	#110503 (ours, open)
Finish Workflow / Post Hooks	consequence of the distributed-plan job above	#110503 (ours, open)
CH Inc sync	-	CH Inc sync (private, not actionable)

The distributed plan, parallel lane runs at the default 0.6 concurrency while its sibling ASan lanes were cut to 0.4 for exactly this reason; today ~19 co-scheduled queries pushed server RSS past the shared cap and the (total) tracker killed dozens of trivial unrelated tests. None of this PR's changes are involved. Fix PR #110503 adds "(total) memory limit exceeded" to MESSAGES_TO_RETRY in tests/clickhouse-test.

Session id: cron:our-pr-ci-monitor:20260715-090000

[clickhouse-gh]

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	86.30%	86.40%	+0.10%
Functions	91.90%	91.90%	+0.00%
Branches	78.50%	78.50%	+0.00%

Changed lines: Changed C/C++ lines covered: 37/40 (92.50%) · Uncovered code

Full report · Diff report

[groeneai]

CI finish ledger - ff79dd8

Every failure below has an owner: a fixing PR (mine or external), or a full-effort fix task whose fixing-PR link will be posted here when it opens. Only CH Inc sync is exempt.

Check / test	Reason	Owner / fixing PR
Stateless tests (amd_llvm_coverage, ParallelReplicas, s3 storage, parallel) / 01666_merge_tree_max_query_limit	trunk regression, not caused by this diff (fires on master and on ~50 unrelated PRs in the same window)	#112385 (mine, merged 2026-07-29 15:37Z); this build predates it, so a rerun after the next master merge picks it up
Sync	-	CH Inc sync (private, not actionable)

Session id: cron:our-pr-ci-monitor:20260729-170000

[groeneai]

CI finish ledger — ff79dd8

Every failure below has an owner: a fixing PR (ours or external), or a full-effort fix task whose
fixing-PR link will be posted here when it opens. Only CH Inc sync is exempt.

Check / test	Reason	Owner / fixing PR
Stateless tests (amd_llvm_coverage, ParallelReplicas, s3 storage, parallel) / 01666_merge_tree_max_query_limit	fails 3/3 reruns under parallel replicas on coverage builds	#112385 (ours, merged 2026-07-29 15:37Z) - this build predates the fix

Session id: cron:our-pr-ci-monitor:20260729-164745

[azat]

Why the set is not ready?

This patch is no good

[groeneai]

The set is permanently unready, by construction.

The sharding key is table metadata, not part of a query. It becomes a standalone ExpressionActions once, at attach time: buildShardingKeyExpression (StorageDistributed.cpp:277-282) uses the 3-argument ExpressionAnalyzer ctor, which passes no prepared_sets (ExpressionAnalyzer.h:118-119), so the analyzer allocates its own (ExpressionAnalyzer.cpp:200-204) and registers the IN (SELECT ...) there as a FutureSetFromSubquery. That analyzer is a temporary: only the ExpressionActionsPtr escapes, getPreparedSets() is never read, so nothing ever adds a CreatingSetStep or calls buildSetInplace (PreparedSets.h:137-148). There is no pipeline at attach time in which the subquery could run.

So it is not "not ready yet". A subquery in a sharding key can never become ready.

You are right that this patch is no good. It guards one consumer of a state that should not exist, and it is also incomplete: the INSERT selector still aborts on this branch. Measured on head 8440d3b1e2b0, debug build:

CREATE TABLE loc (id UInt64) ENGINE = MergeTree ORDER BY id;
-- `two_shards`: any cluster with 2 shards
CREATE TABLE dsub (id UInt64) ENGINE = Distributed(two_shards, currentDatabase(), loc,
    bitAnd(id + (0 IN (SELECT 1)), 1));
INSERT INTO dsub SELECT number FROM numbers(4);

aborts with Not-ready Set through DistributedSink.cpp:727 -> in.cpp:116, same stack as on pristine master. The tuple form bitAnd(id + (0 IN (1, 2)), 1) inserts fine.

What I would do instead: reject a subquery-backed set in the sharding key where the key is validated, in checkShardingKeyExistsAndIsNumeric (:284-300), which is reached from both the constructor (:480) and ALTER (:1514) and already rejects correlated subqueries next door at :637. Gated on mode <= LoadingStrictnessLevel::CREATE, as :502 does, so tables already on disk still attach. Then the pruning paths and the INSERT selector are all correct with no guards at all.

I will rework the PR that way unless you would rather the check live somewhere else.

[groeneai]

Following up on the root cause above, with a design question I would rather ask than guess at.

Rejecting this at DDL time splits into two designs, and the split is structural rather than a matter of taste. A sharding key id IN some_table is one AST node either way: EXPLAIN AST gives Function in -> Identifier for both a Set table and an ordinary MergeTree table. Only the catalog separates them, and they behave differently today:

CREATE TABLE ms (id UInt64) ENGINE = Set;      -- INSERT INTO dist ... works
CREATE TABLE loc (id UInt64) ENGINE = MergeTree ORDER BY id;  -- INSERT aborts, Not-ready Set

So:

A. Syntactic check. Reuse checkExpressionDoesntContainSubqueries (extractKeyExpressionList.cpp), which already guards ORDER BY, PARTITION BY, INDEX and TTL, and apply it unconditionally as those call sites do. One line, replica-stable, no gating. Cost: it also rejects a Set-table key, which works today, and existing tables with such a key stop loading.

B. Exact check. Reject only when getPreparedSets()->hasSubqueries() after the key expression is built, gated on mode <= CREATE like the sanity check at StorageDistributed.cpp:501. Cost: the classification reads the local catalog, so it is an initiator-only guarantee, and a full-definition ATTACH still slips through. Closing that needs attach_short_syntax, which ITableFunction::execute does not carry, so remote() would need provenance plumbed through it.

B keeps Set-table keys and legacy tables working; A is smaller and replica-stable. I lean B, because 01527_dist_sharding_key_dictGet_reload shows a mutable sharding key is intended behaviour, so A's over-rejection would remove something supported, and because A makes an existing table unloadable rather than merely broken on INSERT.

Which would you prefer? I will also fold in the DistributedSink INSERT path, which the current patch misses entirely.