address review comments

2024-11-26 09:32:01 +00:00 · 2024-03-05 16:03:02 +00:00 · 2024-03-05 16:03:02 +00:00 · 7fe7f3a79d
commit 7fe7f3a79d
parent 1b9e6c936e
30 changed files with 38 additions and 39 deletions
--- a/src/Interpreters/MutationsInterpreter.cpp
+++ b/src/Interpreters/MutationsInterpreter.cpp
@ -7,7 +7,7 @@
 #include <Storages/MergeTree/MergeTreeData.h>
 #include <Storages/MergeTree/StorageFromMergeTreeDataPart.h>
 #include <Storages/StorageMergeTree.h>
-#include <Storages/MergeTreeVirtualColumns.h>
+#include <Storages/MergeTree/MergeTreeVirtualColumns.h>
 #include <Processors/Transforms/FilterTransform.h>
 #include <Processors/Transforms/ExpressionTransform.h>
 #include <Processors/Transforms/CreatingSetsTransform.h>
--- a/src/Processors/Merges/Algorithms/SummingSortedAlgorithm.cpp
+++ b/src/Processors/Merges/Algorithms/SummingSortedAlgorithm.cpp
@ -12,7 +12,7 @@
 #include <DataTypes/NestedUtils.h>
 #include <DataTypes/DataTypeLowCardinality.h>
 #include <IO/WriteHelpers.h>
-#include <Storages/MergeTreeVirtualColumns.h>
+#include <Storages/MergeTree/MergeTreeVirtualColumns.h>


 namespace DB
--- a/src/Processors/QueryPlan/Optimizations/optimizeUseNormalProjection.cpp
+++ b/src/Processors/QueryPlan/Optimizations/optimizeUseNormalProjection.cpp
@ -135,7 +135,7 @@ bool optimizeUseNormalProjections(Stack & stack, QueryPlan::Nodes & nodes)
    std::list<NormalProjectionCandidate> candidates;
    NormalProjectionCandidate * best_candidate = nullptr;

-    const Names & required_columns = reading->getRealColumnNames();
+    const Names & required_columns = reading->getAllColumnNames();
    const auto & parts = reading->getParts();
    const auto & alter_conversions = reading->getAlterConvertionsForParts();
    const auto & query_info = reading->getQueryInfo();
--- a/src/Processors/QueryPlan/ReadFromMergeTree.h
+++ b/src/Processors/QueryPlan/ReadFromMergeTree.h
@ -133,7 +133,7 @@ public:
    void describeActions(JSONBuilder::JSONMap & map) const override;
    void describeIndexes(JSONBuilder::JSONMap & map) const override;

-    const Names & getRealColumnNames() const { return all_column_names; }
+    const Names & getAllColumnNames() const { return all_column_names; }

    StorageID getStorageID() const { return data.getStorageID(); }
    UInt64 getSelectedParts() const { return selected_parts; }
--- a/src/Storages/IStorage.h
+++ b/src/Storages/IStorage.h
@ -214,9 +214,9 @@ public:
        metadata.set(std::make_unique<StorageInMemoryMetadata>(metadata_));
    }

-    void setVirtuals(const VirtualColumnsDescription & virtuals_)
+    void setVirtuals(VirtualColumnsDescription virtuals_)
    {
-        virtuals.set(std::make_unique<VirtualColumnsDescription>(virtuals_));
+        virtuals.set(std::make_unique<VirtualColumnsDescription>(std::move(virtuals_)));
    }

    /// Return list of virtual columns (like _part, _table, etc). In the vast
@ -275,7 +275,7 @@ private:
    /// Multiversion storage metadata. Allows to read/write storage metadata without locks.
    MultiVersionStorageMetadataPtr metadata;

-    /// TODO:
+    /// Description of virtual columns. Optional, may be set in constructor.
    MultiVersionVirtualsDescriptionPtr virtuals;

 protected:
--- a/src/Storages/LiveView/StorageLiveView.cpp
+++ b/src/Storages/LiveView/StorageLiveView.cpp
@ -220,7 +220,7 @@ StorageLiveView::StorageLiveView(

    VirtualColumnsDescription virtuals;
    virtuals.addEphemeral("_version", std::make_shared<DataTypeUInt64>(), "");
-    setVirtuals(virtuals);
+    setVirtuals(std::move(virtuals));

    if (!query.select)
        throw Exception(ErrorCodes::INCORRECT_QUERY, "SELECT query is not specified for {}", getName());
--- a/src/Storages/MergeTree/IMergeTreeReader.cpp
+++ b/src/Storages/MergeTree/IMergeTreeReader.cpp
@ -1,6 +1,6 @@
 #include <Storages/MergeTree/IMergeTreeReader.h>
 #include <Storages/MergeTree/MergeTreeReadTask.h>
-#include <Storages/MergeTreeVirtualColumns.h>
+#include <Storages/MergeTree/MergeTreeVirtualColumns.h>
 #include <Storages/MergeTree/LoadedMergeTreeDataPartInfoForReader.h>
 #include <DataTypes/NestedUtils.h>
 #include <Common/escapeForFileName.h>
@ -68,6 +68,8 @@ const IMergeTreeReader::ValueSizeMap & IMergeTreeReader::getAvgValueSizeHints()

 void IMergeTreeReader::fillVirtualColumns(Columns & columns, size_t rows) const
 {
+    chassert(columns.size() == requested_columns.size());
+
    const auto * loaded_part_info = typeid_cast<const LoadedMergeTreeDataPartInfoForReader *>(data_part_info_for_read.get());
    if (!loaded_part_info)
        throw Exception(ErrorCodes::NOT_IMPLEMENTED, "Filling of virtual columns is supported only for LoadedMergeTreeDataPartInfoForReader");
--- a/src/Storages/MergeTree/IMergeTreeReader.h
+++ b/src/Storages/MergeTree/IMergeTreeReader.h
@ -117,7 +117,7 @@ private:
    /// Actual columns description in part.
    const ColumnsDescription & part_columns;

-    /// TODO:
+    /// Fields of virtual columns that were filled in previous stages.
    VirtualFields virtual_fields;
 };

--- a/src/Storages/MergeTree/MergeTask.h
+++ b/src/Storages/MergeTree/MergeTask.h
@ -15,7 +15,7 @@

 #include <QueryPipeline/QueryPipeline.h>

-#include <Storages/MergeTreeVirtualColumns.h>
+#include <Storages/MergeTree/MergeTreeVirtualColumns.h>
 #include <Storages/MergeTree/ColumnSizeEstimator.h>
 #include <Storages/MergeTree/FutureMergedMutatedPart.h>
 #include <Storages/MergeTree/IExecutableTask.h>
--- a/src/Storages/MergeTree/MergeTreeBlockReadUtils.cpp
+++ b/src/Storages/MergeTree/MergeTreeBlockReadUtils.cpp
@ -6,7 +6,7 @@
 #include <Core/NamesAndTypes.h>
 #include <Common/checkStackSize.h>
 #include <Common/typeid_cast.h>
-#include <Storages/MergeTreeVirtualColumns.h>
+#include <Storages/MergeTree/MergeTreeVirtualColumns.h>
 #include <Storages/MergeTree/MergeTreeSelectProcessor.h>
 #include <Columns/ColumnConst.h>
 #include <IO/WriteBufferFromString.h>
--- a/src/Storages/MergeTree/MergeTreeData.cpp
+++ b/src/Storages/MergeTree/MergeTreeData.cpp
@ -67,7 +67,7 @@
 #include <Processors/QueryPlan/QueryIdHolder.h>
 #include <Processors/QueryPlan/ReadFromMergeTree.h>
 #include <Storages/AlterCommands.h>
-#include <Storages/MergeTreeVirtualColumns.h>
+#include <Storages/MergeTree/MergeTreeVirtualColumns.h>
 #include <Storages/Freeze.h>
 #include <Storages/MergeTree/DataPartStorageOnDiskFull.h>
 #include <Storages/MergeTree/MergeTreeDataPartBuilder.h>
--- a/src/Storages/MergeTree/MergeTreePrefetchedReadPool.cpp
+++ b/src/Storages/MergeTree/MergeTreePrefetchedReadPool.cpp
@ -9,7 +9,7 @@
 #include <Storages/MergeTree/MergeTreePrefetchedReadPool.h>
 #include <Storages/MergeTree/MergeTreeRangeReader.h>
 #include <Storages/MergeTree/RangesInDataPart.h>
-#include <Storages/MergeTreeVirtualColumns.h>
+#include <Storages/MergeTree/MergeTreeVirtualColumns.h>
 #include <base/getThreadId.h>
 #include <Common/ElapsedTimeProfileEventIncrement.h>
 #include <Common/logger_useful.h>
--- a/src/Storages/MergeTree/MergeTreeRangeReader.cpp
+++ b/src/Storages/MergeTree/MergeTreeRangeReader.cpp
@ -1153,13 +1153,13 @@ MergeTreeRangeReader::ReadResult MergeTreeRangeReader::startReadingChain(size_t
        size_t pos = read_sample_block.getPositionByName("_part_offset");
        chassert(pos < result.columns.size());
        chassert(result.columns[pos] == nullptr);
-        result.columns[pos] = fillPartOffsetColumn(result, leading_begin_part_offset, leading_end_part_offset);
+        result.columns[pos] = createPartOffsetColumn(result, leading_begin_part_offset, leading_end_part_offset);
    }

    return result;
 }

-ColumnPtr MergeTreeRangeReader::fillPartOffsetColumn(ReadResult & result, UInt64 leading_begin_part_offset, UInt64 leading_end_part_offset)
+ColumnPtr MergeTreeRangeReader::createPartOffsetColumn(ReadResult & result, UInt64 leading_begin_part_offset, UInt64 leading_end_part_offset)
 {
    size_t num_rows = result.numReadRows();

--- a/src/Storages/MergeTree/MergeTreeRangeReader.h
+++ b/src/Storages/MergeTree/MergeTreeRangeReader.h
@ -308,7 +308,7 @@ private:
    ReadResult startReadingChain(size_t max_rows, MarkRanges & ranges);
    Columns continueReadingChain(const ReadResult & result, size_t & num_rows);
    void executePrewhereActionsAndFilterColumns(ReadResult & result) const;
-    ColumnPtr fillPartOffsetColumn(ReadResult & result, UInt64 leading_begin_part_offset, UInt64 leading_end_part_offset);
+    ColumnPtr createPartOffsetColumn(ReadResult & result, UInt64 leading_begin_part_offset, UInt64 leading_end_part_offset);

    IMergeTreeReader * merge_tree_reader = nullptr;
    const MergeTreeIndexGranularity * index_granularity = nullptr;
--- a/src/Storages/MergeTree/MergeTreeReadTask.cpp
+++ b/src/Storages/MergeTree/MergeTreeReadTask.cpp
@ -1,6 +1,6 @@
 #include <Storages/MergeTree/MergeTreeReadTask.h>
 #include <Storages/MergeTree/MergeTreeBlockReadUtils.h>
-#include <Storages/MergeTreeVirtualColumns.h>
+#include <Storages/MergeTree/MergeTreeVirtualColumns.h>
 #include <Common/Exception.h>

 namespace DB
--- a/src/Storages/MergeTree/MergeTreeSelectProcessor.cpp
+++ b/src/Storages/MergeTree/MergeTreeSelectProcessor.cpp
@ -12,7 +12,7 @@
 #include <Processors/Chunk.h>
 #include <Processors/QueryPlan/SourceStepWithFilter.h>
 #include <Processors/Transforms/AggregatingTransform.h>
-#include <Storages/MergeTreeVirtualColumns.h>
+#include <Storages/MergeTree/MergeTreeVirtualColumns.h>
 #include <city.h>

 namespace DB
--- a/src/Storages/MergeTree/MergeTreeSequentialSource.cpp
+++ b/src/Storages/MergeTree/MergeTreeSequentialSource.cpp
@ -2,7 +2,7 @@
 #include <Storages/MergeTree/MergeTreeBlockReadUtils.h>
 #include <Storages/MergeTree/LoadedMergeTreeDataPartInfoForReader.h>
 #include <Storages/MergeTree/MergeTreeDataSelectExecutor.h>
-#include <Storages/MergeTreeVirtualColumns.h>
+#include <Storages/MergeTree/MergeTreeVirtualColumns.h>
 #include <Processors/Transforms/FilterTransform.h>
 #include <Processors/QueryPlan/ISourceStep.h>
 #include <QueryPipeline/QueryPipelineBuilder.h>
--- a/src/Storages/MergeTree/MergeTreeVirtualColumns.cpp
+++ b/src/Storages/MergeTree/MergeTreeVirtualColumns.cpp
@ -1,4 +1,4 @@
-#include <Storages/MergeTreeVirtualColumns.h>
+#include <Storages/MergeTree/MergeTreeVirtualColumns.h>
 #include <Storages/MergeTree/IMergeTreeDataPart.h>
 #include <DataTypes/DataTypesNumber.h>
 #include <Parsers/ASTFunction.h>
--- a/src/Storages/MergeTree/MergeTreeVirtualColumns.h
+++ b/src/Storages/MergeTree/MergeTreeVirtualColumns.h
--- a/src/Storages/MergeTree/MutateTask.cpp
+++ b/src/Storages/MergeTree/MutateTask.cpp
@ -23,7 +23,7 @@
 #include <Storages/MutationCommands.h>
 #include <Storages/MergeTree/MergeTreeDataMergerMutator.h>
 #include <Storages/MergeTree/MergeTreeIndexInverted.h>
-#include <Storages/MergeTreeVirtualColumns.h>
+#include <Storages/MergeTree/MergeTreeVirtualColumns.h>
 #include <DataTypes/DataTypeNullable.h>
 #include <DataTypes/DataTypeVariant.h>
 #include <boost/algorithm/string/replace.hpp>
--- a/src/Storages/StorageKeeperMap.cpp
+++ b/src/Storages/StorageKeeperMap.cpp
@ -333,7 +333,7 @@ StorageKeeperMap::StorageKeeperMap(

    VirtualColumnsDescription virtuals;
    virtuals.addEphemeral(String(version_column_name), std::make_shared<DataTypeInt32>(), "");
-    setVirtuals(virtuals);
+    setVirtuals(std::move(virtuals));

    WriteBufferFromOwnString out;
    out << "KeeperMap metadata format version: 1\n"
--- a/src/Storages/StorageLog.cpp
+++ b/src/Storages/StorageLog.cpp
@ -35,7 +35,6 @@
 #include <Backups/IBackup.h>
 #include <Backups/RestorerFromBackup.h>
 #include <Disks/TemporaryFileOnDisk.h>
-#include <Storages/MergeTreeVirtualColumns.h>

 #include <cassert>
 #include <chrono>
--- a/src/Storages/StorageSnapshot.cpp
+++ b/src/Storages/StorageSnapshot.cpp
@ -1,5 +1,4 @@
 #include <Storages/StorageSnapshot.h>
-#include <Storages/MergeTreeVirtualColumns.h>
 #include <Storages/IStorage.h>
 #include <DataTypes/ObjectUtils.h>
 #include <DataTypes/NestedUtils.h>
--- a/src/Storages/StorageSnapshot.h
+++ b/src/Storages/StorageSnapshot.h
@ -84,9 +84,6 @@ struct StorageSnapshot

    /// If we have a projection then we should use its metadata.
    StorageMetadataPtr getMetadataForQuery() const { return projection ? projection->metadata : metadata; }
-
-private:
-    void init();
 };

 using StorageSnapshotPtr = std::shared_ptr<StorageSnapshot>;
--- a/src/Storages/StorageValues.cpp
+++ b/src/Storages/StorageValues.cpp
@ -12,13 +12,13 @@ StorageValues::StorageValues(
    const StorageID & table_id_,
    const ColumnsDescription & columns_,
    const Block & res_block_,
-    const VirtualColumnsDescription & virtuals_)
+    VirtualColumnsDescription virtuals_)
    : IStorage(table_id_), res_block(res_block_)
 {
    StorageInMemoryMetadata storage_metadata;
    storage_metadata.setColumns(columns_);
    setInMemoryMetadata(storage_metadata);
-    setVirtuals(virtuals_);
+    setVirtuals(std::move(virtuals_));
 }

 Pipe StorageValues::read(
--- a/src/Storages/StorageValues.h
+++ b/src/Storages/StorageValues.h
@ -18,7 +18,7 @@ public:
        const StorageID & table_id_,
        const ColumnsDescription & columns_,
        const Block & res_block_,
-        const VirtualColumnsDescription & virtuals_ = {});
+        VirtualColumnsDescription virtuals_ = {});

    std::string getName() const override { return "Values"; }

--- a/src/Storages/System/StorageSystemDictionaries.cpp
+++ b/src/Storages/System/StorageSystemDictionaries.cpp
@ -56,7 +56,7 @@ StorageSystemDictionaries::StorageSystemDictionaries(const StorageID & storage_i
 {
    VirtualColumnsDescription virtuals;
    virtuals.addEphemeral("key", std::make_shared<DataTypeString>(), "");
-    setVirtuals(virtuals);
+    setVirtuals(std::move(virtuals));
 }

 ColumnsDescription StorageSystemDictionaries::getColumnsDescription()
--- a/src/Storages/System/StorageSystemPartsBase.cpp
+++ b/src/Storages/System/StorageSystemPartsBase.cpp
@ -262,7 +262,7 @@ StorageSystemPartsBase::StorageSystemPartsBase(const StorageID & table_id_, Colu

    VirtualColumnsDescription virtuals;
    virtuals.addEphemeral("_state", std::make_shared<DataTypeString>(), "");
-    setVirtuals(virtuals);
+    setVirtuals(std::move(virtuals));
 }

 }
--- a/src/Storages/VirtualColumnUtils.cpp
+++ b/src/Storages/VirtualColumnUtils.cpp
@ -352,17 +352,17 @@ VirtualColumnsDescription getVirtualsForFileLikeStorage(const ColumnsDescription
 {
    VirtualColumnsDescription desc;

-    auto add_virtual = [&](const auto & name, const auto & type, const auto & comment)
+    auto add_virtual = [&](const auto & name, const auto & type)
    {
        if (storage_columns.has(name))
            return;

-        desc.addEphemeral(name, type, comment);
+        desc.addEphemeral(name, type, "");
    };

-    add_virtual("_path", std::make_shared<DataTypeLowCardinality>(std::make_shared<DataTypeString>()), "");
-    add_virtual("_file", std::make_shared<DataTypeLowCardinality>(std::make_shared<DataTypeString>()), "");
-    add_virtual("_size", makeNullable(std::make_shared<DataTypeUInt64>()), "");
+    add_virtual("_path", std::make_shared<DataTypeLowCardinality>(std::make_shared<DataTypeString>()));
+    add_virtual("_file", std::make_shared<DataTypeLowCardinality>(std::make_shared<DataTypeString>()));
+    add_virtual("_size", makeNullable(std::make_shared<DataTypeUInt64>()));

    return desc;
 }
--- a/src/Storages/VirtualColumnsDescription.h
+++ b/src/Storages/VirtualColumnsDescription.h
@ -7,7 +7,6 @@ namespace DB

 struct VirtualColumnDescription : public ColumnDescription
 {
-public:
    using Self = VirtualColumnDescription;
    VirtualsKind kind;

@ -16,6 +15,9 @@ public:

    bool isEphemeral() const { return kind == VirtualsKind::Ephemeral; }
    bool isPersistent() const { return kind == VirtualsKind::Persistent; }
+
+    /// This method is needed for boost::multi_index because field
+    /// of base class cannot be referenced in boost::multi_index::member.
    const String & getName() const { return name; }
 };