diff --git a/dbms/src/Storages/AlterCommands.cpp b/dbms/src/Storages/AlterCommands.cpp index b24863a2afb..535105e5264 100644 --- a/dbms/src/Storages/AlterCommands.cpp +++ b/dbms/src/Storages/AlterCommands.cpp @@ -257,10 +257,16 @@ void AlterCommand::apply(ColumnsDescription & columns_description, IndicesDescri if (ttl) column.ttl = ttl; - column.type = data_type; + if (data_type) + column.type = data_type; - column.default_desc.kind = default_kind; - column.default_desc.expression = default_expression; + /// User specified default expression or changed + /// datatype. We have to replace default. + if (default_expression || data_type) + { + column.default_desc.kind = default_kind; + column.default_desc.expression = default_expression; + } }); } else if (type == MODIFY_ORDER_BY) @@ -389,13 +395,13 @@ void AlterCommand::apply(ColumnsDescription & columns_description, IndicesDescri bool AlterCommand::isModifyingData() const { - /// Change binary representation on disk + /// Possible change data representation on disk if (type == MODIFY_COLUMN) - return data_type.get() || default_expression; + return data_type != nullptr; - return type == ADD_COLUMN /// We need to change columns.txt in each part - || type == DROP_COLUMN /// We need to change columns.txt in each part - || type == DROP_INDEX; /// We need to remove file from filesystem + return type == ADD_COLUMN /// We need to change columns.txt in each part for MergeTree + || type == DROP_COLUMN /// We need to change columns.txt in each part for MergeTree + || type == DROP_INDEX; /// We need to remove file from filesystem for MergeTree } bool AlterCommand::isSettingsAlter() const diff --git a/dbms/src/Storages/MergeTree/MergeTreeData.cpp b/dbms/src/Storages/MergeTree/MergeTreeData.cpp index 8d892fd69d6..c89af45f2fa 100644 --- a/dbms/src/Storages/MergeTree/MergeTreeData.cpp +++ b/dbms/src/Storages/MergeTree/MergeTreeData.cpp @@ -1342,11 +1342,11 @@ void MergeTreeData::checkAlter(const AlterCommands & commands, const Context & c "before using data skipping indices.", ErrorCodes::BAD_ARGUMENTS); /// Set of columns that shouldn't be altered. - NameSet columns_alter_forbidden; + NameSet columns_alter_type_forbidden; /// Primary key columns can be ALTERed only if they are used in the key as-is /// (and not as a part of some expression) and if the ALTER only affects column metadata. - NameSet columns_alter_metadata_only; + NameSet columns_alter_type_metadata_only; if (partition_key_expr) { @@ -1354,13 +1354,13 @@ void MergeTreeData::checkAlter(const AlterCommands & commands, const Context & c /// TODO: in some cases (e.g. adding an Enum value) a partition key column can still be ALTERed. /// We should allow it. for (const String & col : partition_key_expr->getRequiredColumns()) - columns_alter_forbidden.insert(col); + columns_alter_type_forbidden.insert(col); } for (const auto & index : skip_indices) { for (const String & col : index->expr->getRequiredColumns()) - columns_alter_forbidden.insert(col); + columns_alter_type_forbidden.insert(col); } if (sorting_key_expr) @@ -1368,17 +1368,16 @@ void MergeTreeData::checkAlter(const AlterCommands & commands, const Context & c for (const ExpressionAction & action : sorting_key_expr->getActions()) { auto action_columns = action.getNeededColumns(); - columns_alter_forbidden.insert(action_columns.begin(), action_columns.end()); + columns_alter_type_forbidden.insert(action_columns.begin(), action_columns.end()); } for (const String & col : sorting_key_expr->getRequiredColumns()) - columns_alter_metadata_only.insert(col); + columns_alter_type_metadata_only.insert(col); /// We don't process sample_by_ast separately because it must be among the primary key columns /// and we don't process primary_key_expr separately because it is a prefix of sorting_key_expr. } - if (!merging_params.sign_column.empty()) - columns_alter_forbidden.insert(merging_params.sign_column); + columns_alter_type_forbidden.insert(merging_params.sign_column); std::map old_types; for (const auto & column : getColumns().getAllPhysical()) @@ -1386,34 +1385,26 @@ void MergeTreeData::checkAlter(const AlterCommands & commands, const Context & c for (const AlterCommand & command : commands) { - if (!command.isModifyingData()) + if (command.type == AlterCommand::MODIFY_ORDER_BY && !is_custom_partitioned) { - continue; - } - - if (columns_alter_forbidden.count(command.column_name)) - throw Exception("Trying to ALTER key column " + command.column_name, ErrorCodes::ILLEGAL_COLUMN); - - if (columns_alter_metadata_only.count(command.column_name)) - { - if (command.type == AlterCommand::MODIFY_COLUMN) - { - auto it = old_types.find(command.column_name); - if (it != old_types.end() && isMetadataOnlyConversion(it->second, command.data_type.get())) - continue; - } - throw Exception( - "ALTER of key column " + command.column_name + " must be metadata-only", - ErrorCodes::ILLEGAL_COLUMN); + "ALTER MODIFY ORDER BY is not supported for default-partitioned tables created with the old syntax", + ErrorCodes::BAD_ARGUMENTS); } - - if (command.type == AlterCommand::MODIFY_ORDER_BY) + else if (command.isModifyingData()) { - if (!is_custom_partitioned) - throw Exception( - "ALTER MODIFY ORDER BY is not supported for default-partitioned tables created with the old syntax", - ErrorCodes::BAD_ARGUMENTS); + if (columns_alter_type_forbidden.count(command.column_name)) + throw Exception("Trying to ALTER key column " + command.column_name, ErrorCodes::ILLEGAL_COLUMN); + + if (columns_alter_type_metadata_only.count(command.column_name)) + { + if (command.type == AlterCommand::MODIFY_COLUMN) + { + auto it = old_types.find(command.column_name); + if (it == old_types.end() || !isMetadataOnlyConversion(it->second, command.data_type.get())) + throw Exception("ALTER of key column " + command.column_name + " must be metadata-only", ErrorCodes::ILLEGAL_COLUMN); + } + } } } @@ -1425,12 +1416,15 @@ void MergeTreeData::checkAlter(const AlterCommands & commands, const Context & c for (const auto & setting : new_changes) checkSettingCanBeChanged(setting.name); - /// Check that type conversions are possible. - ExpressionActionsPtr unused_expression; - NameToNameMap unused_map; - bool unused_bool; - createConvertExpression(nullptr, getColumns().getAllPhysical(), new_columns.getAllPhysical(), - getIndices().indices, new_indices.indices, unused_expression, unused_map, unused_bool); + if (commands.isModifyingData()) + { + /// Check that type conversions are possible. + ExpressionActionsPtr unused_expression; + NameToNameMap unused_map; + bool unused_bool; + createConvertExpression(nullptr, getColumns().getAllPhysical(), new_columns.getAllPhysical(), + getIndices().indices, new_indices.indices, unused_expression, unused_map, unused_bool); + } } void MergeTreeData::createConvertExpression(const DataPartPtr & part, const NamesAndTypesList & old_columns, diff --git a/dbms/src/Storages/StorageMergeTree.cpp b/dbms/src/Storages/StorageMergeTree.cpp index e6a847ba1bd..66d22da5c3b 100644 --- a/dbms/src/Storages/StorageMergeTree.cpp +++ b/dbms/src/Storages/StorageMergeTree.cpp @@ -299,9 +299,6 @@ void StorageMergeTree::alter( { lockStructureExclusively(table_lock_holder, context.getCurrentQueryId()); - params.apply(new_columns, new_indices, new_constraints, new_order_by_ast, new_primary_key_ast, new_ttl_table_ast, new_changes); - - IDatabase::ASTModifier settings_modifier = getSettingsModifier(new_changes); context.getDatabase(current_database_name)->alterTable(context, current_table_name, new_columns, new_indices, new_constraints, storage_modifier); update_metadata();