iframe-proxy

RenzoMXD · 2026-03-18T13:05:56Z

Implement the SQL standard UNIQUE predicate that returns true when all rows in a subquery are distinct and false when duplicates exist. Empty subqueries return true (vacuously unique).

The implementation follows the same pattern as EXISTS:

Parser: UniqueLayer class parses UNIQUE(subquery) syntax
Analyzer: rewrites to SELECT count() = uniqExact(*) FROM (subquery)
Stub function: FunctionUnique for correlated subquery support

Closes #99609

Changelog category (leave one):

New Feature

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

Added UNIQUE(subquery) predicate that returns true when all rows in the subquery are distinct. It behaves according to the SQL standard: duplicates yield false, empty subqueries yield true. Closes #99609

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

clickhouse-gh · 2026-03-18T13:12:10Z

Workflow [PR], commit [5fc2308]

Summary: ❌

Performance Comparison: Performance dashboard

job_name	test_name	status	info
Fast test (arm_darwin)		FAIL
	04319_skip_unavailable_shards_mode_table_missing	FAIL	cidb
AST fuzzer (amd_debug, targeted)		FAIL
	Logical error: Block structure mismatch in A stream: different columns: (STID: 0993-27f0)	FAIL	cidb, issue
AST fuzzer (amd_debug)		FAIL
	Logical error: 'Inconsistent AST formatting: the original AST: (STID: 1941-26fa)	FAIL	cidb, issue

AI Review

Summary

This PR adds the SQL UNIQUE(subquery) predicate by parsing it to __unique and rewriting it in the analyzer to a scalar count() = uniqExact(*) subquery, with coverage for duplicate projected names, nested-expression planning, old-analyzer rejection, and only_analyze non-crashing paths. The main remaining issue is in that last area: the new only_analyze placeholder uses a hardcoded boolean value as the projection/header name, so distributed/sample-block consumers can see a different column name from the real execution path for the same query.

Findings

❌ Blockers

src/Analyzer/Resolve/resolveFunction.cpp:416 only_analyze returns the placeholder value string as the projection name (0), while the execute path returns 0 or 1 from the real UNIQUE result. resolveProjectionExpressionNodeList uses that returned name directly in the sample block header, so distributed shard header construction can analyze the same expression as 0 UInt8 on the initiator but execute it as 1 UInt8 on the shard when the subquery is unique, causing a block-structure mismatch.
Suggested fix: use one stable projection/action name for both paths instead of naming the result by the placeholder/real boolean value, and add a distributed or remote regression that compares analyze-time and execution-time headers.

Tests

⚠️ The current 04498_unique_predicate_only_analyze regression only proves that local CREATE VIEW / EXPLAIN paths stop throwing. It does not cover the distributed-shard-header consumer mentioned in the code comment, so it would not catch the header-name divergence above. A focused remote() / Distributed regression is needed.

Final Verdict

Status: ⚠️ Request changes
Minimum required actions: make the UNIQUE projection/header name stable between only_analyze and execute paths, and add a regression that exercises distributed/sample-block header construction for this predicate.

RenzoMXD · 2026-03-18T21:49:21Z

@alexey-milovidov Please review my PR again. Thanks.

amosbird · 2026-03-19T04:38:03Z

The current rewrite SELECT count() = uniqExact(*) FROM (subquery) is heavier than necessary — it must scan all data even if a duplicate is found on the 2nd row, and runs two aggregate functions simultaneously.

The key property of UNIQUE is that non-unique can terminate immediately. We should exploit this.

Proposed approach

Create an internal aggregate function __hasNoDuplicates(*) that maintains a hash set. On each add, it inserts the row hash — if the key already exists (duplicate), it sets a flag and short-circuits.

The critical part: reuse the existing Aggregator::executeOnBlock BREAK path to cancel I/O.

Today, max_rows_to_group_by with group_by_overflow_mode = 'break' causes Aggregator::executeOnBlock to return false. AggregatingTransform then sets is_consume_finished = true and closes its input port, cascading upstream through the pipeline to stop storage reads. This path is stable and well-tested.

Add two virtual methods to IAggregateFunction:

virtual bool isEarlyTerminable() const { return false; }
virtual bool shouldTerminateEarly(AggregateDataPtr place) const { return false; }

After Aggregator::executeOnBlock processes a block, check whether any aggregate requests early termination — if so, return false, taking the exact same code path as BREAK. No new processors or transforms needed.

Analyzer rewrite becomes simply:

UNIQUE(subquery) → SELECT __hasNoDuplicates(*) FROM (subquery) WHERE <null filter>

Benefits

	Current (`count() = uniqExact(*)`)	Proposed (`__hasNoDuplicates`)
Best case (dup on row 2)	O(n) time + full I/O	O(1), stops after 1 block
Worst case (all unique)	O(n), 2 aggregates	O(n), 1 aggregate
I/O cancellation	None	Block-level via existing BREAK path
New processors	None	None
`IAggregateFunction` changes	None	2 virtual methods (default `false`, backward-compatible)

Implementation scope

IAggregateFunction: 2 new virtual methods
Aggregator::executeOnBlock: ~5 lines to check early termination after block processing
AggregateFunctionHasNoDuplicates: ~80 lines (hash set + early termination interface)
resolveFunction.cpp: simpler rewrite than current (1 aggregate instead of 2)

The early termination interface is generic and reusable by future aggregate functions.

RenzoMXD · 2026-03-19T04:46:50Z

@amosbird Thanks very much. I'll fix based on your suggestions.

RenzoMXD · 2026-03-23T16:53:46Z

@alexey-milovidov Can you take another look on my PR please? Thanks.

alexey-milovidov · 2026-03-23T17:11:26Z

I would like to find a better name for this aggregate function, so it will be usable in normal scenarios.

For example, allUnique

- Rename test from 04054_unique_predicate to 04147_unique_predicate to avoid the 04054_* prefix collision with sixteen unrelated tests. - Preserve the original expression as the source AST when constructing the result ConstantNode in non-only_analyze mode, mirroring how the EXISTS branch builds its constant. Without it, EXPLAIN output and error messages lose the original unique(...) shape. - In only_analyze mode, return projection name "unique" instead of the internal name "__unique", so EXPLAIN does not leak the dispatch tag. - Rewrite the fallback comment in the non-analyze path: it does not fire for empty subqueries (count() = uniqExact(*) yields TRUE there), it only defends against a bug where evaluateScalarSubqueryIfNeeded fails to produce a constant. Refs ClickHouse#99877

rschu1ze · 2026-05-04T20:50:13Z

@novikd Could you please continue the review / integration when you're available? Thanks.

Address review comment on ClickHouse#99877 (comment)... ("Falling back to `1` ... hides analyzer/execution inconsistencies and can silently return a wrong UNIQUE result"). After `evaluateScalarSubqueryIfNeeded`, the rewrite expects a non-NULL scalar constant. If that invariant is violated, fail loudly with `LOGICAL_ERROR` instead of defaulting to true.

RenzoMXD · 2026-05-07T11:31:16Z

@novikd Could you review my PR again please?

clickhouse-gh · 2026-06-01T15:27:10Z

Dear @novikd, you haven't been active on this PR for 30 days. You will be unassigned. Will you continue working on it? If so, please feel free to reassign yourself.

…dicate # Conflicts: # src/Analyzer/Resolve/resolveFunction.cpp

The UNIQUE predicate is parsed into the internal function `__unique`, which is only rewritten by the analyzer in `QueryAnalyzer::resolveUniquePredicate`. The old analyzer (`enable_analyzer = 0`) has no handling for it, so a query using UNIQUE previously failed with an obscure `Unknown function __unique` exception. Reject it explicitly in `TreeRewriter` (used only by the old analyzer) with a clear `NOT_IMPLEMENTED` message pointing to `enable_analyzer = 1`, mirroring how QUALIFY and WITH RECURSIVE are handled. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

`ConstantValue` now requires a `ColumnConstPtr`, so constructing it from a mutable `ColumnUInt8` no longer compiles (`no matching constructor for initialization of boost::intrusive_ptr<const DB::ColumnConst>`). Wrap the result column with `ColumnConst::create`, matching the `EXISTS` rewrite in the same file. Also make the per-column `isNotNull` filter address columns by position instead of by name. The previous rewrite referenced the subquery's projected columns by name, which can bind several filters to the same column when a subquery exposes duplicate column names (e.g. `SELECT t.x, s.x FROM t, s`). The subquery is now wrapped once as `SELECT * FROM (subquery)`, every projected column is renamed to a unique synthetic name, and the NULL filter is built over those names. The matcher-expanded projection nodes keep their position-correct column sources, so only the output names change. This also removes the separate probe clone, resolving the subquery only once. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…y-predicate-work

…names Covers the position-safe NULL filter: subqueries that expose duplicate or qualified column names (via cross joins, repeated columns, or `join_use_nulls`) must still drop rows that contain a NULL in any projected column and compare rows by every projected position. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

alexey-milovidov · 2026-06-20T11:56:28Z

Pushed three commits (fast-forward, no force-push):

Fix build error — ConstantValue now requires a ColumnConstPtr, so the rewrite no longer compiled (no matching constructor for boost::intrusive_ptr<const DB::ColumnConst> at resolveFunction.cpp:393). Wrapped the result with ColumnConst::create, matching the EXISTS rewrite. This was the cause of the Fast test / Build (arm_tidy) failures.
Make the NULL filter position-safe (addresses the AI-review block) — the per-column isNotNull filter no longer references the subquery's columns by name. The subquery is wrapped once as SELECT * FROM (subquery), every projected column is renamed to a unique synthetic name, and the filter is built over those names, so it addresses columns by position. This also removes the separate probe clone (the subquery is resolved only once).
Regression test 04401_unique_predicate_duplicate_columns covering duplicate/qualified column names with NULLs (cross joins, repeated columns, join_use_nulls).

Also merged master (the branch was 1683 commits behind; clean merge, no conflicts).

Verification notes: the changed TU passes clang -fsyntax-only against the merged tree, and the produced rewrite was validated semantically against a master build for all of the test cases. A full local build was not possible in this environment, so CI is the final integration check.

…ession The `UNIQUE(subquery)` predicate folds to a constant in `resolveUniquePredicate`, but the resulting `ConstantNode` kept the internal `__unique(subquery)` function as its source expression. `PlannerActionsVisitor` short-circuits the action node name of a constant only when its source expression is a `QUERY`/`UNION` node; for any other source it recurses, reaching the raw non-correlated subquery `QueryNode` and throwing `Only correlated QueryNode can be used as action query tree node`. This happened whenever the predicate was nested inside another expression, for example `uniq(UNIQUE((SELECT ...)), b)` in `ORDER BY`. `EXISTS` avoids this only because it has a dedicated special case in `PlannerActionsVisitor`. Use the rewritten subquery `QueryNode` as the source expression instead of the `__unique` function, matching how scalar subqueries fold to a constant. The planner then names the constant by value and never recurses into the subquery. Found by the AST fuzzer (STID 0250-0df5), reproduced and verified locally. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

alexey-milovidov · 2026-06-26T04:57:34Z

Pushed one commit (fast-forward, no force-push) to fix the only PR-caused CI failure.

The AST fuzzer found a logical error (Only correlated QueryNode can be used as action query tree node, STID 0250-0df5) that triggers when the UNIQUE predicate is nested inside another expression, e.g. uniq(UNIQUE((SELECT ...)), b) in ORDER BY. The predicate folds to a constant in resolveUniquePredicate, but the resulting ConstantNode kept the internal __unique(subquery) function as its source expression. PlannerActionsVisitor only short-circuits the action-node name of a constant when its source is a QUERY/UNION node; otherwise it recurses into the source and reaches the raw non-correlated subquery QueryNode, throwing. (EXISTS escapes only because it has a dedicated special case there.)

Fix: use the rewritten subquery QueryNode as the source expression instead of the __unique function, matching how scalar subqueries fold to a constant, so the planner names the constant by value and never recurses into the subquery.

Verified locally on a full build: the exact fuzzer reproducer now returns a result, and 04147_unique_predicate plus a new 04410_unique_predicate_nested_in_expression regression test (nested in uniq in ORDER BY/SELECT, scalar contexts, duplicate columns, and the exact fuzzer query) both pass.

The remaining red jobs are master-wide flakes unrelated to this PR: the Azure base64.cpp ASan global-buffer-overflow, Hung check (#107941), Cannot start clickhouse-server, and Unexpected exception in refresh scheduling (STID 2508-34af).

The lambda argument name `__table1.` (a backquoted identifier ending in a dot) makes the targeted AST fuzzer mutate the test table into a column whose ALIAS default type mismatches, which forces error-message formatting to convert the lambda back to an AST. IdentifierNode::toASTImpl then builds an ASTIdentifier whose name parts include an empty trailing part (from splitting `__table1.` on the dot), tripping chassert(!part.empty()) in the ASTIdentifier constructor (pre-existing engine assertion, unrelated to this PR's fix; also seen on unrelated PRs ClickHouse#104436 and ClickHouse#99877). The `__table1.y` lambda-arg cases already cover the same code path in buildShardCollapseFanOut (an unquoted lambda-arg name that looks like a `__tableN.` qualifier but whose tail is not a real column, so its numbering must be left intact), without producing an empty identifier part. Keep those and drop the redundant trailing-dot variant so the test no longer surfaces the unrelated assertion under fuzzing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…y-predicate

In only_analyze mode (EXPLAIN, `CREATE VIEW` validation, distributed shard headers) `resolveUniquePredicate` left a raw non-correlated `QueryNode` in expression position. Consumers that build projection actions from the analyzed tree (for example `InterpreterSelectQueryAnalyzer::getSampleBlock`) then reach it through `PlannerActionsVisitor::visitQuery`, which rejects non-correlated query nodes with "Only correlated QueryNode can be used as an action query tree node". As a result a dry-run path such as `CREATE VIEW v AS SELECT UNIQUE(SELECT number FROM numbers(3))` failed even though the same `SELECT` executes normally. Produce a typed `UInt8` `ConstantNode` with the rewritten subquery as its source expression instead, exactly as the executed path does. Only the result type and structure matter in only_analyze mode; the value is a placeholder, as it is for any scalar subquery in only_analyze mode. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Covers the only_analyze `getSampleBlock` paths (`CREATE VIEW` validation and `EXPLAIN`), including the UNIQUE predicate nested inside another expression, which previously threw "Only correlated QueryNode can be used as an action query tree node". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

alexey-milovidov · 2026-07-03T09:21:23Z

Advanced this PR (pushed 5fc2308f836):

Merged master (as @rschu1ze requested — the branch was ~2 weeks behind). The merge was clean, no conflicts.
Addressed the unresolved review thread on resolveFunction.cpp (the only_analyze path). Previously resolveUniquePredicate left a raw non-correlated QueryNode in expression position in only_analyze mode, so consumers that build projection actions from the analyzed tree — e.g. InterpreterSelectQueryAnalyzer::getSampleBlock, used by CREATE VIEW validation and distributed shard headers — reached it through PlannerActionsVisitor::visitQuery and threw Only correlated QueryNode can be used as an action query tree node. So CREATE VIEW v AS SELECT UNIQUE(SELECT number FROM numbers(3)) failed even though the plain SELECT worked. The only_analyze result now folds to a typed UInt8 ConstantNode with the rewritten subquery as source, exactly as the executed path does. Added regression test 04498_unique_predicate_only_analyze.

CI failure triage (all three pre-merge reds are not caused by this feature's logic):

AST fuzzer — Logical error: Block structure mismatch in A stream from a fuzzer-mutated query UNIQUE(SELECT DISTINCT NULL FROM primes(...) GROUP BY 1, toUInt256(1) WITH ROLLUP). This is the known master-wide signature tracked in Logical error: Block structure mismatch in A stream: different columns: (STID: 0993-27f0) #108142 (STID 0993-27f0), which is still firing several times a day on master across ~140 unrelated PRs — a core WITH ROLLUP/UnionStep const-column bug, not specific to UNIQUE. I could not reproduce it on the merged code across ~20 ROLLUP/CUBE/GROUPING SETS/parallel/two-level variants. Related: Logical error: Block structure mismatch in A stream: different columns: (STID: 0993-27f0) #108142.
Stress test (amd_debug) — Logical error: Unexpected return type from equals (STID 1611-4523). The crashing query is a fuzzed 3-argument equals(t % 19, toNullable(toNullable('%KILL QUERY%')), 16) on 01786_explain_merge_tree — no UNIQUE involved. The same signature also appeared on unrelated PR Delete Iceberg table files on DROP with iceberg_delete_data_on_drop #109071. Unrelated to this PR.
Docker keeper image — clickhouse/clickhouse-keeper:head-arm64 push failed; the same infra failure hit 3 PRs the same day. Infra flake, unrelated.

CI will re-run on the merged commit.

clickhouse-gh · 2026-07-03T09:45:52Z

+        placeholder_column->getData().push_back(static_cast<UInt8>(0));
+        ConstantValue placeholder_value(ColumnConst::create(std::move(placeholder_column), 1), std::make_shared<DataTypeUInt8>());
+        auto placeholder_const_node = std::make_shared<ConstantNode>(std::move(placeholder_value), new_unique_subquery);
+        auto placeholder_projection_name = placeholder_const_node->getValueStringRepresentation();


only_analyze still needs a stable projection/header name here, not the placeholder value. resolveProjectionExpressionNodeList uses the returned ProjectionNames directly for the sample block header, so this branch always names the column 0, while the execute path below returns 0 or 1 from the real UNIQUE result.

That leaks into one of the consumers named in the comment: distributed shard header construction. A query like SELECT UNIQUE((SELECT number FROM numbers(3))) FROM remote(...) can analyze to header 0 UInt8 on the initiator, but execute to 1 UInt8 on the shard when the subquery is actually unique, which is a block-structure mismatch even though the type matches.

Please make the UNIQUE projection name stable across both paths (for example via a fixed synthetic name / __actionName-style wrapper) and add a regression that compares analyze-time and execution-time headers for a distributed or remote case.

clickhouse-gh · 2026-07-03T12:55:46Z

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered: 125/136 (91.91%) · Uncovered code

Full report · Diff report

alexey-milovidov added the can be tested Allows running workflows for external contributors label Mar 18, 2026

alexey-milovidov reviewed Mar 18, 2026

View reviewed changes

Comment thread src/Analyzer/Resolve/resolveFunction.cpp Outdated

clickhouse-gh Bot added the pr-feature Pull request with new product feature label Mar 18, 2026

RenzoMXD requested a review from alexey-milovidov March 18, 2026 13:50