Merge pull request #5167 from yandex/remove-useless-code-in-columns-description

Removed bad and useless code in ColumnsDescription

dbms/programs/server/TCPHandler.cpp

@@ -370,8 +370,8 @@ void TCPHandler::processInsertQuery(const Settings & global_settings)
     if (client_revision >= DBMS_MIN_REVISION_WITH_COLUMN_DEFAULTS_METADATA)
     {
         const auto & db_and_table = query_context->getInsertionTable();
-        if (auto * columns = ColumnsDescription::loadFromContext(*query_context, db_and_table.first, db_and_table.second))
-            sendTableColumns(*columns);
+        if (query_context->getSettingsRef().input_format_defaults_for_omitted_fields)
+            sendTableColumns(query_context->getTable(db_and_table.first, db_and_table.second)->getColumns());
     }
 
     /// Send block to the client - table structure.

dbms/src/DataStreams/InputStreamFromASTInsertQuery.cpp

@@ -7,6 +7,8 @@
 #include <DataStreams/InputStreamFromASTInsertQuery.h>
 #include <DataStreams/AddingDefaultsBlockInputStream.h>
 #include <Storages/ColumnsDescription.h>
+#include <Storages/IStorage.h>
+
 
 namespace DB
 {
@@ -49,10 +51,10 @@ InputStreamFromASTInsertQuery::InputStreamFromASTInsertQuery(
 
     res_stream = context.getInputFormat(format, *input_buffer_contacenated, header, context.getSettings().max_insert_block_size);
 
-    auto columns_description = ColumnsDescription::loadFromContext(context, ast_insert_query->database, ast_insert_query->table);
-    if (columns_description)
+    if (context.getSettingsRef().input_format_defaults_for_omitted_fields)
     {
-        auto column_defaults = columns_description->getDefaults();
+        StoragePtr storage = context.getTable(ast_insert_query->database, ast_insert_query->table);
+        auto column_defaults = storage->getColumns().getDefaults();
         if (!column_defaults.empty())
             res_stream = std::make_shared<AddingDefaultsBlockInputStream>(res_stream, column_defaults, context);
     }

dbms/src/Functions/GatherUtils/Selectors.h

@@ -4,7 +4,7 @@
 #include <Functions/GatherUtils/ArraySourceVisitor.h>
 #include <Functions/GatherUtils/ArraySinkVisitor.h>
 #include <Functions/GatherUtils/ValueSourceVisitor.h>
-#include <Core/TypeListNumber.h>
+
 
 namespace DB::GatherUtils
 {

dbms/src/Storages/AlterCommands.cpp

@@ -203,30 +203,31 @@ void AlterCommand::apply(ColumnsDescription & columns_description, IndicesDescri
     }
     else if (type == MODIFY_COLUMN)
     {
-        ColumnDescription & column = columns_description.get(column_name);
-
-        if (codec)
+        columns_description.modify(column_name, [&](ColumnDescription & column)
         {
-            /// User doesn't specify data type, it means that datatype doesn't change
-            /// let's use info about old type
-            if (data_type == nullptr)
-                codec->useInfoAboutType(column.type);
-            column.codec = codec;
-        }
+            if (codec)
+            {
+                /// User doesn't specify data type, it means that datatype doesn't change
+                /// let's use info about old type
+                if (data_type == nullptr)
+                    codec->useInfoAboutType(column.type);
+                column.codec = codec;
+            }
 
-        if (!is_mutable())
-        {
-            column.comment = comment;
-            return;
-        }
+            if (!is_mutable())
+            {
+                column.comment = comment;
+                return;
+            }
 
-        if (ttl)
-            column.ttl = ttl;
+            if (ttl)
+                column.ttl = ttl;
 
-        column.type = data_type;
+            column.type = data_type;
 
-        column.default_desc.kind = default_kind;
-        column.default_desc.expression = default_expression;
+            column.default_desc.kind = default_kind;
+            column.default_desc.expression = default_expression;
+        });
     }
     else if (type == MODIFY_ORDER_BY)
     {
@@ -241,7 +242,7 @@ void AlterCommand::apply(ColumnsDescription & columns_description, IndicesDescri
     }
     else if (type == COMMENT_COLUMN)
     {
-        columns_description.get(column_name).comment = comment;
+        columns_description.modify(column_name, [&](ColumnDescription & column) { column.comment = comment; });
     }
     else if (type == ADD_INDEX)
     {

dbms/src/Storages/ColumnsDescription.cpp

@@ -21,8 +21,6 @@
 #include <Common/typeid_cast.h>
 #include <Compression/CompressionFactory.h>
 
-#include <optional>
-
 
 namespace DB
 {
@@ -123,40 +121,11 @@ ColumnsDescription::ColumnsDescription(NamesAndTypesList ordinary)
         add(ColumnDescription(std::move(elem.name), std::move(elem.type)));
 }
 
-ColumnsDescription::ColumnsDescription(const ColumnsDescription & other)
-{
-    *this = other;
-}
-
-ColumnsDescription & ColumnsDescription::operator=(const ColumnsDescription & other)
-{
-    if (&other != this)
-    {
-        columns = other.columns;
-        name_to_column.clear();
-        for (auto it = columns.begin(); it != columns.end(); ++it)
-            name_to_column.emplace(it->name, it);
-    }
-    return *this;
-}
-
-ColumnsDescription::ColumnsDescription(ColumnsDescription && other) noexcept {
-    *this = std::move(other);
-}
-
-ColumnsDescription & ColumnsDescription::operator=(ColumnsDescription && other) noexcept
-{
-    assert(&other != this);
-    columns = std::move(other.columns);
-    name_to_column = std::move(other.name_to_column);
-    return *this;
-}
-
 
 /// We are trying to find first column from end with name `column_name` or with a name beginning with `column_name` and ".".
 /// For example "fruits.bananas"
 /// names are considered the same if they completely match or `name_without_dot` matches the part of the name to the point
-static auto getNameRange(const std::list<ColumnDescription> & columns, const String & name_without_dot)
+static auto getNameRange(const ColumnsDescription::Container & columns, const String & name_without_dot)
 {
     String name_with_dot = name_without_dot + ".";
 
@@ -201,8 +170,7 @@ void ColumnsDescription::add(ColumnDescription column, const String & after_colu
         insert_it = range.second;
     }
 
-    auto it = columns.insert(insert_it, std::move(column));
-    name_to_column.emplace(it->name, it);
+    columns.get<0>().insert(insert_it, std::move(column));
 }
 
 void ColumnsDescription::remove(const String & column_name)
@@ -213,10 +181,7 @@ void ColumnsDescription::remove(const String & column_name)
             ErrorCodes::NO_SUCH_COLUMN_IN_TABLE);
 
     for (auto list_it = range.first; list_it != range.second;)
-    {
-        name_to_column.erase(list_it->name);
-        list_it = columns.erase(list_it);
-    }
+        list_it = columns.get<0>().erase(list_it);
 }
 
 
@@ -239,8 +204,7 @@ void ColumnsDescription::flattenNested()
         }
 
         ColumnDescription column = std::move(*it);
-        it = columns.erase(it);
-        name_to_column.erase(column.name);
+        it = columns.get<0>().erase(it);
 
         const DataTypes & elements = type_tuple->getElements();
         const Strings & names = type_tuple->getElementNames();
@@ -253,8 +217,7 @@ void ColumnsDescription::flattenNested()
             nested_column.name = Nested::concatenateName(column.name, names[i]);
             nested_column.type = std::make_shared<DataTypeArray>(elements[i]);
 
-            auto inserted_it = columns.insert(it, std::move(nested_column));
-            name_to_column.emplace(inserted_it->name, inserted_it);
+            columns.get<0>().insert(it, std::move(nested_column));
         }
     }
 }
@@ -264,10 +227,8 @@ NamesAndTypesList ColumnsDescription::getOrdinary() const
 {
     NamesAndTypesList ret;
     for (const auto & col : columns)
-    {
         if (col.default_desc.kind == ColumnDefaultKind::Default)
             ret.emplace_back(col.name, col.type);
-    }
     return ret;
 }
 
@@ -275,10 +236,8 @@ NamesAndTypesList ColumnsDescription::getMaterialized() const
 {
     NamesAndTypesList ret;
     for (const auto & col : columns)
-    {
         if (col.default_desc.kind == ColumnDefaultKind::Materialized)
             ret.emplace_back(col.name, col.type);
-    }
     return ret;
 }
 
@@ -286,10 +245,8 @@ NamesAndTypesList ColumnsDescription::getAliases() const
 {
     NamesAndTypesList ret;
     for (const auto & col : columns)
-    {
         if (col.default_desc.kind == ColumnDefaultKind::Alias)
             ret.emplace_back(col.name, col.type);
-    }
     return ret;
 }
 
@@ -304,8 +261,7 @@ NamesAndTypesList ColumnsDescription::getAll() const
 
 bool ColumnsDescription::has(const String & column_name) const
 {
-    auto it = name_to_column.find(column_name);
-    return it != name_to_column.end();
+    return columns.get<1>().find(column_name) != columns.get<1>().end();
 }
 
 bool ColumnsDescription::hasNested(const String & column_name) const
@@ -314,24 +270,14 @@ bool ColumnsDescription::hasNested(const String & column_name) const
     return range.first != range.second && range.first->name.length() > column_name.length();
 }
 
-ColumnDescription & ColumnsDescription::get(const String & column_name)
-{
-    auto it = name_to_column.find(column_name);
-    if (it == name_to_column.end())
-        throw Exception("There is no column " + column_name + " in table.",
-            ErrorCodes::NO_SUCH_COLUMN_IN_TABLE);
-
-    return *it->second;
-}
-
 const ColumnDescription & ColumnsDescription::get(const String & column_name) const
 {
-    auto it = name_to_column.find(column_name);
-    if (it == name_to_column.end())
+    auto it = columns.get<1>().find(column_name);
+    if (it == columns.get<1>().end())
         throw Exception("There is no column " + column_name + " in table.",
             ErrorCodes::NO_SUCH_COLUMN_IN_TABLE);
 
-    return *it->second;
+    return *it;
 }
 
 
@@ -339,10 +285,8 @@ NamesAndTypesList ColumnsDescription::getAllPhysical() const
 {
     NamesAndTypesList ret;
     for (const auto & col : columns)
-    {
         if (col.default_desc.kind != ColumnDefaultKind::Alias)
             ret.emplace_back(col.name, col.type);
-    }
     return ret;
 }
 
@@ -350,25 +294,23 @@ Names ColumnsDescription::getNamesOfPhysical() const
 {
     Names ret;
     for (const auto & col : columns)
-    {
         if (col.default_desc.kind != ColumnDefaultKind::Alias)
             ret.emplace_back(col.name);
-    }
     return ret;
 }
 
 NameAndTypePair ColumnsDescription::getPhysical(const String & column_name) const
 {
-    auto it = name_to_column.find(column_name);
-    if (it == name_to_column.end() || it->second->default_desc.kind == ColumnDefaultKind::Alias)
+    auto it = columns.get<1>().find(column_name);
+    if (it == columns.get<1>().end() || it->default_desc.kind == ColumnDefaultKind::Alias)
         throw Exception("There is no physical column " + column_name + " in table.", ErrorCodes::NO_SUCH_COLUMN_IN_TABLE);
-    return NameAndTypePair(it->second->name, it->second->type);
+    return NameAndTypePair(it->name, it->type);
 }
 
 bool ColumnsDescription::hasPhysical(const String & column_name) const
 {
-    auto it = name_to_column.find(column_name);
-    return it != name_to_column.end() && it->second->default_desc.kind != ColumnDefaultKind::Alias;
+    auto it = columns.get<1>().find(column_name);
+    return it != columns.get<1>().end() && it->default_desc.kind != ColumnDefaultKind::Alias;
 }
 
 
@@ -376,25 +318,23 @@ ColumnDefaults ColumnsDescription::getDefaults() const
 {
     ColumnDefaults ret;
     for (const auto & column : columns)
-    {
         if (column.default_desc.expression)
             ret.emplace(column.name, column.default_desc);
-    }
 
     return ret;
 }
 
 bool ColumnsDescription::hasDefault(const String & column_name) const
 {
-    auto it = name_to_column.find(column_name);
-    return it != name_to_column.end() && it->second->default_desc.expression;
+    auto it = columns.get<1>().find(column_name);
+    return it != columns.get<1>().end() && it->default_desc.expression;
 }
 
 std::optional<ColumnDefault> ColumnsDescription::getDefault(const String & column_name) const
 {
-    auto it = name_to_column.find(column_name);
-    if (it != name_to_column.end() && it->second->default_desc.expression)
-        return it->second->default_desc;
+    auto it = columns.get<1>().find(column_name);
+    if (it != columns.get<1>().end() && it->default_desc.expression)
+        return it->default_desc;
 
     return {};
 }
@@ -402,12 +342,12 @@ std::optional<ColumnDefault> ColumnsDescription::getDefault(const String & colum
 
 CompressionCodecPtr ColumnsDescription::getCodecOrDefault(const String & column_name, CompressionCodecPtr default_codec) const
 {
-    const auto it = name_to_column.find(column_name);
+    const auto it = columns.get<1>().find(column_name);
 
-    if (it == name_to_column.end() || !it->second->codec)
+    if (it == columns.get<1>().end() || !it->codec)
         return default_codec;
 
-    return it->second->codec;
+    return it->codec;
 }
 
 CompressionCodecPtr ColumnsDescription::getCodecOrDefault(const String & column_name) const
@@ -419,11 +359,8 @@ ColumnsDescription::ColumnTTLs ColumnsDescription::getColumnTTLs() const
 {
     ColumnTTLs ret;
     for (const auto & column : columns)
-    {
         if (column.ttl)
             ret.emplace(column.name, column.ttl);
-    }
-
     return ret;
 }
 
@@ -464,18 +401,4 @@ ColumnsDescription ColumnsDescription::parse(const String & str)
     return result;
 }
 
-const ColumnsDescription * ColumnsDescription::loadFromContext(const Context & context, const String & db, const String & table)
-{
-    if (context.getSettingsRef().input_format_defaults_for_omitted_fields)
-    {
-        if (context.isTableExist(db, table))
-        {
-            StoragePtr storage = context.getTable(db, table);
-            return &storage->getColumns();
-        }
-    }
-
-    return nullptr;
-}
-
 }

dbms/src/Storages/ColumnsDescription.h

@@ -2,15 +2,28 @@
 
 #include <Core/NamesAndTypes.h>
 #include <Core/Names.h>
-#include <Storages/ColumnDefault.h>
 #include <Core/Block.h>
+#include <Common/Exception.h>
+#include <Storages/ColumnDefault.h>
 #include <Storages/ColumnCodec.h>
 #include <optional>
 
+#include <boost/multi_index_container.hpp>
+#include <boost/multi_index/sequenced_index.hpp>
+#include <boost/multi_index/ordered_index.hpp>
+#include <boost/multi_index/member.hpp>
+
 
 namespace DB
 {
 
+namespace ErrorCodes
+{
+    extern const int LOGICAL_ERROR;
+}
+
+
+/// Description of a single table column (in CREATE TABLE for example).
 struct ColumnDescription
 {
     String name;
@@ -30,15 +43,13 @@ struct ColumnDescription
     void readText(ReadBuffer & buf);
 };
 
+
+/// Description of multiple table columns (in CREATE TABLE for example).
 class ColumnsDescription
 {
 public:
     ColumnsDescription() = default;
     explicit ColumnsDescription(NamesAndTypesList ordinary_);
-    ColumnsDescription(const ColumnsDescription & other);
-    ColumnsDescription & operator=(const ColumnsDescription & other);
-    ColumnsDescription(ColumnsDescription &&) noexcept;
-    ColumnsDescription & operator=(ColumnsDescription && other) noexcept;
 
     /// `after_column` can be a Nested column name;
     void add(ColumnDescription column, const String & after_column = String());
@@ -50,8 +61,8 @@ public:
     bool operator==(const ColumnsDescription & other) const { return columns == other.columns; }
     bool operator!=(const ColumnsDescription & other) const { return !(*this == other); }
 
-    std::list<ColumnDescription>::const_iterator begin() const { return columns.begin(); }
-    std::list<ColumnDescription>::const_iterator end() const { return columns.end(); }
+    auto begin() const { return columns.begin(); }
+    auto end() const { return columns.end(); }
 
     NamesAndTypesList getOrdinary() const;
     NamesAndTypesList getMaterialized() const;
@@ -64,9 +75,18 @@ public:
 
     bool has(const String & column_name) const;
     bool hasNested(const String & column_name) const;
-    ColumnDescription & get(const String & column_name);
     const ColumnDescription & get(const String & column_name) const;
 
+    template <typename F>
+    void modify(const String & column_name, F && f)
+    {
+        auto it = columns.get<1>().find(column_name);
+        if (it == columns.get<1>().end())
+            throw Exception("Cannot find column " + column_name + " in ColumnsDescription", ErrorCodes::LOGICAL_ERROR);
+        if (!columns.get<1>().modify(it, std::forward<F>(f)))
+            throw Exception("Cannot modify ColumnDescription for column " + column_name + ": column name cannot be changed", ErrorCodes::LOGICAL_ERROR);
+    }
+
     /// ordinary + materialized.
     NamesAndTypesList getAllPhysical() const;
     Names getNamesOfPhysical() const;
@@ -83,11 +103,15 @@ public:
     String toString() const;
     static ColumnsDescription parse(const String & str);
 
-    static const ColumnsDescription * loadFromContext(const Context & context, const String & db, const String & table);
+    /// Keep the sequence of columns and allow to lookup by name.
+    using Container = boost::multi_index_container<
+        ColumnDescription,
+        boost::multi_index::indexed_by<
+            boost::multi_index::sequenced<>,
+            boost::multi_index::ordered_unique<boost::multi_index::member<ColumnDescription, String, &ColumnDescription::name>>>>;
 
 private:
-    std::list<ColumnDescription> columns;
-    std::unordered_map<String, std::list<ColumnDescription>::iterator> name_to_column;
+    Container columns;
 };
 
 }