Skip to content

25.8: Fix Nullable(X)->Y cross-type ALTER MODIFY COLUMN (follow-up to #84770)#1612

Open
il9ue wants to merge 1 commit intoreleases/25.8.16from
fix/nullable-cross-type-modify-25.8
Open

25.8: Fix Nullable(X)->Y cross-type ALTER MODIFY COLUMN (follow-up to #84770)#1612
il9ue wants to merge 1 commit intoreleases/25.8.16from
fix/nullable-cross-type-modify-25.8

Conversation

@il9ue
Copy link
Copy Markdown

@il9ue il9ue commented Apr 1, 2026

Changelog category (leave one):

  • Bug Fix

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

Fix ALTER MODIFY COLUMN failure when converting nullable columns to a different non-nullable type (e.g. Nullable(UInt8) to String). Previously, cross-type nullable conversions would fail with NO_COMMON_TYPE error. Follow-up to upstream ClickHouse#84770.

CI/CD Options

Exclude tests:

  • Fast test
  • Integration Tests
  • Stateless tests
  • Stateful tests
  • Performance tests
  • All with ASAN
  • All with TSAN
  • All with MSAN
  • All with UBSAN
  • All with Coverage
  • All with Aarch64
  • All Regression
  • Disable CI Cache

Regression jobs to run:

  • Fast suites (mostly <1h)
  • Aggregate Functions (2h)
  • Alter (1.5h)
  • Benchmark (30m)
  • ClickHouse Keeper (1h)
  • Iceberg (2h)
  • LDAP (1h)
  • Parquet (1.5h)
  • RBAC (1.5h)
  • SSL Server (1h)
  • S3 (2h)
  • Tiered Storage (2h)

@il9ue
Copy link
Copy Markdown
Author

il9ue commented Apr 1, 2026

Context

This is a follow-up fix for the backport of upstream ClickHouse/ClickHouse#84770 ("Handle NULLs in ALTER MODIFY COLUMN"), which was backported to Altinity 24.8 via #1370 and to 25.8 via #1344.

Problem

PR ClickHouse#84770 correctly handles Nullable(X) → X conversions (same base type), but fails for Nullable(X) → Y where X != Y (cross-type conversion).

Reproducer:

CREATE TABLE XXX (x Nullable(UInt8), y String) ENGINE=MergeTree ORDER BY tuple();
INSERT INTO XXX (y) VALUES (1);
ALTER TABLE XXX MODIFY COLUMN x String DEFAULT '';
-- FAILS: NO_COMMON_TYPE error

This was reported by a customer on Altinity Stable 24.8.14. The same failure is reproducible on upstream v26.1.6.6 (ClickHouse Playground), while v26.2.5.45 succeeds.

Root Cause

convertRequiredExpressions() in inplaceBlockConversions.cpp generates:

_CAST(ifNull(column, default_value), 'TargetType')

When converting Nullable(UInt8) → String, ifNull(Nullable(UInt8), String_default) internally calls getLeastSupertype({UInt8, String}), which throws NO_COMMON_TYPE because UInt8 and String have no common supertype.

Why cherry-picking the upstream fix (PR ClickHouse#96790) is not sufficient

Upstream resolved this in ClickHouse/ClickHouse#96790 by modifying ifNull.cpp to use getLeastSupertypeOrVariant() when use_variant_as_common_type = true. In v26.2+, this setting defaults to true, so the ifNull call succeeds by returning Variant(UInt8, String).

However, on Altinity 24.8 and 25.8, use_variant_as_common_type defaults to false. Cherry-picking ClickHouse#96790 alone would NOT fix the bug on these branches — the code path would still call getLeastSupertype() and throw.

Fix

Instead of modifying ifNull behavior, this PR fixes the expression generation in convertRequiredExpressions() directly.

Before (broken for cross-type):

_CAST(ifNull(col, default), 'String')

After (works for all type combinations):

ifNull(_CAST(col, 'Nullable(String)'), _CAST(default, 'String'))

This works because:

  1. _CAST(Nullable(UInt8), 'Nullable(String)')Nullable(String) — standard type cast, always succeeds
  2. ifNull(Nullable(String), String)String — trivial, same base types
  3. No dependency on use_variant_as_common_type or any other settings
  4. Same-type conversions (Nullable(X) → X) continue to work correctly

Scope

  • Files changed: src/Interpreters/inplaceBlockConversions.cpp (~10 lines in convertRequiredExpressions())
  • Tests added: Cross-type test case in 03575_modify_column_null_to_default.sql
  • Risk: Low — only affects the nullable-to-non-nullable code path when base types differ

Branches

This fix applies to both Altinity branches that have PR ClickHouse#84770:

  • 24.8.14separate PR (cherry-pick of same commit)
  • 25.8.16 ← this PR

Altinity 25.3 does not have ClickHouse#84770 and is not affected.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 1, 2026

Workflow [PR], commit [c613cbf]

PR ClickHouse#84770 generates _CAST(ifNull(col, default), TargetType).
When source and target base types differ (e.g. Nullable(UInt8) -> String),
ifNull throws NO_COMMON_TYPE because getLeastSupertype({UInt8, String}) fails.

Fix: swap to ifNull(_CAST(col, Nullable(TargetType)), _CAST(default, TargetType)).

Ref: ClickHouse#84770, ClickHouse#5985
Signed-off-by: Daniel Q. Kim <daniel.kim@altinity.com>
@il9ue il9ue force-pushed the fix/nullable-cross-type-modify-25.8 branch from fd1a650 to c613cbf Compare April 2, 2026 08:32
@il9ue
Copy link
Copy Markdown
Author

il9ue commented Apr 2, 2026

Updated: moved test to a separate file to fix CI failures

The -- { echoOff } directive didn't suppress SQL echo as expected because the original test (03575_modify_column_null_to_default.sql) runs in -- { echoOn } mode throughout. Appended test lines were being echoed to stdout, causing a mismatch with the .reference file.

What changed in this amend:

  • Removed our additions from the existing 03575_modify_column_null_to_default.sql / .reference (restored to original)
  • Created a new dedicated test file: 03575_modify_column_null_to_default_cross_type.sql / .reference
  • Added no-random-settings, no-random-merge-tree-settings tags to avoid randomized settings interfering with the test
  • The C++ fix in inplaceBlockConversions.cpp is unchanged

Other CI failures are pre-existing and unrelated to this PR:

  • 02477_single_value_data_string_regression — memory limit exceeded (flaky, environment-dependent)
  • RabbitMQ integration tests — all failing (infrastructure issue)
  • Integration tests timeout / Docker image errors — CI runner issues
  • Grype scan — CVE scan finding pre-existing vulnerabilities in base image
  • DCO sign-off — now fixed with git commit --amend -s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant