24.8.14 Backport of #84770: Handle NULLs in ALTER MODIFY COLUMN#1370
Merged
zvonand merged 2 commits intocustomizations/24.8.14from Feb 7, 2026
Merged
24.8.14 Backport of #84770: Handle NULLs in ALTER MODIFY COLUMN#1370zvonand merged 2 commits intocustomizations/24.8.14from
zvonand merged 2 commits intocustomizations/24.8.14from
Conversation
…umn_null_to_default Handle NULLs in ALTER MODIFY COLUMN
Collaborator
|
This is an automated comment for commit c8d0661 with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page
Successful checks
|
Collaborator
Author
|
Code from upstream was merged clearly. The PR contains a test that passes. |
Collaborator
PR #1370 Audit ReviewAI audit note: This review was generated by AI (gpt-5.3-codex). Audit update for PR #1370 (24.8.14 Backport of ClickHouse#84770: Handle NULLs in ALTER MODIFY COLUMN) Confirmed defectsMedium: Shared default-expression AST is reused without cloning in conversion pipeline
/// Converting a column from nullable to non-nullable may cause 'Cannot convert column' error when NULL values exist.
/// Users should specify DEFAULT expression in ALTER MODIFY COLUMN statement to replace NULL values.
if (isNullableOrLowCardinalityNullable(column_in_block.type) && !isNullableOrLowCardinalityNullable(required_column.type))
{
ASTPtr default_value;
if (auto it = column_defaults.find(required_column.name); it != column_defaults.end())
default_value = it->second.expression;
...
auto convert_func = makeASTFunction("_CAST",
makeASTFunction("ifNull", std::make_shared<ASTIdentifier>(required_column.name), default_value),
std::make_shared<ASTLiteral>(required_column.type->getName())); if (column_default)
{
/// expressions must be cloned to prevent modification by the ExpressionAnalyzer
auto column_default_expr = column_default->expression->clone();Coverage summary
|
Collaborator
PR #1370 CI TriageSummary
Verdict: PASS — No failures caused by this PR. The CI report explicitly confirms: New Fails in PR: 0, Regression New Fails: 0, Checks New Fails: 0 (compared with base sha Detailed AnalysisPre-existing Flaky Tests (Unrelated)
This is a known flaky test related to parallel quorum insert timing. It failed on the first run and passed on the rerun, confirming it is not related to this PR's changes. Infrastructure Issues (Unrelated)Sign aarch64 / Sign release — Persistent CI Infrastructure Failure
Integration Tests Timeout — Resolved on Rerun
Recommendations
|
Collaborator
@zvonand can you please check the audit review, if it makes sense to raise an issue for this? |
Collaborator
Author
|
This is only a backport, almost 1:1 matches upstream's code In backports, we take the upstream PRs as is |
Collaborator
|
@zvonand can we at least just make sure there is nothing critical in the audit review? I can mark it as verified afterwards. |
Collaborator
Author
|
I do not see anything critical. Again, this is only a backport, we take it as is. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
ALTER MODIFY COLUMN now requires explicit DEFAULT when converting nullable columns to non-nullable types. Previously such ALTERs could get stuck with cannot convert null to not null errors, now NULLs are replaced with column's default expression. (ClickHouse#84770 by @vdimir)
CI/CD Options
Exclude tests:
Regression jobs to run: