Merge pull request ClickHouse#86695 from ClickHouse/backport/25.8/86005

Backport ClickHouse#86005 to 25.8: Forbid altering columns whose subcolumns are used in PK or partition expression

Author: clickhouse-gh[bot]

src/Interpreters/MutationsInterpreter.cpp

@@ -533,7 +533,8 @@ static void validateUpdateColumns(
             }
         }
 
-        if (!storage_columns.tryGetColumn(GetColumnsOptions::Ordinary, column_name))
+        auto ordinary_storage_column = storage_columns.tryGetColumn(GetColumnsOptions::Ordinary, column_name);
+        if (!ordinary_storage_column)
         {
             /// Allow to override value of lightweight delete filter virtual column
             if (column_name == RowExistsColumn::name)
@@ -550,6 +551,16 @@ static void validateUpdateColumns(
                 throw Exception(ErrorCodes::NO_SUCH_COLUMN_IN_TABLE, "There is no column {} in table", backQuote(column_name));
             }
         }
+        else
+        {
+            /// Check if we have a subcolumn of this column as a key column.
+            for (const auto & key_column : key_columns)
+            {
+                auto [key_column_name, key_subcolumn_name] = Nested::splitName(key_column);
+                if (key_column_name == column_name && ordinary_storage_column->type->hasSubcolumn(key_subcolumn_name))
+                    throw Exception(ErrorCodes::CANNOT_UPDATE_COLUMN, "Cannot UPDATE column {} because its subcolumn {} is a key column", backQuote(column_name), backQuote(key_column));
+            }
+        }
     }
 }
 

src/Storages/MergeTree/MergeTreeData.cpp

@@ -4018,6 +4018,11 @@ void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, Context
     /// Columns to check that the type change is safe for partition key.
     NameSet columns_alter_type_check_safe_for_partition;
 
+    /// Columns with subcolumns used in primary key or partition key.
+    std::unordered_map<String, std::vector<String>> column_to_subcolumns_used_in_keys;
+
+    const auto & old_columns = old_metadata.getColumns();
+
     if (old_metadata.hasPartitionKey())
     {
         /// Forbid altering columns inside partition key expressions because it can change partition ID format.
@@ -4030,7 +4035,13 @@ void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, Context
 
         /// But allow to alter columns without expressions under certain condition.
         for (const String & col : partition_key_expr->getRequiredColumns())
-            columns_alter_type_check_safe_for_partition.insert(col);
+        {
+            auto storage_column = old_columns.tryGetColumnOrSubcolumn(GetColumnsOptions::All, col);
+            if (storage_column && storage_column->isSubcolumn())
+                column_to_subcolumns_used_in_keys[storage_column->getNameInStorage()].push_back(backQuoteIfNeed(col));
+            else
+                columns_alter_type_check_safe_for_partition.insert(col);
+        }
     }
 
     if (old_metadata.hasSortingKey())
@@ -4042,7 +4053,13 @@ void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, Context
                 columns_alter_type_forbidden.insert(child->result_name);
         }
         for (const String & col : sorting_key_expr->getRequiredColumns())
-            columns_alter_type_metadata_only.insert(col);
+        {
+            auto storage_column = old_columns.tryGetColumnOrSubcolumn(GetColumnsOptions::All, col);
+            if (storage_column && storage_column->isSubcolumn())
+                column_to_subcolumns_used_in_keys[storage_column->getNameInStorage()].push_back(backQuoteIfNeed(col));
+            else
+                columns_alter_type_metadata_only.insert(col);
+        }
 
         /// We don't process sample_by_ast separately because it must be among the primary key columns
         /// and we don't process primary_key_expr separately because it is a prefix of sorting_key_expr.
@@ -4172,6 +4189,15 @@ void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, Context
                                 backQuoteIfNeed(command.column_name));
             }
 
+            if (column_to_subcolumns_used_in_keys.contains(command.column_name))
+            {
+                throw Exception(
+                    ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN,
+                    "Trying to ALTER RENAME column {} whose subcolumns ({}) are part of key expression",
+                    backQuoteIfNeed(command.column_name),
+                    boost::join(column_to_subcolumns_used_in_keys[command.column_name], ", "));
+            }
+
             /// Don't check columns in indices here. RENAME works fine with index columns.
 
             if (auto it = columns_in_projections.find(command.column_name); it != columns_in_projections.end())
@@ -4191,6 +4217,15 @@ void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, Context
                     "Trying to ALTER DROP key {} column which is a part of key expression", backQuoteIfNeed(command.column_name));
             }
 
+            if (column_to_subcolumns_used_in_keys.contains(command.column_name))
+            {
+                throw Exception(
+                    ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN,
+                    "Trying to ALTER DROP column {} whose subcolumns ({}) are part of key expression",
+                    backQuoteIfNeed(command.column_name),
+                    boost::join(column_to_subcolumns_used_in_keys[command.column_name], ", "));
+            }
+
             /// Don't check columns in indices or projections here. If required columns of indices
             /// or projections get dropped, it will be checked later in AlterCommands::apply. This
             /// allows projections with * to drop columns. One example can be found in
@@ -4239,6 +4274,15 @@ void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, Context
                 throw Exception(ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN, "ALTER of key column {} is forbidden",
                     backQuoteIfNeed(command.column_name));
 
+            if (column_to_subcolumns_used_in_keys.contains(command.column_name))
+            {
+                throw Exception(
+                    ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN,
+                    "Trying to ALTER column {} whose subcolumns ({}) are part of key expression",
+                    backQuoteIfNeed(command.column_name),
+                    boost::join(column_to_subcolumns_used_in_keys[command.column_name], ", "));
+            }
+
             if (auto it = columns_in_indices.find(command.column_name); it != columns_in_indices.end())
             {
                 throw Exception(

tests/queries/0_stateless/03597_alter_column_with_subcolumn_in_key.reference

@@ -0,0 +1,14 @@
+drop table if exists test;
+create table test (id UInt32, t Tuple(a UInt32)) engine=MergeTree order by t.a;
+insert into test select 1, tuple(1);
+alter table test update t = tuple(2) where 1; -- {serverError CANNOT_UPDATE_COLUMN}
+alter table test modify column t Tuple(a String); -- {serverError ALTER_OF_COLUMN_IS_FORBIDDEN}
+drop table test;
+
+drop table if exists test;
+create table test (id UInt32, json JSON) engine=MergeTree order by json.a::Int64;
+insert into test select 1, '{"a" : 42}';
+alter table test update json = '{}' where 1; -- {serverError CANNOT_UPDATE_COLUMN}
+alter table test modify column json JSON(a String); -- {serverError ALTER_OF_COLUMN_IS_FORBIDDEN}
+drop table test;
+