From 2002289003076bc04b6f19a2a1df7f3a52295312 Mon Sep 17 00:00:00 2001 From: alesapin Date: Thu, 15 Oct 2020 16:02:39 +0300 Subject: [PATCH] Fix bug in alter primary key for replicated versioned collapsing merge tree --- src/Storages/StorageReplicatedMergeTree.cpp | 16 +++++--- ...r_add_and_modify_order_zookeeper.reference | 6 +++ ...6_alter_add_and_modify_order_zookeeper.sql | 38 +++++++++++++++++++ 3 files changed, 55 insertions(+), 5 deletions(-) create mode 100644 tests/queries/0_stateless/01526_alter_add_and_modify_order_zookeeper.reference create mode 100644 tests/queries/0_stateless/01526_alter_add_and_modify_order_zookeeper.sql diff --git a/src/Storages/StorageReplicatedMergeTree.cpp b/src/Storages/StorageReplicatedMergeTree.cpp index 2ac8ddb7846..be8aea05c89 100644 --- a/src/Storages/StorageReplicatedMergeTree.cpp +++ b/src/Storages/StorageReplicatedMergeTree.cpp @@ -833,7 +833,7 @@ void StorageReplicatedMergeTree::checkTableStructure(const String & zookeeper_pr } void StorageReplicatedMergeTree::setTableStructure( -ColumnsDescription new_columns, const ReplicatedMergeTreeTableMetadata::Diff & metadata_diff) + ColumnsDescription new_columns, const ReplicatedMergeTreeTableMetadata::Diff & metadata_diff) { StorageInMemoryMetadata new_metadata = getInMemoryMetadata(); StorageInMemoryMetadata old_metadata = getInMemoryMetadata(); @@ -856,7 +856,7 @@ ColumnsDescription new_columns, const ReplicatedMergeTreeTableMetadata::Diff & m if (!metadata_diff.empty()) { - auto parse_key_expr = [](const String & key_expr) + auto parse_key_expr = [] (const String & key_expr) { ParserNotEmptyExpressionList parser(false); auto new_sorting_key_expr_list = parseQuery(parser, key_expr, 0, DBMS_DEFAULT_MAX_PARSER_DEPTH); @@ -3936,13 +3936,19 @@ void StorageReplicatedMergeTree::alter( ReplicatedMergeTreeTableMetadata future_metadata_in_zk(*this, current_metadata); if (ast_to_str(future_metadata.sorting_key.definition_ast) != ast_to_str(current_metadata->sorting_key.definition_ast)) - future_metadata_in_zk.sorting_key = serializeAST(*future_metadata.sorting_key.expression_list_ast); + { + /// We serialize definition_ast as list, because code which apply ALTER (setTableStructure) expect serialized non empty expression + /// list here and we cannot change this representation for compatibility. Also we have preparsed AST `sorting_key.expression_list_ast` + /// in KeyDescription, but it contain version column for VersionedCollapsingMergeTree, which shouldn't be defined as a part of key definition AST. + /// So the best compatible way is just to convert definition_ast to list and serialize it. In all other places key.expression_list_ast should be used. + future_metadata_in_zk.sorting_key = serializeAST(*extractKeyExpressionList(future_metadata.sorting_key.definition_ast)); + } if (ast_to_str(future_metadata.sampling_key.definition_ast) != ast_to_str(current_metadata->sampling_key.definition_ast)) - future_metadata_in_zk.sampling_expression = serializeAST(*future_metadata.sampling_key.expression_list_ast); + future_metadata_in_zk.sampling_expression = serializeAST(*extractKeyExpressionList(future_metadata.sampling_key.definition_ast)); if (ast_to_str(future_metadata.partition_key.definition_ast) != ast_to_str(current_metadata->partition_key.definition_ast)) - future_metadata_in_zk.partition_key = serializeAST(*future_metadata.partition_key.expression_list_ast); + future_metadata_in_zk.partition_key = serializeAST(*extractKeyExpressionList(future_metadata.partition_key.definition_ast)); if (ast_to_str(future_metadata.table_ttl.definition_ast) != ast_to_str(current_metadata->table_ttl.definition_ast)) { diff --git a/tests/queries/0_stateless/01526_alter_add_and_modify_order_zookeeper.reference b/tests/queries/0_stateless/01526_alter_add_and_modify_order_zookeeper.reference new file mode 100644 index 00000000000..4063d93d542 --- /dev/null +++ b/tests/queries/0_stateless/01526_alter_add_and_modify_order_zookeeper.reference @@ -0,0 +1,6 @@ +2019-10-01 a 1 aa 1 1 1 +2019-10-01 a 1 aa 1 1 1 0 +CREATE TABLE default.table_for_alter\n(\n `d` Date,\n `a` String,\n `b` UInt8,\n `x` String,\n `y` Int8,\n `version` UInt64,\n `sign` Int8 DEFAULT 1,\n `order` UInt32\n)\nENGINE = ReplicatedVersionedCollapsingMergeTree(\'/clickhouse/tables/01526_alter_add/t1\', \'1\', sign, version)\nPARTITION BY y\nPRIMARY KEY d\nORDER BY (d, order)\nSETTINGS index_granularity = 8192 +2019-10-01 a 1 aa 1 1 1 0 0 +2019-10-02 b 2 bb 2 2 2 1 2 +CREATE TABLE default.table_for_alter\n(\n `d` Date,\n `a` String,\n `b` UInt8,\n `x` String,\n `y` Int8,\n `version` UInt64,\n `sign` Int8 DEFAULT 1,\n `order` UInt32,\n `datum` UInt32\n)\nENGINE = ReplicatedVersionedCollapsingMergeTree(\'/clickhouse/tables/01526_alter_add/t1\', \'1\', sign, version)\nPARTITION BY y\nPRIMARY KEY d\nORDER BY (d, order, datum)\nSETTINGS index_granularity = 8192 diff --git a/tests/queries/0_stateless/01526_alter_add_and_modify_order_zookeeper.sql b/tests/queries/0_stateless/01526_alter_add_and_modify_order_zookeeper.sql new file mode 100644 index 00000000000..b718ba199c1 --- /dev/null +++ b/tests/queries/0_stateless/01526_alter_add_and_modify_order_zookeeper.sql @@ -0,0 +1,38 @@ +DROP TABLE IF EXISTS table_for_alter; + +SET replication_alter_partitions_sync = 2; + +CREATE TABLE table_for_alter +( + `d` Date, + `a` String, + `b` UInt8, + `x` String, + `y` Int8, + `version` UInt64, + `sign` Int8 DEFAULT 1 +) +ENGINE = ReplicatedVersionedCollapsingMergeTree('/clickhouse/tables/01526_alter_add/t1', '1', sign, version) +PARTITION BY y +ORDER BY d +SETTINGS index_granularity = 8192; + +INSERT INTO table_for_alter VALUES(toDate('2019-10-01'), 'a', 1, 'aa', 1, 1, 1); + +SELECT * FROM table_for_alter; + +ALTER TABLE table_for_alter ADD COLUMN order UInt32, MODIFY ORDER BY (d, order); + +SELECT * FROM table_for_alter; + +SHOW CREATE TABLE table_for_alter; + +ALTER TABLE table_for_alter ADD COLUMN datum UInt32, MODIFY ORDER BY (d, order, datum); + +INSERT INTO table_for_alter VALUES(toDate('2019-10-02'), 'b', 2, 'bb', 2, 2, 2, 1, 2); + +SELECT * FROM table_for_alter ORDER BY d; + +SHOW CREATE TABLE table_for_alter; + +DROP TABLE IF EXISTS table_for_alter;