Merge pull request #35797 from azat/nested-columns-fixes

RFC: Fix ALTER DROP COLUMN of nested column with compact parts

src/Storages/AlterCommands.cpp

@@ -785,7 +785,7 @@ bool AlterCommand::isRequireMutationStage(const StorageInMemoryMetadata & metada
 
     /// Drop alias is metadata alter, in other case mutation is required.
     if (type == DROP_COLUMN)
-        return metadata.columns.hasPhysical(column_name);
+        return metadata.columns.hasColumnOrNested(GetColumnsOptions::AllPhysical, column_name);
 
     if (type != MODIFY_COLUMN || data_type == nullptr)
         return false;

src/Storages/ColumnsDescription.cpp

@@ -397,6 +397,15 @@ NamesAndTypesList ColumnsDescription::getSubcolumns(const String & name_in_stora
     return NamesAndTypesList(range.first, range.second);
 }
 
+NamesAndTypesList ColumnsDescription::getNested(const String & column_name) const
+{
+    auto range = getNameRange(columns, column_name);
+    NamesAndTypesList nested;
+    for (auto & it = range.first; it != range.second; ++it)
+        nested.emplace_back(it->name, it->type);
+    return nested;
+}
+
 void ColumnsDescription::addSubcolumnsToList(NamesAndTypesList & source_list) const
 {
     NamesAndTypesList subcolumns_list;
@@ -603,6 +612,13 @@ bool ColumnsDescription::hasColumnOrSubcolumn(GetColumnsOptions::Kind kind, cons
             || hasSubcolumn(column_name);
 }
 
+bool ColumnsDescription::hasColumnOrNested(GetColumnsOptions::Kind kind, const String & column_name) const
+{
+    auto range = getNameRange(columns, column_name);
+    return range.first != range.second &&
+        defaultKindToGetKind(range.first->default_desc.kind) & kind;
+}
+
 bool ColumnsDescription::hasDefaults() const
 {
     for (const auto & column : columns)

src/Storages/ColumnsDescription.h

@@ -127,7 +127,10 @@ public:
     NamesAndTypesList getEphemeral() const;
     NamesAndTypesList getAllPhysical() const; /// ordinary + materialized.
     NamesAndTypesList getAll() const; /// ordinary + materialized + aliases + ephemeral
+    /// Returns .size0/.null/...
     NamesAndTypesList getSubcolumns(const String & name_in_storage) const;
+    /// Returns column_name.*
+    NamesAndTypesList getNested(const String & column_name) const;
 
     using ColumnTTLs = std::unordered_map<String, ASTPtr>;
     ColumnTTLs getColumnTTLs() const;
@@ -167,6 +170,7 @@ public:
 
     bool hasPhysical(const String & column_name) const;
     bool hasColumnOrSubcolumn(GetColumnsOptions::Kind kind, const String & column_name) const;
+    bool hasColumnOrNested(GetColumnsOptions::Kind kind, const String & column_name) const;
 
     NameAndTypePair getPhysical(const String & column_name) const;
     NameAndTypePair getColumnOrSubcolumn(GetColumnsOptions::Kind kind, const String & column_name) const;

src/Storages/MergeTree/MutateTask.cpp

@@ -80,20 +80,28 @@ static void splitMutationCommands(
             {
                 for_file_renames.push_back(command);
             }
-            else if (part_columns.has(command.column_name))
+            else if (bool has_column = part_columns.has(command.column_name), has_nested_column = part_columns.hasNested(command.column_name); has_column || has_nested_column)
             {
-                if (command.type == MutationCommand::Type::DROP_COLUMN)
+                if (command.type == MutationCommand::Type::DROP_COLUMN || command.type == MutationCommand::Type::RENAME_COLUMN)
                 {
+                    if (has_nested_column)
+                    {
+                        const auto & nested = part_columns.getNested(command.column_name);
+                        assert(!nested.empty());
+                        for (const auto & nested_column : nested)
+                            mutated_columns.emplace(nested_column.name);
+                    }
+                    else
                         mutated_columns.emplace(command.column_name);
                 }
-                else if (command.type == MutationCommand::Type::RENAME_COLUMN)
+
+                if (command.type == MutationCommand::Type::RENAME_COLUMN)
                 {
                     for_interpreter.push_back(
                     {
                         .type = MutationCommand::Type::READ_COLUMN,
                         .column_name = command.rename_to,
                     });
-                    mutated_columns.emplace(command.column_name);
                     part_columns.rename(command.column_name, command.rename_to);
                 }
             }

tests/queries/0_stateless/02260_alter_compact_part_drop_nested_column.sql.j2

@@ -0,0 +1,17 @@
+{# force compact parts and wide #}
+{% for min_bytes_for_wide_part in [100000, 0] %}
+DROP TABLE IF EXISTS compact_alter_{{ min_bytes_for_wide_part }};
+CREATE TABLE compact_alter_{{ min_bytes_for_wide_part }} (d Date, s String, k UInt64) ENGINE=MergeTree() PARTITION BY d ORDER BY k SETTINGS min_bytes_for_wide_part={{ min_bytes_for_wide_part }};
+
+INSERT INTO compact_alter_{{ min_bytes_for_wide_part }} VALUES ('2015-01-01', '2015-01-01 00:00:00', 10);
+
+ALTER TABLE compact_alter_{{ min_bytes_for_wide_part }} ADD COLUMN n.d Int;
+-- force columns creation
+OPTIMIZE TABLE compact_alter_{{ min_bytes_for_wide_part }} FINAL;
+-- this command will not drop n.d from compact part columns.txt
+ALTER TABLE compact_alter_{{ min_bytes_for_wide_part }} DROP COLUMN n;
+-- and now modify column will trigger READ_COLUMN of n.d, and it will bail
+ALTER TABLE compact_alter_{{ min_bytes_for_wide_part }} MODIFY COLUMN s DateTime('UTC') DEFAULT '1970-01-01 00:00:00';
+
+DROP TABLE compact_alter_{{ min_bytes_for_wide_part }};
+{% endfor %}