From ac9f0ff4da147e55b07eda3c765f56bcbf93498d Mon Sep 17 00:00:00 2001
From: alesapin <alesapin@gmail.com>
Date: Wed, 30 Sep 2020 22:44:35 +0300
Subject: [PATCH] More strict check for version column

---
 src/Storages/MergeTree/MergeTreeData.cpp      | 53 +++++++++++++++----
 ...ersion_versioned_collapsing_merge_tree.sql |  5 ++
 2 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/src/Storages/MergeTree/MergeTreeData.cpp b/src/Storages/MergeTree/MergeTreeData.cpp
index 83fc75d662e..0aee7a25875 100644
--- a/src/Storages/MergeTree/MergeTreeData.cpp
+++ b/src/Storages/MergeTree/MergeTreeData.cpp
@@ -1416,6 +1416,49 @@ void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, const S
             getPartitionIDFromQuery(command.partition, global_context);
         }
 
+        /// Some type changes for version column is allowed despite it's a part of sorting key
+        if (command.type == AlterCommand::MODIFY_COLUMN && command.column_name == merging_params.version_column)
+        {
+            auto new_type = command.data_type;
+            auto old_type = old_types[command.column_name];
+            /// Check new type can be used as version
+            if (!new_type->canBeUsedAsVersion())
+                throw Exception("Cannot alter version column " + backQuoteIfNeed(command.column_name) +
+                    " to type " + new_type->getName() +
+                    " because version column must be of an integer type or of type Date or DateTime"
+                    , ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN);
+
+            auto which_new_type = WhichDataType(new_type);
+            auto which_old_type = WhichDataType(old_type);
+
+            /// Check alter to different sign or float -> int and so on
+            if ((which_old_type.isInt() && !which_new_type.isInt())
+                || (which_old_type.isUInt() && !which_new_type.isUInt())
+                || (which_old_type.isDate() && !which_new_type.isDate())
+                || (which_old_type.isDateTime() && !which_new_type.isDateTime())
+                || (which_old_type.isFloat() && !which_new_type.isFloat()))
+            {
+                throw Exception("Cannot alter version column " + backQuoteIfNeed(command.column_name) +
+                    " from type " + old_type->getName() +
+                    " to type " + new_type->getName() + " because new type will change sort order of version column." +
+                    " The only possible conversion is expansion of the number of bytes of the current type."
+                    , ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN);
+            }
+
+            /// Check alter to smaller size: UInt64 -> UInt32 and so on
+            if (new_type->getSizeOfValueInMemory() < old_type->getSizeOfValueInMemory())
+            {
+                throw Exception("Cannot alter version column " + backQuoteIfNeed(command.column_name) +
+                    " from type " + old_type->getName() +
+                    " to type " + new_type->getName() + " because new type is smaller than current in the number of bytes." +
+                    " The only possible conversion is expansion of the number of bytes of the current type."
+                    , ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN);
+            }
+
+            /// Positive case, alter allowed
+            continue;
+        }
+
         if (command.type == AlterCommand::MODIFY_ORDER_BY && !is_custom_partitioned)
         {
             throw Exception(
@@ -1458,16 +1501,6 @@ void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, const S
         }
         else if (command.isRequireMutationStage(getInMemoryMetadata()))
         {
-            /// Type change for version column is allowed despite it's a part of sorting key
-            if (command.type == AlterCommand::MODIFY_COLUMN && command.column_name == merging_params.version_column)
-            {
-                if (!command.data_type->canBeUsedAsVersion())
-                    throw Exception("Cannot alter version column " + backQuoteIfNeed(command.column_name) +
-                        " to type " + command.data_type->getName() +
-                        " because version column must be of an integer type or of type Date or DateTime"
-                        , ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN);
-                continue;
-            }
             /// This alter will override data on disk. Let's check that it doesn't
             /// modify immutable column.
             if (columns_alter_type_forbidden.count(command.column_name))
diff --git a/tests/queries/0_stateless/01511_alter_version_versioned_collapsing_merge_tree.sql b/tests/queries/0_stateless/01511_alter_version_versioned_collapsing_merge_tree.sql
index 62cb4e77640..8f0b2d12ab0 100644
--- a/tests/queries/0_stateless/01511_alter_version_versioned_collapsing_merge_tree.sql
+++ b/tests/queries/0_stateless/01511_alter_version_versioned_collapsing_merge_tree.sql
@@ -37,5 +37,10 @@ INSERT INTO TABLE table_with_version VALUES(3, '3', 65555, -1);
 SELECT * FROM table_with_version FINAL ORDER BY key;
 
 ALTER TABLE table_with_version MODIFY COLUMN version String; --{serverError 524}
+ALTER TABLE table_with_version MODIFY COLUMN version Int64; --{serverError 524}
+ALTER TABLE table_with_version MODIFY COLUMN version UInt16; --{serverError 524}
+ALTER TABLE table_with_version MODIFY COLUMN version Float64; --{serverError 524}
+ALTER TABLE table_with_version MODIFY COLUMN version Date; --{serverError 524}
+ALTER TABLE table_with_version MODIFY COLUMN version DateTime; --{serverError 524}
 
 DROP TABLE IF EXISTS table_with_version;