Skip to content

Fix defaultImplementationForNulls returning wrong constness at 0 rows#100293

Merged
alexey-milovidov merged 6 commits intomasterfrom
fix-totals-having-const-mismatch-v2
Mar 24, 2026
Merged

Fix defaultImplementationForNulls returning wrong constness at 0 rows#100293
alexey-milovidov merged 6 commits intomasterfrom
fix-totals-having-const-mismatch-v2

Conversation

@alexey-milovidov
Copy link
Copy Markdown
Member

@alexey-milovidov alexey-milovidov commented Mar 21, 2026

Closes #100289.

defaultImplementationForNulls had an early return at input_rows_count == 0 that always produced a non-const column, even when all arguments were constant. This caused ActionsDAG::updateHeader (which evaluates on 0-row headers) to produce non-const results for functions like concat that use the default null handling, while functions like divide (which handle nulls themselves) correctly produced const results.

The inconsistency manifested when two UNION DISTINCT branches used different functions — one branch's output column was const while the other's was not, causing "Block structure mismatch" or "non constant in source stream but must be constant in result" errors.

Two fixes:

  1. defaultImplementationForNulls (IFunction.cpp): move the 0-row early return after the all_columns_constant analysis, so functions with all-const arguments correctly produce const results at 0 rows.
  2. TotalsHavingTransform: use updateHeader (same as the main output port) instead of ExpressionActions::execute for computing the totals port header, ensuring both ports have consistent column constness.

Found by AST fuzzer: https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=1ca65921c124da4ad3595798f0a72ef47706ecd6&name_0=MasterCI&name_1=AST%20fuzzer%20%28amd_debug%29

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 "Block structure mismatch" exception when using GROUP BY ... WITH TOTALS HAVING combined with UNION DISTINCT and nullable expressions.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

`defaultImplementationForNulls` had an early return at `input_rows_count == 0`
that always produced a non-const column, even when all arguments were constant.
This caused `ActionsDAG::updateHeader` (which evaluates on 0-row headers) to
produce non-const results for functions like `concat` that use the default null
handling, while functions like `divide` (which handle nulls themselves) correctly
produced const results.

The inconsistency manifested when two UNION DISTINCT branches used different
functions — one branch's output column was const while the other's was not,
causing "Block structure mismatch" or "non constant in source stream but must
be constant in result" errors.

Fix: move the 0-row check after the `all_columns_constant` analysis so that
functions with all-const arguments correctly produce const results at 0 rows.

Also fix `TotalsHavingTransform` to use `updateHeader` (same as the main output
port) instead of `ExpressionActions::execute` for computing the totals port
header, ensuring both ports have consistent column constness.

Found by AST fuzzer: https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=1ca65921c124da4ad3595798f0a72ef47706ecd6&name_0=MasterCI&name_1=AST%20fuzzer%20%28amd_debug%29

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Mar 21, 2026

Workflow [PR], commit [040b5e3]

Summary:


AI Review

Summary

This PR fixes a real constness mismatch on 0-row headers between ActionsDAG::updateHeader and ExpressionActions::execute, which could surface as Block structure mismatch / non constant ... must be constant exceptions in UNION DISTINCT with WITH TOTALS HAVING. The IFunction::defaultImplementationForNulls fix is correct and targeted, and the TotalsHavingTransform header-path alignment is consistent with main-port behavior. Added regression coverage is relevant. No blocker or major correctness issues found in the patch.

Missing context

  • ⚠️ Full CI is still in progress, so this review is code-based plus available metadata only.

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
Safe rollout
Compilation time

Final Verdict

  • Status: ✅ Approve

@clickhouse-gh clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Mar 21, 2026
@alexey-milovidov alexey-milovidov marked this pull request as ready for review March 21, 2026 17:59
-- the Union step's converting actions would fail with "Block structure mismatch" or
-- "non constant in source stream but must be constant in result".

DROP TABLE IF EXISTS t_totals_const;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is good, it reproduces the issue.

alexey-milovidov and others added 5 commits March 21, 2026 20:14
The `transform` function casts WHEN values to the expression type. When
WHEN values are Nullable (e.g. `CAST(NULL AS Nullable(Int32))`) but the
expression is non-Nullable, the cast fails with "Cannot convert NULL
value to non-Nullable type".

This became visible after the `defaultImplementationForNulls` change in
this branch made `updateHeader` actually execute functions at 0 rows
with all-constant args, triggering the cast path in `transform`.

Fix by checking upfront whether `transform` can handle the types: use
`tryGetLeastSupertype` and a Nullable compatibility check to determine
applicability, rather than failing during execution. When `transform`
cannot handle the types, fall through to the `multiIf` path which
handles Nullable WHEN values correctly via `caseWhenEquals`.

https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=100293&sha=f067c3bec9fe4a7a07e810a27c9587ea9f777bfb&name_0=PR&name_1=SQLLogic%20test

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Mar 24, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 83.90% 83.90% +0.00%
Functions 24.70% 24.60% -0.10%
Branches 76.50% 76.50% +0.00%

PR changed lines: PR changed-lines coverage: 98.57% (69/70, 0 noise lines excluded)
Diff coverage report
Uncovered code

if (e.code() != ErrorCodes::NOT_IMPLEMENTED)
throw;
FunctionBasePtr function_base;
try
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WTF

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, as it fixed the bug, let it live.

@alexey-milovidov alexey-milovidov self-assigned this Mar 24, 2026
@alexey-milovidov alexey-milovidov merged commit 7c42d3b into master Mar 24, 2026
151 of 152 checks passed
@alexey-milovidov alexey-milovidov deleted the fix-totals-having-const-mismatch-v2 branch March 24, 2026 14:12
@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Mar 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-bugfix Pull request with bugfix, not backported by default pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants