From 1707f84a44ce010710aefa097684c8e85bf6d35b Mon Sep 17 00:00:00 2001 From: alesapin Date: Mon, 27 Jul 2020 12:42:37 +0300 Subject: [PATCH] Less strict check and rare rename bug --- src/Interpreters/MutationsInterpreter.cpp | 8 +-- src/Interpreters/MutationsInterpreter.h | 2 +- src/Storages/MergeTree/IMergeTreeDataPart.cpp | 3 +- .../MergeTree/MergeTreeDataMergerMutator.cpp | 55 +++++++++++++++---- .../MergeTree/MergeTreeDataPartWide.cpp | 3 +- 5 files changed, 53 insertions(+), 18 deletions(-) diff --git a/src/Interpreters/MutationsInterpreter.cpp b/src/Interpreters/MutationsInterpreter.cpp index bb83001852c..e502f517ad8 100644 --- a/src/Interpreters/MutationsInterpreter.cpp +++ b/src/Interpreters/MutationsInterpreter.cpp @@ -777,10 +777,10 @@ bool MutationsInterpreter::Stage::isAffectingAllColumns(const Names & storage_co bool MutationsInterpreter::isAffectingAllColumns() const { auto storage_columns = metadata_snapshot->getColumns().getNamesOfPhysical(); - for (const auto & stage : stages) - if (stage.isAffectingAllColumns(storage_columns)) - return true; - return false; + if (stages.empty()) + throw Exception(ErrorCodes::LOGICAL_ERROR, "Mutation interpreter has no stages"); + + return stages.back().isAffectingAllColumns(storage_columns); } } diff --git a/src/Interpreters/MutationsInterpreter.h b/src/Interpreters/MutationsInterpreter.h index 8d189b121ee..c9130ad6613 100644 --- a/src/Interpreters/MutationsInterpreter.h +++ b/src/Interpreters/MutationsInterpreter.h @@ -42,7 +42,7 @@ public: /// Only changed columns. const Block & getUpdatedHeader() const; - /// At least one mutation stage affects all columns in storage + /// Latest mutation stage affects all columns in storage bool isAffectingAllColumns() const; private: diff --git a/src/Storages/MergeTree/IMergeTreeDataPart.cpp b/src/Storages/MergeTree/IMergeTreeDataPart.cpp index 0c329299209..d5b59f3d7b6 100644 --- a/src/Storages/MergeTree/IMergeTreeDataPart.cpp +++ b/src/Storages/MergeTree/IMergeTreeDataPart.cpp @@ -557,7 +557,8 @@ void IMergeTreeDataPart::loadRowsCount() /// columns have to be loaded for (const auto & column : getColumns()) { - if (column.type->isValueRepresentedByNumber()) + /// Most trivial types + if (column.type->isValueRepresentedByNumber() && !column.type->haveSubtypes()) { auto size = getColumnSize(column.name, *column.type); diff --git a/src/Storages/MergeTree/MergeTreeDataMergerMutator.cpp b/src/Storages/MergeTree/MergeTreeDataMergerMutator.cpp index cd14a35edba..2b407cb7269 100644 --- a/src/Storages/MergeTree/MergeTreeDataMergerMutator.cpp +++ b/src/Storages/MergeTree/MergeTreeDataMergerMutator.cpp @@ -1478,13 +1478,14 @@ NamesAndTypesList MergeTreeDataMergerMutator::getColumnsForNewDataPart( return updated_header.getNamesAndTypesList(); NameSet removed_columns; - NameToNameMap renamed_columns; + NameToNameMap renamed_columns_to_from; + /// All commands are validated in AlterCommand so we don't care about order for (const auto & command : commands_for_removes) { if (command.type == MutationCommand::DROP_COLUMN) removed_columns.insert(command.column_name); if (command.type == MutationCommand::RENAME_COLUMN) - renamed_columns.emplace(command.rename_to, command.column_name); + renamed_columns_to_from.emplace(command.rename_to, command.column_name); } Names source_column_names = source_part->getColumns().getNames(); NameSet source_columns_name_set(source_column_names.begin(), source_column_names.end()); @@ -1497,17 +1498,49 @@ NamesAndTypesList MergeTreeDataMergerMutator::getColumnsForNewDataPart( it->type = updated_type; ++it; } - else if (source_columns_name_set.count(it->name) && !removed_columns.count(it->name)) - { - ++it; - } - else if (renamed_columns.count(it->name) && source_columns_name_set.count(renamed_columns[it->name])) - { - ++it; - } else { - it = storage_columns.erase(it); + if (!source_columns_name_set.count(it->name)) + { + /// Source part doesn't have column but some other column + /// was renamed to it's name. + auto renamed_it = renamed_columns_to_from.find(it->name); + if (renamed_it != renamed_columns_to_from.end() + && source_columns_name_set.count(renamed_it->second)) + ++it; + else + it = storage_columns.erase(it); + } + else + { + bool was_renamed = false; + bool was_removed = removed_columns.count(it->name); + + /// Check that this column was renamed to some other name + for (const auto & [rename_to, rename_from] : renamed_columns_to_from) + { + if (rename_from == it->name) + { + was_renamed = true; + break; + } + } + + /// If we want to rename this column to some other name, than it + /// should it's previous version should be dropped or removed + if (renamed_columns_to_from.count(it->name) && !was_renamed && !was_removed) + throw Exception( + ErrorCodes::LOGICAL_ERROR, + "Incorrect mutation commands, trying to rename column {} to {}, but part {} already has column {}", renamed_columns_to_from[it->name], it->name, source_part->name, it->name); + + + /// Column was renamed and no other column renamed to it's name + /// or column is dropped. + if (!renamed_columns_to_from.count(it->name) && (was_renamed || was_removed)) + it = storage_columns.erase(it); + else + ++it; + } } } diff --git a/src/Storages/MergeTree/MergeTreeDataPartWide.cpp b/src/Storages/MergeTree/MergeTreeDataPartWide.cpp index a3e45a22d92..f133d438866 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartWide.cpp +++ b/src/Storages/MergeTree/MergeTreeDataPartWide.cpp @@ -240,7 +240,8 @@ void MergeTreeDataPartWide::calculateEachColumnSizes(ColumnSizeByName & each_col total_size.add(size); #ifndef NDEBUG - if (rows_count != 0 && column.type->isValueRepresentedByNumber()) + /// Most trivial types + if (rows_count != 0 && column.type->isValueRepresentedByNumber() && !column.type->haveSubtypes()) { size_t rows_in_column = size.data_uncompressed / column.type->getSizeOfValueInMemory(); if (rows_in_column != rows_count)