diff --git a/src/Interpreters/inplaceBlockConversions.cpp b/src/Interpreters/inplaceBlockConversions.cpp index 291e7915dd4e..4f6d069e1036 100644 --- a/src/Interpreters/inplaceBlockConversions.cpp +++ b/src/Interpreters/inplaceBlockConversions.cpp @@ -21,13 +21,13 @@ #include #include - namespace DB { -namespace ErrorCode +namespace ErrorCodes { extern const int LOGICAL_ERROR; + extern const int BAD_ARGUMENTS; } namespace @@ -136,7 +136,7 @@ ASTPtr defaultRequiredExpressions(const Block & block, const NamesAndTypesList & return default_expr_list; } -ASTPtr convertRequiredExpressions(Block & block, const NamesAndTypesList & required_columns) +ASTPtr convertRequiredExpressions(Block & block, const NamesAndTypesList & required_columns, const ColumnDefaults & column_defaults, bool forbid_default_defaults) { ASTPtr conversion_expr_list = std::make_shared(); for (const auto & required_column : required_columns) @@ -148,6 +148,32 @@ ASTPtr convertRequiredExpressions(Block & block, const NamesAndTypesList & requi if (column_in_block.type->equals(*required_column.type)) continue; + /// 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)) + { + /// Before executing ALTER we explicitly check that user provided DEFAULT value to make it a conscious decision. + /// However, we may still need to use type's default value in some cases + /// (e.g. if a second ALTER removes the DEFAULT, but first is not completed). + ASTPtr default_value; + if (auto it = column_defaults.find(required_column.name); it != column_defaults.end()) + default_value = it->second.expression; + else if (!forbid_default_defaults) + default_value = std::make_shared(required_column.type->getDefault()); + else + throw Exception(ErrorCodes::BAD_ARGUMENTS, + "Cannot convert column '{}' from nullable type {} to non-nullable type {}. " + "Please specify `DEFAULT` expression in ALTER MODIFY COLUMN statement", + required_column.name, column_in_block.type->getName(), required_column.type->getName()); + + auto convert_func = makeASTFunction("_CAST", + makeASTFunction("ifNull", std::make_shared(required_column.name), default_value), + std::make_shared(required_column.type->getName())); + + conversion_expr_list->children.emplace_back(setAlias(convert_func, required_column.name)); + continue; + } + auto cast_func = makeASTFunction( "_CAST", std::make_shared(required_column.name), std::make_shared(required_column.type->getName())); @@ -175,9 +201,9 @@ std::optional createExpressions( } -void performRequiredConversions(Block & block, const NamesAndTypesList & required_columns, ContextPtr context) +void performRequiredConversions(Block & block, const NamesAndTypesList & required_columns, ContextPtr context, const ColumnDefaults & column_defaults, bool forbid_default_defaults) { - ASTPtr conversion_expr_list = convertRequiredExpressions(block, required_columns); + ASTPtr conversion_expr_list = convertRequiredExpressions(block, required_columns, column_defaults, forbid_default_defaults); if (conversion_expr_list->children.empty()) return; diff --git a/src/Interpreters/inplaceBlockConversions.h b/src/Interpreters/inplaceBlockConversions.h index 570eb75dd4a8..e7f1994b7848 100644 --- a/src/Interpreters/inplaceBlockConversions.h +++ b/src/Interpreters/inplaceBlockConversions.h @@ -3,6 +3,7 @@ #include #include #include +#include #include @@ -34,7 +35,8 @@ std::optional evaluateMissingDefaults( bool null_as_default = false); /// Tries to convert columns in block to required_columns -void performRequiredConversions(Block & block, const NamesAndTypesList & required_columns, ContextPtr context); +void performRequiredConversions(Block & block, const NamesAndTypesList & required_columns, ContextPtr context, + const ColumnDefaults & column_defaults, bool forbid_default_defaults = false); void fillMissingColumns( Columns & res_columns, diff --git a/src/Storages/AlterCommands.cpp b/src/Storages/AlterCommands.cpp index 14b6fd6134ab..3a586fccd8ee 100644 --- a/src/Storages/AlterCommands.cpp +++ b/src/Storages/AlterCommands.cpp @@ -1327,7 +1327,7 @@ void AlterCommands::apply(StorageInMemoryMetadata & metadata, ContextPtr context throw Exception(ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN, "Cannot ALTER column"); /// Check if new metadata is convertible from old metadata for projection. Block old_projection_block = projection.sample_block; - performRequiredConversions(old_projection_block, new_projection.sample_block.getNamesAndTypesList(), context); + performRequiredConversions(old_projection_block, new_projection.sample_block.getNamesAndTypesList(), context, metadata_copy.getColumns().getDefaults()); new_projections.add(std::move(new_projection)); } catch (Exception & exception) diff --git a/src/Storages/MergeTree/IMergeTreeReader.cpp b/src/Storages/MergeTree/IMergeTreeReader.cpp index c020cf7fa623..8ff7c6ca50b5 100644 --- a/src/Storages/MergeTree/IMergeTreeReader.cpp +++ b/src/Storages/MergeTree/IMergeTreeReader.cpp @@ -333,7 +333,7 @@ void IMergeTreeReader::performRequiredConversions(Columns & res_columns) const copy_block.insert({res_columns[pos], getColumnInPart(*name_and_type).type, name_and_type->name}); } - DB::performRequiredConversions(copy_block, requested_columns, data_part_info_for_read->getContext()); + DB::performRequiredConversions(copy_block, requested_columns, data_part_info_for_read->getContext(), storage_snapshot->metadata->getColumns().getDefaults()); /// Move columns from block. name_and_type = requested_columns.begin(); diff --git a/src/Storages/MergeTree/MergeTreeData.cpp b/src/Storages/MergeTree/MergeTreeData.cpp index d1fea772e267..3e9926239672 100644 --- a/src/Storages/MergeTree/MergeTreeData.cpp +++ b/src/Storages/MergeTree/MergeTreeData.cpp @@ -4349,7 +4349,7 @@ void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, Context if (!columns_to_check_conversion.empty()) { auto old_header = old_metadata.getSampleBlock(); - performRequiredConversions(old_header, columns_to_check_conversion, local_context); + performRequiredConversions(old_header, columns_to_check_conversion, local_context, new_metadata.getColumns().getDefaults(), true); } if (old_metadata.hasSettingsChanges()) diff --git a/tests/queries/0_stateless/02662_sparse_columns_mutations_1.sql b/tests/queries/0_stateless/02662_sparse_columns_mutations_1.sql index 3bf37e8e62b2..a12e8a780ffe 100644 --- a/tests/queries/0_stateless/02662_sparse_columns_mutations_1.sql +++ b/tests/queries/0_stateless/02662_sparse_columns_mutations_1.sql @@ -30,7 +30,7 @@ ORDER BY name; SELECT countIf(s = 'foo'), arraySort(groupUniqArray(s)) FROM t_sparse_mutations_1; -ALTER TABLE t_sparse_mutations_1 MODIFY COLUMN s String; +ALTER TABLE t_sparse_mutations_1 MODIFY COLUMN s String DEFAULT ''; SELECT name, type, serialization_kind FROM system.parts_columns WHERE database = currentDatabase() AND table = 't_sparse_mutations_1' AND column = 's' AND active diff --git a/tests/queries/0_stateless/03575_modify_column_null_to_default.reference b/tests/queries/0_stateless/03575_modify_column_null_to_default.reference new file mode 100644 index 000000000000..4fe707bcfcbb --- /dev/null +++ b/tests/queries/0_stateless/03575_modify_column_null_to_default.reference @@ -0,0 +1,38 @@ +-- { echoOn } + +SELECT * from nullable_test ORDER BY ALL; +1 1 1 A +\N \N \N \N +SYSTEM STOP MERGES nullable_test; +ALTER TABLE nullable_test MODIFY COLUMN my_int_nullable UInt64 SETTINGS mutations_sync = 0, alter_sync = 0; -- { serverError BAD_ARGUMENTS } +ALTER TABLE nullable_test MODIFY COLUMN my_int_nullable UInt64 DEFAULT 42 SETTINGS mutations_sync = 0, alter_sync = 0; +SELECT * from nullable_test ORDER BY ALL; +1 1 1 A +42 \N \N \N +SYSTEM START MERGES nullable_test; +SELECT * from nullable_test ORDER BY ALL; +1 1 1 A +42 \N \N \N +OPTIMIZE TABLE nullable_test FINAL; +SELECT * from nullable_test ORDER BY ALL; +1 1 1 A +42 \N \N \N +ALTER TABLE nullable_test MODIFY COLUMN my_text_lc_nullable String DEFAULT 'empty'; +SELECT * from nullable_test ORDER BY ALL; +1 1 1 A +42 \N \N empty +-- Previouly existing DEFAULT NULL does not allow to modify +ALTER TABLE nullable_test MODIFY COLUMN my_int_nullable_with_default UInt64 SETTINGS mutations_sync = 0, alter_sync = 0; -- { serverError CANNOT_CONVERT_TYPE } +SELECT * from nullable_test ORDER BY ALL; +1 1 1 A +42 \N \N empty +ALTER TABLE nullable_test MODIFY COLUMN my_int_nullable_with_default UInt64 DEFAULT 43 SETTINGS mutations_sync = 0, alter_sync = 0; +SELECT * from nullable_test ORDER BY ALL; +1 1 1 A +42 43 \N empty +-- But when we have DEFAULT which is non NULL we can keep it +ALTER TABLE nullable_test MODIFY COLUMN my_int_nullable_with_default2 UInt64 SETTINGS mutations_sync = 0, alter_sync = 0; +SELECT * from nullable_test ORDER BY ALL; +1 1 1 A +42 43 11 empty +DROP TABLE IF EXISTS nullable_test; diff --git a/tests/queries/0_stateless/03575_modify_column_null_to_default.sql b/tests/queries/0_stateless/03575_modify_column_null_to_default.sql new file mode 100644 index 000000000000..7cd678de2cb3 --- /dev/null +++ b/tests/queries/0_stateless/03575_modify_column_null_to_default.sql @@ -0,0 +1,46 @@ +DROP TABLE IF EXISTS nullable_test; + +CREATE TABLE nullable_test( + my_int_nullable Nullable(UInt32), + my_int_nullable_with_default Nullable(UInt32) DEFAULT NULL, + my_int_nullable_with_default2 Nullable(UInt32) DEFAULT 11, + my_text_lc_nullable LowCardinality(Nullable(String)), +) ORDER BY tuple(); + +INSERT INTO nullable_test VALUES (NULL, NULL, NULL, NULL), (1, 1, 1, 'A'); + +-- { echoOn } + +SELECT * from nullable_test ORDER BY ALL; + +SYSTEM STOP MERGES nullable_test; + +ALTER TABLE nullable_test MODIFY COLUMN my_int_nullable UInt64 SETTINGS mutations_sync = 0, alter_sync = 0; -- { serverError BAD_ARGUMENTS } +ALTER TABLE nullable_test MODIFY COLUMN my_int_nullable UInt64 DEFAULT 42 SETTINGS mutations_sync = 0, alter_sync = 0; + +SELECT * from nullable_test ORDER BY ALL; + +SYSTEM START MERGES nullable_test; + +SELECT * from nullable_test ORDER BY ALL; + +OPTIMIZE TABLE nullable_test FINAL; +SELECT * from nullable_test ORDER BY ALL; + +ALTER TABLE nullable_test MODIFY COLUMN my_text_lc_nullable String DEFAULT 'empty'; +SELECT * from nullable_test ORDER BY ALL; + +-- Previouly existing DEFAULT NULL does not allow to modify +ALTER TABLE nullable_test MODIFY COLUMN my_int_nullable_with_default UInt64 SETTINGS mutations_sync = 0, alter_sync = 0; -- { serverError CANNOT_CONVERT_TYPE } + +SELECT * from nullable_test ORDER BY ALL; + +ALTER TABLE nullable_test MODIFY COLUMN my_int_nullable_with_default UInt64 DEFAULT 43 SETTINGS mutations_sync = 0, alter_sync = 0; +SELECT * from nullable_test ORDER BY ALL; + +-- But when we have DEFAULT which is non NULL we can keep it +ALTER TABLE nullable_test MODIFY COLUMN my_int_nullable_with_default2 UInt64 SETTINGS mutations_sync = 0, alter_sync = 0; + +SELECT * from nullable_test ORDER BY ALL; + +DROP TABLE IF EXISTS nullable_test;