From ec933d9d03657edd468c11e2a5f35d4dd219e1b6 Mon Sep 17 00:00:00 2001 From: alesapin Date: Wed, 10 Jun 2020 12:09:51 +0300 Subject: [PATCH] Better naming --- src/Storages/AlterCommands.cpp | 27 ++++++++++++++-- src/Storages/IStorage.cpp | 8 ++--- src/Storages/IStorage.h | 1 + src/Storages/MergeTree/MergeTreeData.cpp | 32 ++++++++----------- src/Storages/MergeTree/MergeTreeData.h | 3 +- .../MergeTree/registerStorageMergeTree.cpp | 7 ++++ src/Storages/StorageMergeTree.cpp | 2 +- src/Storages/StorageReplicatedMergeTree.cpp | 22 ++++++------- tests/queries/0_stateless/00933_alter_ttl.sql | 3 +- 9 files changed, 65 insertions(+), 40 deletions(-) diff --git a/src/Storages/AlterCommands.cpp b/src/Storages/AlterCommands.cpp index 5652d1717ec..4755ad0fd35 100644 --- a/src/Storages/AlterCommands.cpp +++ b/src/Storages/AlterCommands.cpp @@ -314,8 +314,6 @@ void AlterCommand::apply(StorageInMemoryMetadata & metadata, const Context & con } }); - if (metadata.sorting_key.sample_block.has(column_name)) - metadata.sorting_key = KeyDescription::getKeyFromAST(metadata.sorting_key.definition_ast, metadata.columns, context); } else if (type == MODIFY_ORDER_BY) { @@ -714,6 +712,31 @@ void AlterCommands::apply(StorageInMemoryMetadata & metadata, const Context & co if (!command.ignore) command.apply(metadata_copy, context); + /// Changes in columns may lead to changes in keys expression + metadata_copy.sorting_key = KeyDescription::getKeyFromAST(metadata_copy.sorting_key.definition_ast, metadata_copy.columns, context); + + if (metadata_copy.primary_key.definition_ast != nullptr) + { + metadata_copy.primary_key = KeyDescription::getKeyFromAST(metadata_copy.primary_key.definition_ast, metadata_copy.columns, context); + } + else + { + metadata_copy.primary_key = metadata_copy.sorting_key; + metadata_copy.primary_key.definition_ast = nullptr; + } + + /// Changes in columns may lead to changes in TTL expressions + auto column_ttl_asts = metadata_copy.columns.getColumnTTLs(); + for (const auto & [name, ast] : column_ttl_asts) + { + auto new_ttl_entry = TTLDescription::getTTLFromAST(ast, metadata_copy.columns, context, metadata_copy.primary_key); + metadata_copy.column_ttls_by_name[name] = new_ttl_entry; + } + + if (metadata_copy.table_ttl.definition_ast != nullptr) + metadata.table_ttl = TTLTableDescription::getTTLForTableFromAST( + metadata_copy.table_ttl.definition_ast, metadata_copy.columns, context, metadata_copy.primary_key); + metadata = std::move(metadata_copy); } diff --git a/src/Storages/IStorage.cpp b/src/Storages/IStorage.cpp index 31dd87a7ce4..83c95d9a20f 100644 --- a/src/Storages/IStorage.cpp +++ b/src/Storages/IStorage.cpp @@ -380,10 +380,10 @@ void IStorage::alter( { lockStructureExclusively(table_lock_holder, context.getCurrentQueryId(), context.getSettingsRef().lock_acquire_timeout); auto table_id = getStorageID(); - StorageInMemoryMetadata old_metadata = getInMemoryMetadata(); - params.apply(old_metadata, context); - DatabaseCatalog::instance().getDatabase(table_id.database_name)->alterTable(context, table_id, old_metadata); - setColumns(std::move(old_metadata.columns)); + StorageInMemoryMetadata new_metadata = getInMemoryMetadata(); + params.apply(new_metadata, context); + DatabaseCatalog::instance().getDatabase(table_id.database_name)->alterTable(context, table_id, new_metadata); + setColumns(std::move(new_metadata.columns)); } diff --git a/src/Storages/IStorage.h b/src/Storages/IStorage.h index 82012eacc03..c06b969ebf5 100644 --- a/src/Storages/IStorage.h +++ b/src/Storages/IStorage.h @@ -154,6 +154,7 @@ public: /// thread-unsafe part. lockStructure must be acquired void setSettingsChanges(const ASTPtr & settings_changes_); bool hasSettingsChanges() const { return metadata.settings_changes != nullptr; } + /// Select query for *View storages. const SelectQueryDescription & getSelectQuery() const; void setSelectQuery(const SelectQueryDescription & select_); bool hasSelectQuery() const; diff --git a/src/Storages/MergeTree/MergeTreeData.cpp b/src/Storages/MergeTree/MergeTreeData.cpp index 50971bbc881..4a65d05cc8e 100644 --- a/src/Storages/MergeTree/MergeTreeData.cpp +++ b/src/Storages/MergeTree/MergeTreeData.cpp @@ -185,7 +185,7 @@ MergeTreeData::MergeTreeData( min_format_version = MERGE_TREE_DATA_MIN_FORMAT_VERSION_WITH_CUSTOM_PARTITIONING; } - setTTLExpressions(metadata_.columns, metadata_.table_ttl); + setTTLExpressions(metadata_); /// format_file always contained on any data path PathWithDisk version_file; @@ -296,9 +296,9 @@ void MergeTreeData::setProperties(const StorageInMemoryMetadata & new_metadata, if (new_primary_key.definition_ast == nullptr) { /// We copy sorting key, and restore definition_ast to empty value, - /// because in merge tree code we sometimes chech, that our primary key - /// is fake (copied from sorting key, i.e. isPrimaryKeyDefined() == - /// false, but hasSortingKey() == true) + /// because in merge tree code we chech, that our primary key is fake + /// (copied from sorting key, i.e. isPrimaryKeyDefined() == false, but + /// hasSortingKey() == true) new_primary_key = new_metadata.sorting_key; new_primary_key.definition_ast = nullptr; } @@ -522,14 +522,11 @@ void MergeTreeData::initPartitionKey(const KeyDescription & new_partition_key) /// Todo replace columns with TTL for columns -void MergeTreeData::setTTLExpressions(const ColumnsDescription & new_columns, - const TTLTableDescription & new_table_ttl, bool only_check) +void MergeTreeData::setTTLExpressions(const StorageInMemoryMetadata & new_metadata, bool only_check) { - auto new_column_ttls_asts = new_columns.getColumnTTLs(); + auto new_column_ttls = new_metadata.column_ttls_by_name; - TTLColumnsDescription new_column_ttl_by_name = getColumnTTLs(); - - if (!new_column_ttls_asts.empty()) + if (!new_column_ttls.empty()) { NameSet columns_ttl_forbidden; @@ -541,20 +538,18 @@ void MergeTreeData::setTTLExpressions(const ColumnsDescription & new_columns, for (const auto & col : getColumnsRequiredForSortingKey()) columns_ttl_forbidden.insert(col); - for (const auto & [name, ast] : new_column_ttls_asts) + for (const auto & [name, ttl_description] : new_column_ttls) { if (columns_ttl_forbidden.count(name)) throw Exception("Trying to set TTL for key column " + name, ErrorCodes::ILLEGAL_COLUMN); - else - { - auto new_ttl_entry = TTLDescription::getTTLFromAST(ast, new_columns, global_context, getPrimaryKey()); - new_column_ttl_by_name[name] = new_ttl_entry; - } } + if (!only_check) - setColumnTTLs(new_column_ttl_by_name); + setColumnTTLs(new_column_ttls); } + auto new_table_ttl = new_metadata.table_ttl; + if (new_table_ttl.definition_ast) { for (const auto & move_ttl : new_table_ttl.move_ttl) @@ -570,7 +565,6 @@ void MergeTreeData::setTTLExpressions(const ColumnsDescription & new_columns, } } - if (!only_check) { auto move_ttl_entries_lock = std::lock_guard(move_ttl_entries_mutex); @@ -1367,7 +1361,7 @@ void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, const S setProperties(new_metadata, /* only_check = */ true); - setTTLExpressions(new_metadata.columns, new_metadata.table_ttl, /* only_check = */ true); + setTTLExpressions(new_metadata, /* only_check = */ true); if (hasSettingsChanges()) { diff --git a/src/Storages/MergeTree/MergeTreeData.h b/src/Storages/MergeTree/MergeTreeData.h index 0e2e9c71324..a239f8cc0d7 100644 --- a/src/Storages/MergeTree/MergeTreeData.h +++ b/src/Storages/MergeTree/MergeTreeData.h @@ -784,8 +784,7 @@ protected: void initPartitionKey(const KeyDescription & new_partition_key); - void setTTLExpressions(const ColumnsDescription & columns, - const TTLTableDescription & new_table_ttl, bool only_check = false); + void setTTLExpressions(const StorageInMemoryMetadata & new_metadata, bool only_check = false); void checkStoragePolicy(const StoragePolicyPtr & new_storage_policy) const; diff --git a/src/Storages/MergeTree/registerStorageMergeTree.cpp b/src/Storages/MergeTree/registerStorageMergeTree.cpp index 50775a04255..90eee680d65 100644 --- a/src/Storages/MergeTree/registerStorageMergeTree.cpp +++ b/src/Storages/MergeTree/registerStorageMergeTree.cpp @@ -530,6 +530,13 @@ static StoragePtr create(const StorageFactory::Arguments & args) for (auto & constraint : args.query.columns_list->constraints->children) metadata.constraints.constraints.push_back(constraint); + auto column_ttl_asts = args.columns.getColumnTTLs(); + for (const auto & [name, ast] : column_ttl_asts) + { + auto new_ttl_entry = TTLDescription::getTTLFromAST(ast, args.columns, args.context, metadata.primary_key); + metadata.column_ttls_by_name[name] = new_ttl_entry; + } + storage_settings->loadFromQuery(*args.storage_def); if (args.storage_def->settings) diff --git a/src/Storages/StorageMergeTree.cpp b/src/Storages/StorageMergeTree.cpp index 76f752abb68..2a7efa164d4 100644 --- a/src/Storages/StorageMergeTree.cpp +++ b/src/Storages/StorageMergeTree.cpp @@ -282,7 +282,7 @@ void StorageMergeTree::alter( /// Reinitialize primary key because primary key column types might have changed. setProperties(new_metadata); - setTTLExpressions(new_metadata.columns, new_metadata.table_ttl); + setTTLExpressions(new_metadata); DatabaseCatalog::instance().getDatabase(table_id.database_name)->alterTable(context, table_id, new_metadata); diff --git a/src/Storages/StorageReplicatedMergeTree.cpp b/src/Storages/StorageReplicatedMergeTree.cpp index 8efe22e03f9..42adf4d2b45 100644 --- a/src/Storages/StorageReplicatedMergeTree.cpp +++ b/src/Storages/StorageReplicatedMergeTree.cpp @@ -472,9 +472,9 @@ void StorageReplicatedMergeTree::checkTableStructure(const String & zookeeper_pr void StorageReplicatedMergeTree::setTableStructure(ColumnsDescription new_columns, const ReplicatedMergeTreeTableMetadata::Diff & metadata_diff) { - StorageInMemoryMetadata current_metadata = getInMemoryMetadata(); - if (new_columns != current_metadata.columns) - current_metadata.columns = new_columns; + StorageInMemoryMetadata new_metadata = getInMemoryMetadata(); + if (new_columns != new_metadata.columns) + new_metadata.columns = new_columns; if (!metadata_diff.empty()) { @@ -492,37 +492,37 @@ void StorageReplicatedMergeTree::setTableStructure(ColumnsDescription new_column tuple->arguments->children = new_sorting_key_expr_list->children; order_by_ast = tuple; } - current_metadata.sorting_key = KeyDescription::getKeyFromAST(order_by_ast, current_metadata.columns, global_context); + new_metadata.sorting_key = KeyDescription::getKeyFromAST(order_by_ast, new_metadata.columns, global_context); if (!isPrimaryKeyDefined()) { /// Primary and sorting key become independent after this ALTER so we have to /// save the old ORDER BY expression as the new primary key. - current_metadata.primary_key = getSortingKey(); + new_metadata.primary_key = getSortingKey(); } } if (metadata_diff.skip_indices_changed) - current_metadata.secondary_indices = IndicesDescription::parse(metadata_diff.new_skip_indices, new_columns, global_context); + new_metadata.secondary_indices = IndicesDescription::parse(metadata_diff.new_skip_indices, new_columns, global_context); if (metadata_diff.constraints_changed) - current_metadata.constraints = ConstraintsDescription::parse(metadata_diff.new_constraints); + new_metadata.constraints = ConstraintsDescription::parse(metadata_diff.new_constraints); if (metadata_diff.ttl_table_changed) { ParserTTLExpressionList parser; auto ttl_for_table_ast = parseQuery(parser, metadata_diff.new_ttl_table, 0, DBMS_DEFAULT_MAX_PARSER_DEPTH); - current_metadata.table_ttl = TTLTableDescription::getTTLForTableFromAST(ttl_for_table_ast, current_metadata.columns, global_context, current_metadata.primary_key); + new_metadata.table_ttl = TTLTableDescription::getTTLForTableFromAST(ttl_for_table_ast, new_metadata.columns, global_context, new_metadata.primary_key); } } auto table_id = getStorageID(); - DatabaseCatalog::instance().getDatabase(table_id.database_name)->alterTable(global_context, table_id, current_metadata); + DatabaseCatalog::instance().getDatabase(table_id.database_name)->alterTable(global_context, table_id, new_metadata); /// Even if the primary/sorting keys didn't change we must reinitialize it /// because primary key column types might have changed. - setProperties(current_metadata); - setTTLExpressions(new_columns, current_metadata.table_ttl); + setProperties(new_metadata); + setTTLExpressions(new_metadata); } diff --git a/tests/queries/0_stateless/00933_alter_ttl.sql b/tests/queries/0_stateless/00933_alter_ttl.sql index d3298b3fbe1..4e0fde00952 100644 --- a/tests/queries/0_stateless/00933_alter_ttl.sql +++ b/tests/queries/0_stateless/00933_alter_ttl.sql @@ -21,6 +21,7 @@ drop table if exists ttl; create table ttl (d Date, a Int) engine = MergeTree order by tuple() partition by toDayOfMonth(d); alter table ttl modify column a Int ttl d + interval 1 day; desc table ttl; -alter table ttl modify column d Int ttl d + interval 1 day; -- { serverError 524 } +alter table ttl modify column d Int ttl d + interval 1 day; -- { serverError 43 } +alter table ttl modify column d DateTime ttl d + interval 1 day; -- { serverError 524 } drop table if exists ttl;