Merge pull request #64846 from rschu1ze/simplify-inverted-indexes

Simplify handling of old 'inverted' indexes

src/Interpreters/InterpreterCreateQuery.cpp

@@ -756,7 +756,7 @@ InterpreterCreateQuery::TableProperties InterpreterCreateQuery::getTableProperti
                     throw Exception(ErrorCodes::SUPPORT_IS_DISABLED, "Experimental full-text index feature is not enabled (the setting 'allow_experimental_full_text_index')");
                 /// ----
                 /// Temporary check during a transition period. Please remove at the end of 2024.
-                if (index_desc.type == INVERTED_INDEX_NAME && settings.allow_experimental_inverted_index) /// The funny condition is not a mistake, see 02346_fulltext_index_old_name.sql
+                if (index_desc.type == INVERTED_INDEX_NAME && !settings.allow_experimental_inverted_index)
                     throw Exception(ErrorCodes::ILLEGAL_INDEX, "Please use index type 'full_text' instead of 'inverted'");
                 /// ----
                 if (index_desc.type == "annoy" && !settings.allow_experimental_annoy_index)

src/Storages/MergeTree/MergeTreeIndexFullText.cpp

@@ -742,6 +742,7 @@ bool MergeTreeConditionFullText::tryPrepareSetGinFilter(
 
 MergeTreeIndexGranulePtr MergeTreeIndexFullText::createIndexGranule() const
 {
+    /// ------
     /// Index type 'inverted' was renamed to 'full_text' in May 2024.
     /// Tables with old indexes can be loaded during a transition period. We still want let users know that they should drop existing
     /// indexes and re-create them. Function `createIndexGranule` is called whenever the index is used by queries. Reject the query if we
@@ -749,6 +750,7 @@ MergeTreeIndexGranulePtr MergeTreeIndexFullText::createIndexGranule() const
     /// TODO: remove this at the end of 2024.
     if (index.type == INVERTED_INDEX_NAME)
         throw Exception(ErrorCodes::ILLEGAL_INDEX, "Indexes of type 'inverted' are no longer supported. Please drop and recreate the index as type 'full-text'");
+    /// ------
 
     return std::make_shared<MergeTreeIndexGranuleFullText>(index.name, index.column_names.size(), params);
 }

tests/queries/0_stateless/02346_fulltext_index_old_name.sql

@@ -1,22 +1,16 @@
+-- Index type 'inverted' was renamed to 'full_text' in April 2024.
+-- Such indexes are experimental. Test what happens when ClickHouse encounters tables with the old index type.
+
 DROP TABLE IF EXISTS tab;
 
--- Index type 'inverted' was renamed to 'full_text' in April 2024.
--- Such indexes are experimental. Nevertheless test what happens when ClickHouse encounters tables with the old index type.
+-- It must be possible to load old tables with 'inverted'-type indexes
+-- In stateless tests, we cannot use old persistences. Emulate "loading an old index" by creating it (internally, similar code executes).
 
--- Create a full text index with the old type
--- This was how it was done in the old days. These days this throws an exception.
-SET allow_experimental_inverted_index = 1;
-CREATE TABLE tab(k UInt64, s String, INDEX idx(s) TYPE inverted(2)) ENGINE = MergeTree() ORDER BY k; -- { serverError ILLEGAL_INDEX };
-
--- There are unfortunately side effects of this behavior. In particular, if ClickHouse's automatic table load during
--- startup finds a table with 'inverted'-type indexes created by an older version, it immediately halts as it thinks
--- the persistence is corrupt. Similarly (but less severely), tables with 'inverted' index cannot be attached.
--- A backdoor avoids this. Just set allow_experimental_inverted_index = 0 (which is the default).
---
--- Note that the backdoor will exist only temporarily during a transition period. It will be removed in future. Its only purpose is
--- to simplify the migrationn of experimental inverted indexes to experimental full-text indexes instead of simply breaking existing
--- tables.
+-- Creation only works with the (old) setting enabled.
 SET allow_experimental_inverted_index = 0;
+CREATE TABLE tab(k UInt64, s String, INDEX idx(s) TYPE inverted(2)) ENGINE = MergeTree() ORDER BY k; -- { serverError ILLEGAL_INDEX }
+
+SET allow_experimental_inverted_index = 1;
 CREATE TABLE tab(k UInt64, s String, INDEX idx(s) TYPE inverted(2)) ENGINE = MergeTree() ORDER BY k;
 INSERT INTO tab VALUES (1, 'ab') (2, 'bc');
 
@@ -24,14 +18,12 @@ INSERT INTO tab VALUES (1, 'ab') (2, 'bc');
 DETACH TABLE tab;
 ATTACH TABLE tab;
 
--- No, the backdoor does not make 'inverted' indexes non-experimental.
--- On the one hand, the backdoor is undocumented, on the other hand, SELECTs that use such indexes now throw an exception,
--- making 'inverted' indexes useless.
+-- To encourage users to migrate to the new index type, we now throw an exception when the index is used by queries.
 SELECT * from tab WHERE s = 'bc'; -- { serverError ILLEGAL_INDEX }
 
 -- The exception recommends to drop the index and create a 'full_text' index instead. Let's try.
 ALTER TABLE tab DROP INDEX idx;
-SET allow_experimental_full_text_index = 1; -- note that this is a different setting
+SET allow_experimental_full_text_index = 1; -- the new setting
 ALTER TABLE tab ADD INDEX idx(s) TYPE full_text(2);
 
 SELECT * from tab WHERE s = 'bc';