Merge pull request #16011 from ClickHouse/fix_alter_add_and_modify_order_by

Fix alter sorting key for ReplicatedVersionedCollapsingMergeTree

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))
         {

tests/queries/0_stateless/01526_alter_add_and_modify_order_zookeeper.reference

+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

tests/queries/0_stateless/01526_alter_add_and_modify_order_zookeeper.sql

+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;