Merge pull request ClickHouse#79853 from rschu1ze/gin-test

Some cleanups around inverted index

Author: rschu1ze

docs/en/engines/table-engines/mergetree-family/invertedindexes.md

@@ -60,10 +60,6 @@ ENGINE = MergeTree
 ORDER BY key
 ```
 
-:::note
-In earlier versions of ClickHouse, the corresponding index type name was `inverted`.
-:::
-
 where `N` specifies the tokenizer:
 
 - `gin(0)` (or shorter: `gin()`) set the tokenizer to "tokens", i.e. split strings along spaces,

src/Interpreters/GinFilter.cpp

@@ -11,8 +11,6 @@
 #include <Storages/MergeTree/GinIndexStore.h>
 #include <Storages/MergeTree/MergeTreeIndexBloomFilterText.h>
 #include <Storages/MergeTree/MergeTreeIndexGin.h>
-#include <string>
-#include <algorithm>
 #include <city.h>
 
 namespace DB

src/Storages/MergeTree/MergeTreeDataPartWriterOnDisk.cpp

@@ -1,4 +1,6 @@
 #include <Storages/MergeTree/MergeTreeDataPartWriterOnDisk.h>
+
+#include <Storages/MergeTree/MergeTreeData.h>
 #include <Storages/MergeTree/MergeTreeIndexGin.h>
 #include <Storages/MergeTree/MergeTreeSettings.h>
 #include <Common/ElapsedTimeProfileEventIncrement.h>

src/Storages/MergeTree/MergeTreeIndexGin.cpp

@@ -3,30 +3,28 @@
 #include <Interpreters/GinFilter.h>
 #include <Interpreters/ITokenExtractor.h>
 #include <Storages/MergeTree/KeyCondition.h>
-#include <Storages/MergeTree/MergeTreeData.h>
-#include <base/types.h>
-#include <memory>
+#include <Storages/MergeTree/MergeTreeIndices.h>
 
 namespace DB
 {
+
 struct MergeTreeIndexGranuleGin final : public IMergeTreeIndexGranule
 {
-    explicit MergeTreeIndexGranuleGin(
+    MergeTreeIndexGranuleGin(
         const String & index_name_,
         size_t columns_number,
-        const GinFilterParameters & params_);
+        const GinFilterParameters & gin_filter_params_);
 
     ~MergeTreeIndexGranuleGin() override = default;
 
     void serializeBinary(WriteBuffer & ostr) const override;
     void deserializeBinary(ReadBuffer & istr, MergeTreeIndexVersion version) override;
 
     bool empty() const override { return !has_elems; }
-
     size_t memoryUsageBytes() const override;
 
     const String index_name;
-    const GinFilterParameters params;
+    const GinFilterParameters gin_filter_params;
     GinFilters gin_filters;
     bool has_elems;
 };
@@ -35,28 +33,25 @@ using MergeTreeIndexGranuleGinPtr = std::shared_ptr<MergeTreeIndexGranuleGin>;
 
 struct MergeTreeIndexAggregatorGin final : IMergeTreeIndexAggregator
 {
-    explicit MergeTreeIndexAggregatorGin(
+    MergeTreeIndexAggregatorGin(
         GinIndexStorePtr store_,
         const Names & index_columns_,
         const String & index_name_,
-        const GinFilterParameters & params_,
+        const GinFilterParameters & gin_filter_params_,
         TokenExtractorPtr token_extractor_);
 
     ~MergeTreeIndexAggregatorGin() override = default;
 
     bool empty() const override { return !granule || granule->empty(); }
     MergeTreeIndexGranulePtr getGranuleAndReset() override;
-
     void update(const Block & block, size_t * pos, size_t limit) override;
-
     void addToGinFilter(UInt32 rowID, const char * data, size_t length, GinFilter & gin_filter);
 
     GinIndexStorePtr store;
     Names index_columns;
     const String index_name;
-    const GinFilterParameters params;
+    const GinFilterParameters gin_filter_params;
     TokenExtractorPtr token_extractor;
-
     MergeTreeIndexGranuleGinPtr granule;
 };
 
@@ -65,22 +60,17 @@ class MergeTreeIndexConditionGin final : public IMergeTreeIndexCondition, WithCo
 {
 public:
     MergeTreeIndexConditionGin(
-            const ActionsDAG::Node * predicate,
-            ContextPtr context,
-            const Block & index_sample_block,
-            const GinFilterParameters & params_,
-            TokenExtractorPtr token_extactor_);
+        const ActionsDAG::Node * predicate,
+        ContextPtr context,
+        const Block & index_sample_block,
+        const GinFilterParameters & gin_filter_params_,
+        TokenExtractorPtr token_extactor_);
 
     ~MergeTreeIndexConditionGin() override = default;
 
     bool alwaysUnknownOrTrue() const override;
-    bool mayBeTrueOnGranule([[maybe_unused]]MergeTreeIndexGranulePtr idx_granule) const override
-    {
-        /// should call mayBeTrueOnGranuleInPart instead
-        assert(false);
-        return false;
-    }
-    bool mayBeTrueOnGranuleInPart(MergeTreeIndexGranulePtr idx_granule, [[maybe_unused]] PostingsCacheForStore & cache_store) const;
+    bool mayBeTrueOnGranule(MergeTreeIndexGranulePtr idx_granule) const override;
+    bool mayBeTrueOnGranuleInPart(MergeTreeIndexGranulePtr idx_granule, PostingsCacheForStore & cache_store) const;
 
 private:
     struct KeyTuplePositionMapping
@@ -90,6 +80,7 @@ class MergeTreeIndexConditionGin final : public IMergeTreeIndexCondition, WithCo
         size_t tuple_index;
         size_t key_index;
     };
+
     /// Uses RPN like KeyCondition
     struct RPNElement
     {
@@ -135,7 +126,6 @@ class MergeTreeIndexConditionGin final : public IMergeTreeIndexCondition, WithCo
     using RPN = std::vector<RPNElement>;
 
     bool traverseAtomAST(const RPNBuilderTreeNode & node, RPNElement & out);
-
     bool traverseASTEquals(
         const String & function_name,
         const RPNBuilderTreeNode & key_ast,
@@ -145,14 +135,10 @@ class MergeTreeIndexConditionGin final : public IMergeTreeIndexCondition, WithCo
 
     bool tryPrepareSetGinFilter(const RPNBuilderTreeNode & lhs, const RPNBuilderTreeNode & rhs, RPNElement & out);
 
-    static bool createFunctionEqualsCondition(
-        RPNElement & out, const Field & value, const GinFilterParameters & params, TokenExtractorPtr token_extractor);
-
     const Block & header;
-    GinFilterParameters params;
+    GinFilterParameters gin_filter_params;
     TokenExtractorPtr token_extractor;
     RPN rpn;
-    /// Sets from syntax analyzer.
     PreparedSetsPtr prepared_sets;
 };
 
@@ -161,11 +147,8 @@ class MergeTreeIndexGin final : public IMergeTreeIndex
 public:
     MergeTreeIndexGin(
         const IndexDescription & index_,
-        const GinFilterParameters & params_,
-        std::unique_ptr<ITokenExtractor> && token_extractor_)
-        : IMergeTreeIndex(index_)
-        , params(params_)
-        , token_extractor(std::move(token_extractor_)) {}
+        const GinFilterParameters & gin_filter_params_,
+        std::unique_ptr<ITokenExtractor> && token_extractor_);
 
     ~MergeTreeIndexGin() override = default;
 
@@ -174,8 +157,7 @@ class MergeTreeIndexGin final : public IMergeTreeIndex
     MergeTreeIndexAggregatorPtr createIndexAggregatorForPart(const GinIndexStorePtr & store, const MergeTreeWriterSettings & /*settings*/) const override;
     MergeTreeIndexConditionPtr createIndexCondition(const ActionsDAG::Node * predicate, ContextPtr context) const override;
 
-    GinFilterParameters params;
-    /// Function for selecting next token.
+    GinFilterParameters gin_filter_params;
     std::unique_ptr<ITokenExtractor> token_extractor;
 };
 

src/Storages/MergeTree/MergeTreeIndexGin.h

@@ -592,17 +592,17 @@ void vectorSimilarityIndexValidator(const IndexDescription & index, bool /* atta
 
     /// Check that the index is created on a single column
     if (index.column_names.size() != 1 || index.data_types.size() != 1)
-        throw Exception(ErrorCodes::INCORRECT_NUMBER_OF_COLUMNS, "Vector similarity indexes must be created on a single column");
+        throw Exception(ErrorCodes::INCORRECT_NUMBER_OF_COLUMNS, "Vector similarity index must be created on a single column");
 
     /// Check that the data type is Array(Float32|Float64|BFloat16)
     DataTypePtr data_type = index.sample_block.getDataTypes()[0];
     const auto * data_type_array = typeid_cast<const DataTypeArray *>(data_type.get());
     if (!data_type_array)
-        throw Exception(ErrorCodes::ILLEGAL_COLUMN, "Vector similarity indexes can only be created on columns of type Array(Float32|Float64|BFloat16)");
+        throw Exception(ErrorCodes::ILLEGAL_COLUMN, "Vector similarity index can only be created on columns of type Array(Float32|Float64|BFloat16)");
     TypeIndex nested_type_index = data_type_array->getNestedType()->getTypeId();
     WhichDataType which(nested_type_index);
     if (!which.isNativeFloat() && !which.isBFloat16())
-        throw Exception(ErrorCodes::ILLEGAL_COLUMN, "Vector similarity indexes can only be created on columns of type Array(Float32|Float64|BFloat16)");
+        throw Exception(ErrorCodes::ILLEGAL_COLUMN, "Vector similarity index can only be created on columns of type Array(Float32|Float64|BFloat16)");
 }
 
 }

src/Storages/MergeTree/MergeTreeIndexVectorSimilarity.cpp

@@ -0,0 +1,6 @@
+Two or less arguments
+1st argument (tokenizer) must be UInt64
+2st argument (max_rows_per_postings_list) must be UInt64
+2st argument (max_rows_per_postings_list) must be bigger than MIN_ROWS_PER_POSTINGS_LIST
+Must be created on single column
+Must be created on String or FixedString or Array(String) or Array(FixedString) or LowCardinality(String) or LowCardinality(FixedString) columns

tests/queries/0_stateless/02346_gin_index_creation_negative.reference

@@ -0,0 +1,27 @@
+-- Tests that various conditions are checked during creation of GIN indexes.
+
+-- Using SETTINGS min_bytes_for_full_part_storage = 0 because GIN indexes currently don't work with packed parts
+
+SET allow_experimental_full_text_index = 1;
+
+DROP TABLE IF EXISTS tab;
+
+SELECT 'Two or less arguments';
+CREATE TABLE tab (key UInt64, str String, INDEX inv_idx str TYPE gin('cant_have', 'three', 'args')) ENGINE = MergeTree ORDER BY key SETTINGS min_bytes_for_full_part_storage = 0; -- { serverError INCORRECT_QUERY }
+
+SELECT '1st argument (tokenizer) must be UInt64';
+CREATE TABLE tab (key UInt64, str String, INDEX inv_idx str TYPE gin('string_arg')) ENGINE = MergeTree ORDER BY key SETTINGS min_bytes_for_full_part_storage = 0; -- { serverError INCORRECT_QUERY }
+
+SELECT '2st argument (max_rows_per_postings_list) must be UInt64';
+CREATE TABLE tab (key UInt64, str String, INDEX inv_idx str TYPE gin(1, 'string_arg')) ENGINE = MergeTree ORDER BY key SETTINGS min_bytes_for_full_part_storage = 0; -- { serverError INCORRECT_QUERY }
+
+SELECT '2st argument (max_rows_per_postings_list) must be bigger than MIN_ROWS_PER_POSTINGS_LIST';
+CREATE TABLE tab (key UInt64, str String, INDEX inv_idx str TYPE gin(1, 1)) ENGINE = MergeTree ORDER BY key SETTINGS min_bytes_for_full_part_storage = 0; -- { serverError INCORRECT_QUERY }
+
+SELECT 'Must be created on single column';
+CREATE TABLE tab (key UInt64, str1 String, str2 String, INDEX inv_idx (str1, str2) TYPE gin(1, 9999)) ENGINE = MergeTree ORDER BY key SETTINGS min_bytes_for_full_part_storage = 0; -- { serverError INCORRECT_NUMBER_OF_COLUMNS }
+
+SELECT 'Must be created on String or FixedString or Array(String) or Array(FixedString) or LowCardinality(String) or LowCardinality(FixedString) columns';
+CREATE TABLE tab (key UInt64, str UInt64, INDEX inv_idx str TYPE gin(1, 9999)) ENGINE = MergeTree ORDER BY key SETTINGS min_bytes_for_full_part_storage = 0; -- { serverError INCORRECT_QUERY }
+CREATE TABLE tab (key UInt64, str Float32, INDEX inv_idx str TYPE gin(1, 999)) ENGINE = MergeTree ORDER BY key SETTINGS min_bytes_for_full_part_storage = 0; -- { serverError INCORRECT_QUERY }
+CREATE TABLE tab (key UInt64, str Nullable(String), INDEX inv_idx str TYPE gin(1, 999)) ENGINE = MergeTree ORDER BY key SETTINGS min_bytes_for_full_part_storage = 0; -- { serverError INCORRECT_QUERY }

tests/queries/0_stateless/02346_gin_index_creation_negative.sql

@@ -2,7 +2,7 @@
 
 SET allow_experimental_full_text_index = 1;
 
-CREATE TABLE t
+CREATE TABLE tab
 (
     key UInt64,
     str String,
@@ -12,7 +12,7 @@ ENGINE = MergeTree
 ORDER BY key
 SETTINGS min_bytes_for_full_part_storage = 0; -- GIN indexes currently don't work with packed parts
 
-INSERT INTO t VALUES (1, 'Hello World');
+INSERT INTO tab VALUES (1, 'Hello World');
 
-ALTER TABLE t DETACH PART 'all_1_1_0';
-ALTER TABLE t ATTACH PART 'all_1_1_0';
+ALTER TABLE tab DETACH PART 'all_1_1_0';
+ALTER TABLE tab ATTACH PART 'all_1_1_0';