iframe-proxy

alexey-milovidov · 2026-03-30T02:28:25Z

When count_distinct_implementation is set to a nonsensical value like 'null' and aggregate_functions_null_for_empty is enabled, rewriteAggregateFunctionNameIfNeeded in the new analyzer dereferences the result of tryGetProperties without checking if the optional has a value. Since the function name is unknown, tryGetProperties returns std::nullopt, and the subsequent properties->returns_default_when_only_null triggers a libc++ hardening assertion (optional operator-> on disengaged value), aborting the server.

The old analyzer path in TreeRewriter.cpp already had the correct properties && guard. This fix aligns the new analyzer with that pattern.

Found by BuzzHouse fuzzer: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=101118&sha=8f61fec0aa6f37072934afaeb78da447750657a8&name_0=PR&name_1=BuzzHouse%20%28amd_msan%29
#101118

Changelog category (leave one):

Critical Bug Fix (crash, data loss, RBAC)

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

Fix server crash when count_distinct_implementation is set to an unknown function name and aggregate_functions_null_for_empty is enabled.

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

…unction name The `rewriteAggregateFunctionNameIfNeeded` in the new analyzer was dereferencing the result of `tryGetProperties` without checking if the optional has a value. When `count_distinct_implementation` is set to a nonsensical value like `'null'`, `tryGetProperties` returns `std::nullopt`, and the subsequent `properties->returns_default_when_only_null` triggers a libc++ hardening assertion (optional operator-> on disengaged value). The old analyzer path in `TreeRewriter.cpp` already had the correct `properties &&` guard. This fix aligns the new analyzer with that pattern. Found by BuzzHouse fuzzer. https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=101118&sha=8f61fec0aa6f37072934afaeb78da447750657a8&name_0=PR&name_1=BuzzHouse%20%28amd_msan%29 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

clickhouse-gh · 2026-03-30T02:29:04Z

Workflow [PR], commit [876bc8e]

Summary: ⏳

job_name	test_name	status	info	comment
Bugfix validation (functional tests)		failure
	04070_count_distinct_implementation_invalid	FAIL	cidb

AI Review

Summary

This PR adds a new stateless regression test (04070_count_distinct_implementation_invalid) for the count_distinct_implementation + aggregate_functions_null_for_empty path. I found two high-signal issues: the test does not validate the claimed abort scenario in bugfix validation, and the PR description/title claim a code fix while the diff contains only test files. Because of that mismatch and insufficient bugfix proof, this should not be merged as-is.

Findings

❌ Blockers

[tests/queries/0_stateless/04070_count_distinct_implementation_invalid.sql:3] The test expectation (UNKNOWN_AGGREGATE_FUNCTION/UNKNOWN_FUNCTION) can pass even when there is no server abort. CI confirms this: 04070_count_distinct_implementation_invalid runs [ OK ] but Bugfix validation (functional tests) still fails with “Failed to reproduce the bug”. This means the regression test does not demonstrate base-fails / PR-passes behavior for the claimed abort fix.
- Suggested fix: make the reproducer assert process-level failure on baseline (and no failure on fixed build), or use a query/setting combination that deterministically triggers the abort in the unfixed version.
[tests/queries/0_stateless/04070_count_distinct_implementation_invalid.sql:1] PR metadata claims a source-code fix in rewriteAggregateFunctionNameIfNeeded (missing properties && guard), but the PR diff contains only test additions and no analyzer source changes. If this PR is intended to fix the bug, it is incomplete.
- Suggested fix: include the actual analyzer code change in the same PR, or update title/description/changelog to clearly state this PR is test-only and link the PR/commit that contains the runtime fix.

Tests

⚠️ Add a bugfix-validation-capable regression that fails on an unfixed build and passes with the fix. Current test is green in regular execution and does not prove the abort scenario.

ClickHouse Rules

Item	Status	Notes
Deletion logging	➖
Serialization versioning	➖
Core-area scrutiny	✅
No test removal	✅
Experimental gate	➖
No magic constants	✅
Backward compatibility	✅
`SettingsChangesHistory.cpp`	➖
PR metadata quality	❌	Title/description/changelog imply a runtime fix, but diff is test-only.
Safe rollout	⚠️	Bugfix cannot be safely rolled out if the runtime fix is missing or not verifiable by bugfix validation.
Compilation time	✅

Final Verdict

Status: ❌ Block

Minimum required actions:

Include the actual analyzer fix (properties && guard in the relevant rewriteAggregateFunctionNameIfNeeded path) in this PR, or explicitly scope this PR to test-only and point to the fixing commit/PR.
Update the regression test so Bugfix validation can reproduce the bug on baseline and verify the fix on this PR.

…ctions_null_for_empty Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

alexey-milovidov · 2026-03-30T02:30:06Z

@@ -0,0 +1,3 @@
+-- Regression test: setting count_distinct_implementation to an unknown function
+-- name with aggregate_functions_null_for_empty caused a crash (optional deref on nullopt).
+SELECT countDistinct(1) SETTINGS count_distinct_implementation = 'null', aggregate_functions_null_for_empty = 1; -- { serverError UNKNOWN_AGGREGATE_FUNCTION }


The test is good - it reproduces the issue.

clickhouse-gh · 2026-03-30T06:33:36Z

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	84.20%	84.20%	+0.00%
Functions	90.90%	90.90%	+0.00%
Branches	76.80%	76.70%	-0.10%

Changed lines: 100.00% (6/6) · Uncovered code

Full report · Diff report

…in-rewriteAggregateFunctionName

The test `04068_count_distinct_implementation_invalid` conflicts with `04068_constant_fold_union_intersect` added on master. Renumbered to 04070. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…in-rewriteAggregateFunctionName

…in-rewriteAggregateFunctionName # Conflicts: # src/Analyzer/Resolve/QueryAnalyzer.cpp

…tionName

alexey-milovidov · 2026-04-07T00:28:05Z

The Stress test (arm_msan) failure is fixed by #101239, which should be merged first. After it is merged, please update the branch to include the fix.

…tionName

clickhouse-gh · 2026-04-07T01:35:54Z

@@ -0,0 +1,3 @@
+-- Regression test: setting count_distinct_implementation to an unknown function
+-- name with aggregate_functions_null_for_empty caused a crash (optional deref on nullopt).
+SELECT countDistinct(1) SETTINGS count_distinct_implementation = 'null', aggregate_functions_null_for_empty = 1; -- { serverError UNKNOWN_AGGREGATE_FUNCTION, UNKNOWN_FUNCTION }


The regression test currently does not prove the claimed failure mode (server abort). In CI, Bugfix validation (functional tests) reports "Failed to reproduce the bug" while this query itself is [ OK ], which means both baseline and PR build accept this expectation.

Because the query is expected to return UNKNOWN_AGGREGATE_FUNCTION/UNKNOWN_FUNCTION, this test can pass even when there is no abort. Please make the test detect process-level failure (or use a reproducer that fails on base and passes on the fix), so bugfix validation can actually validate the fix.

clickhouse-gh · 2026-04-07T01:36:06Z

@@ -0,0 +1,3 @@
+-- Regression test: setting count_distinct_implementation to an unknown function


PR title/description says this change fixes an abort in rewriteAggregateFunctionNameIfNeeded, but this PR diff only adds a regression test and does not modify the analyzer code path itself. If the fix is intended to be in this PR, please include the actual source change (the missing properties && guard) so the bug is resolved, not only tested.

Algunenano · 2026-04-07T08:36:56Z

clickhouse-gh Bot added pr-critical-bugfix pr-must-backport Pull request should be backported intentionally. Use this label with great care! labels Mar 30, 2026

Add test for invalid count_distinct_implementation with aggregate_fun…

4cf9323

…ctions_null_for_empty Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

alexey-milovidov commented Mar 30, 2026

View reviewed changes

Algunenano self-assigned this Mar 30, 2026

Support the error from the old analyzer

0290034

Algunenano approved these changes Mar 30, 2026

View reviewed changes

alexey-milovidov and others added 7 commits March 30, 2026 08:12

Merge remote-tracking branch 'origin/master' into fix-optional-deref-…

6ff8a9f

…in-rewriteAggregateFunctionName

Merge master and renumber test to resolve number conflict

0996d45

The test `04068_count_distinct_implementation_invalid` conflicts with `04068_constant_fold_union_intersect` added on master. Renumbered to 04070. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Merge remote-tracking branch 'origin/master' into fix-optional-deref-…

9e759d2

…in-rewriteAggregateFunctionName

Merge remote-tracking branch 'origin/master' into fix-optional-deref-…

c96f99f

…in-rewriteAggregateFunctionName

Merge remote-tracking branch 'origin/master' into fix-optional-deref-…

62f7035

…in-rewriteAggregateFunctionName

Merge remote-tracking branch 'origin/master' into fix-optional-deref-…

e2db9f1

…in-rewriteAggregateFunctionName # Conflicts: # src/Analyzer/Resolve/QueryAnalyzer.cpp

Merge branch 'master' into fix-optional-deref-in-rewriteAggregateFunc…

8357ef7

…tionName

Merge branch 'master' into fix-optional-deref-in-rewriteAggregateFunc…

876bc8e

…tionName

clickhouse-gh Bot reviewed Apr 7, 2026

View reviewed changes

Algunenano closed this Apr 7, 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 crash when count_distinct_implementation is set to an unknown function name#101152

Fix crash when count_distinct_implementation is set to an unknown function name#101152
alexey-milovidov wants to merge 11 commits into
masterfrom
fix-optional-deref-in-rewriteAggregateFunctionName

alexey-milovidov commented Mar 30, 2026 •

edited

Loading

Uh oh!

clickhouse-gh Bot commented Mar 30, 2026 •

edited

Loading

Uh oh!

alexey-milovidov Mar 30, 2026

Uh oh!

clickhouse-gh Bot commented Mar 30, 2026

Uh oh!

alexey-milovidov commented Apr 7, 2026

Uh oh!

clickhouse-gh Bot Apr 7, 2026

Uh oh!

clickhouse-gh Bot Apr 7, 2026

Uh oh!

Algunenano commented Apr 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -0,0 +1,3 @@
		-- Regression test: setting count_distinct_implementation to an unknown function

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

alexey-milovidov commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

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

Documentation entry for user-facing changes

Uh oh!

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

Uh oh!

AI Review

Summary

Findings

Tests

ClickHouse Rules

Final Verdict

Uh oh!

alexey-milovidov Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot commented Mar 30, 2026

LLVM Coverage Report

Uh oh!

alexey-milovidov commented Apr 7, 2026

Uh oh!

clickhouse-gh Bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

Algunenano commented Apr 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alexey-milovidov commented Mar 30, 2026 •

edited

Loading

clickhouse-gh Bot commented Mar 30, 2026 •

edited

Loading