From 73ddc25e1eaa31e30231892719d3a74b985e173d Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Thu, 31 Mar 2022 12:50:13 +0300 Subject: [PATCH 1/2] Require mutations for DROP COLUMN by root column name for nested columns Signed-off-by: Azat Khuzhin --- src/Storages/AlterCommands.cpp | 2 +- src/Storages/ColumnsDescription.cpp | 7 +++++++ src/Storages/ColumnsDescription.h | 1 + 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/Storages/AlterCommands.cpp b/src/Storages/AlterCommands.cpp index e59078c642c..845aae52582 100644 --- a/src/Storages/AlterCommands.cpp +++ b/src/Storages/AlterCommands.cpp @@ -785,7 +785,7 @@ bool AlterCommand::isRequireMutationStage(const StorageInMemoryMetadata & metada /// Drop alias is metadata alter, in other case mutation is required. if (type == DROP_COLUMN) - return metadata.columns.hasPhysical(column_name); + return metadata.columns.hasColumnOrNested(GetColumnsOptions::AllPhysical, column_name); if (type != MODIFY_COLUMN || data_type == nullptr) return false; diff --git a/src/Storages/ColumnsDescription.cpp b/src/Storages/ColumnsDescription.cpp index 1264da77b04..21758202221 100644 --- a/src/Storages/ColumnsDescription.cpp +++ b/src/Storages/ColumnsDescription.cpp @@ -603,6 +603,13 @@ bool ColumnsDescription::hasColumnOrSubcolumn(GetColumnsOptions::Kind kind, cons || hasSubcolumn(column_name); } +bool ColumnsDescription::hasColumnOrNested(GetColumnsOptions::Kind kind, const String & column_name) const +{ + auto range = getNameRange(columns, column_name); + return range.first != range.second && + defaultKindToGetKind(range.first->default_desc.kind) & kind; +} + bool ColumnsDescription::hasDefaults() const { for (const auto & column : columns) diff --git a/src/Storages/ColumnsDescription.h b/src/Storages/ColumnsDescription.h index 670aeaa293d..96265804f06 100644 --- a/src/Storages/ColumnsDescription.h +++ b/src/Storages/ColumnsDescription.h @@ -167,6 +167,7 @@ public: bool hasPhysical(const String & column_name) const; bool hasColumnOrSubcolumn(GetColumnsOptions::Kind kind, const String & column_name) const; + bool hasColumnOrNested(GetColumnsOptions::Kind kind, const String & column_name) const; NameAndTypePair getPhysical(const String & column_name) const; NameAndTypePair getColumnOrSubcolumn(GetColumnsOptions::Kind kind, const String & column_name) const; From bf312c2a5b6e42f9773d07cd9ed44532e42c6c91 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Thu, 31 Mar 2022 13:24:05 +0300 Subject: [PATCH 2/2] Fix ALTER DROP COLUMN of nested column with compact parts ALTER DROP COLUMN of nested column did not requires mutation before, and so it leaves nested column as-is, and in case of compact parts subsequent alter, that requires mutation, will trigger READ_COLUMN of that nested column (because it exists in part), but it will fail because there is no such column in the table already. Here is example of such a failure on CI - [1]. [1]: https://s3.amazonaws.com/clickhouse-test-reports/35459/52099b23a1cb9a7ff036c5c60aa037c999b333ef/stateless_tests__thread__actions__[1/3].html Signed-off-by: Azat Khuzhin --- src/Storages/ColumnsDescription.cpp | 9 +++++++++ src/Storages/ColumnsDescription.h | 3 +++ src/Storages/MergeTree/MutateTask.cpp | 18 +++++++++++++----- ...r_compact_part_drop_nested_column.reference | 0 ...lter_compact_part_drop_nested_column.sql.j2 | 17 +++++++++++++++++ 5 files changed, 42 insertions(+), 5 deletions(-) create mode 100644 tests/queries/0_stateless/02260_alter_compact_part_drop_nested_column.reference create mode 100644 tests/queries/0_stateless/02260_alter_compact_part_drop_nested_column.sql.j2 diff --git a/src/Storages/ColumnsDescription.cpp b/src/Storages/ColumnsDescription.cpp index 21758202221..3aa5b28fed5 100644 --- a/src/Storages/ColumnsDescription.cpp +++ b/src/Storages/ColumnsDescription.cpp @@ -397,6 +397,15 @@ NamesAndTypesList ColumnsDescription::getSubcolumns(const String & name_in_stora return NamesAndTypesList(range.first, range.second); } +NamesAndTypesList ColumnsDescription::getNested(const String & column_name) const +{ + auto range = getNameRange(columns, column_name); + NamesAndTypesList nested; + for (auto & it = range.first; it != range.second; ++it) + nested.emplace_back(it->name, it->type); + return nested; +} + void ColumnsDescription::addSubcolumnsToList(NamesAndTypesList & source_list) const { NamesAndTypesList subcolumns_list; diff --git a/src/Storages/ColumnsDescription.h b/src/Storages/ColumnsDescription.h index 96265804f06..75db8b92545 100644 --- a/src/Storages/ColumnsDescription.h +++ b/src/Storages/ColumnsDescription.h @@ -127,7 +127,10 @@ public: NamesAndTypesList getEphemeral() const; NamesAndTypesList getAllPhysical() const; /// ordinary + materialized. NamesAndTypesList getAll() const; /// ordinary + materialized + aliases + ephemeral + /// Returns .size0/.null/... NamesAndTypesList getSubcolumns(const String & name_in_storage) const; + /// Returns column_name.* + NamesAndTypesList getNested(const String & column_name) const; using ColumnTTLs = std::unordered_map; ColumnTTLs getColumnTTLs() const; diff --git a/src/Storages/MergeTree/MutateTask.cpp b/src/Storages/MergeTree/MutateTask.cpp index c71d2a89e19..aac315e8f25 100644 --- a/src/Storages/MergeTree/MutateTask.cpp +++ b/src/Storages/MergeTree/MutateTask.cpp @@ -80,20 +80,28 @@ static void splitMutationCommands( { for_file_renames.push_back(command); } - else if (part_columns.has(command.column_name)) + else if (bool has_column = part_columns.has(command.column_name), has_nested_column = part_columns.hasNested(command.column_name); has_column || has_nested_column) { - if (command.type == MutationCommand::Type::DROP_COLUMN) + if (command.type == MutationCommand::Type::DROP_COLUMN || command.type == MutationCommand::Type::RENAME_COLUMN) { - mutated_columns.emplace(command.column_name); + if (has_nested_column) + { + const auto & nested = part_columns.getNested(command.column_name); + assert(!nested.empty()); + for (const auto & nested_column : nested) + mutated_columns.emplace(nested_column.name); + } + else + mutated_columns.emplace(command.column_name); } - else if (command.type == MutationCommand::Type::RENAME_COLUMN) + + if (command.type == MutationCommand::Type::RENAME_COLUMN) { for_interpreter.push_back( { .type = MutationCommand::Type::READ_COLUMN, .column_name = command.rename_to, }); - mutated_columns.emplace(command.column_name); part_columns.rename(command.column_name, command.rename_to); } } diff --git a/tests/queries/0_stateless/02260_alter_compact_part_drop_nested_column.reference b/tests/queries/0_stateless/02260_alter_compact_part_drop_nested_column.reference new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/queries/0_stateless/02260_alter_compact_part_drop_nested_column.sql.j2 b/tests/queries/0_stateless/02260_alter_compact_part_drop_nested_column.sql.j2 new file mode 100644 index 00000000000..57db8618f8d --- /dev/null +++ b/tests/queries/0_stateless/02260_alter_compact_part_drop_nested_column.sql.j2 @@ -0,0 +1,17 @@ +{# force compact parts and wide #} +{% for min_bytes_for_wide_part in [100000, 0] %} +DROP TABLE IF EXISTS compact_alter_{{ min_bytes_for_wide_part }}; +CREATE TABLE compact_alter_{{ min_bytes_for_wide_part }} (d Date, s String, k UInt64) ENGINE=MergeTree() PARTITION BY d ORDER BY k SETTINGS min_bytes_for_wide_part={{ min_bytes_for_wide_part }}; + +INSERT INTO compact_alter_{{ min_bytes_for_wide_part }} VALUES ('2015-01-01', '2015-01-01 00:00:00', 10); + +ALTER TABLE compact_alter_{{ min_bytes_for_wide_part }} ADD COLUMN n.d Int; +-- force columns creation +OPTIMIZE TABLE compact_alter_{{ min_bytes_for_wide_part }} FINAL; +-- this command will not drop n.d from compact part columns.txt +ALTER TABLE compact_alter_{{ min_bytes_for_wide_part }} DROP COLUMN n; +-- and now modify column will trigger READ_COLUMN of n.d, and it will bail +ALTER TABLE compact_alter_{{ min_bytes_for_wide_part }} MODIFY COLUMN s DateTime('UTC') DEFAULT '1970-01-01 00:00:00'; + +DROP TABLE compact_alter_{{ min_bytes_for_wide_part }}; +{% endfor %}