diff --git a/be/src/storage/index/index_reader_helper.h b/be/src/storage/index/index_reader_helper.h index bd2aacdabb1a63..9c00951958f87f 100644 --- a/be/src/storage/index/index_reader_helper.h +++ b/be/src/storage/index/index_reader_helper.h @@ -18,6 +18,7 @@ #pragma once #include "storage/index/index_iterator.h" +#include "storage/index/inverted/analyzer/analyzer.h" #include "storage/index/inverted/inverted_index_reader.h" namespace doris::segment_v2 { @@ -88,15 +89,36 @@ class IndexReaderHelper { return iter->get_reader(InvertedIndexReaderType::STRING_TYPE) != nullptr; } + // Positions -- and therefore phrase and relevance work -- are only reachable + // when the index tokenizes. The reason is on the QUERY side, not the write + // side: InvertedIndexAnalyzer::get_analyse_result() returns the entire search + // string as ONE term whenever should_analyzer() is false, and every phrase + // variant (MATCH_PHRASE, _PREFIX, _EDGE) takes its terms from there. A + // single-term phrase is just a term query, so no query against a + // non-tokenizing index can observe a position. + // + // Note it is NOT enough to say such an index holds one term per document: + // an ARRAY column with parser=none emits one term per element and the writer + // does advance positions between them (SniiIndexColumnWriter::_add_array_values). + // Those positions are simply unreachable, because the query can never supply + // a second term to match against them. + // + // New indexes no longer carry the option at all (Index.java drops it), but + // tablet metadata already on disk still does, so the check is repeated here. + static bool persists_scoring_inputs(const TabletIndex* index_meta) { + const auto& properties = index_meta->properties(); + return get_parser_phrase_support_string_from_properties(properties) == + INVERTED_INDEX_PARSER_PHRASE_SUPPORT_YES && + inverted_index::InvertedIndexAnalyzer::should_analyzer(properties); + } + static bool is_need_similarity_score(InvertedIndexQueryType query_type, const TabletIndex* index_meta) { if (query_type == InvertedIndexQueryType::MATCH_ANY_QUERY || query_type == InvertedIndexQueryType::MATCH_ALL_QUERY || query_type == InvertedIndexQueryType::MATCH_PHRASE_QUERY || query_type == InvertedIndexQueryType::MATCH_PHRASE_PREFIX_QUERY) { - const auto& properties = index_meta->properties(); - if (get_parser_phrase_support_string_from_properties(properties) == - INVERTED_INDEX_PARSER_PHRASE_SUPPORT_YES) { + if (persists_scoring_inputs(index_meta)) { return true; } } @@ -108,9 +130,7 @@ class IndexReaderHelper { if (query_type == TExprOpcode::MATCH_ANY || query_type == TExprOpcode::MATCH_ALL || query_type == TExprOpcode::MATCH_PHRASE || query_type == TExprOpcode::MATCH_PHRASE_PREFIX) { - const auto& properties = index_meta->properties(); - if (get_parser_phrase_support_string_from_properties(properties) == - INVERTED_INDEX_PARSER_PHRASE_SUPPORT_YES) { + if (persists_scoring_inputs(index_meta)) { return true; } } diff --git a/be/test/storage/segment/index_reader_helper_test.cpp b/be/test/storage/segment/index_reader_helper_test.cpp index d52e036f22997a..a609a95e1076bc 100644 --- a/be/test/storage/segment/index_reader_helper_test.cpp +++ b/be/test/storage/segment/index_reader_helper_test.cpp @@ -25,6 +25,8 @@ #include "common/be_mock_util.h" #include "storage/index/index_reader.h" +#include "storage/index/inverted/abstract_analysis_factory.h" +#include "storage/index/inverted/analyzer/analyzer.h" #include "storage/index/inverted/inverted_index_parser.h" #include "storage/index/inverted/inverted_index_query_type.h" #include "storage/index/inverted/inverted_index_reader.h" @@ -380,6 +382,10 @@ TEST_F(IndexReaderHelperTest, IsNeedSimilarityScoreWithInvertedIndexQueryTypeTes index_meta_pb->add_col_unique_id(1); auto* properties = index_meta_pb->mutable_properties(); + // These cases pin the QUERY TYPE filter, so the index has to be one that can + // actually be scored: tokenizing, with positions. support_phrase alone is not + // enough -- see the ...WithoutTokenizer cases below. + (*properties)[INVERTED_INDEX_PARSER_KEY] = INVERTED_INDEX_PARSER_ENGLISH; (*properties)[INVERTED_INDEX_PARSER_PHRASE_SUPPORT_KEY] = INVERTED_INDEX_PARSER_PHRASE_SUPPORT_YES; @@ -396,6 +402,121 @@ TEST_F(IndexReaderHelperTest, IsNeedSimilarityScoreWithInvertedIndexQueryTypeTes InvertedIndexQueryType::LESS_THAN_QUERY, &index_meta)); } +// support_phrase asks for term positions, and a position is only observable when +// a query can supply a second term to match against it. should_analyzer() is the +// exact line: below it the query string is never split, so a phrase degenerates +// to a term query and the index can neither serve a phrase nor be ranked -- +// exactly as on V1/V2/V3, where such an index is served by a reader with no +// similarity path at all. Index.java now drops the option before it reaches the +// BE, but tablet metadata already on disk still carries it, which is what these +// cases cover. Note a NORMALIZER sits ABOVE that line: should_analyzer() reads it +// through get_analyzer_name_from_properties, so it stays scoreable. +TEST_F(IndexReaderHelperTest, IsNeedSimilarityScoreRequiresATokenizer) { + const auto meta_with = [](const std::vector>& props) { + auto pb = std::make_unique(); + pb->set_index_type(IndexType::INVERTED); + pb->set_index_id(1); + pb->set_index_name("test_index"); + pb->add_col_unique_id(1); + auto* properties = pb->mutable_properties(); + for (const auto& [key, value] : props) { + (*properties)[key] = value; + } + TabletIndex meta; + meta.init_from_pb(*pb); + return meta; + }; + + // Keyword index: no parser, no analyzer. + const TabletIndex keyword = meta_with( + {{INVERTED_INDEX_PARSER_PHRASE_SUPPORT_KEY, INVERTED_INDEX_PARSER_PHRASE_SUPPORT_YES}}); + EXPECT_FALSE(IndexReaderHelper::is_need_similarity_score( + InvertedIndexQueryType::MATCH_ANY_QUERY, &keyword)); + EXPECT_FALSE(IndexReaderHelper::is_need_similarity_score(TExprOpcode::MATCH_ANY, &keyword)); + + // parser=none is the untokenized lane spelled out. + const TabletIndex parser_none = meta_with( + {{INVERTED_INDEX_PARSER_KEY, INVERTED_INDEX_PARSER_NONE}, + {INVERTED_INDEX_PARSER_PHRASE_SUPPORT_KEY, INVERTED_INDEX_PARSER_PHRASE_SUPPORT_YES}}); + EXPECT_FALSE(IndexReaderHelper::is_need_similarity_score( + InvertedIndexQueryType::MATCH_PHRASE_QUERY, &parser_none)); + EXPECT_FALSE( + IndexReaderHelper::is_need_similarity_score(TExprOpcode::MATCH_PHRASE, &parser_none)); + + // A real parser with positions is the shape that does get ranked. + const TabletIndex tokenized = meta_with( + {{INVERTED_INDEX_PARSER_KEY, INVERTED_INDEX_PARSER_ENGLISH}, + {INVERTED_INDEX_PARSER_PHRASE_SUPPORT_KEY, INVERTED_INDEX_PARSER_PHRASE_SUPPORT_YES}}); + EXPECT_TRUE(IndexReaderHelper::is_need_similarity_score( + InvertedIndexQueryType::MATCH_PHRASE_QUERY, &tokenized)); + EXPECT_TRUE(IndexReaderHelper::is_need_similarity_score(TExprOpcode::MATCH_PHRASE, &tokenized)); + + // A NORMALIZER counts as analyzed on this side: get_analyzer_name_from_properties + // falls back to the normalizer key, so should_analyzer() is true and the column + // gets a FULLTEXT reader. Index.java must therefore KEEP support_phrase for it -- + // this case is the BE half of that contract. + const TabletIndex normalizer = meta_with( + {{INVERTED_INDEX_NORMALIZER_NAME_KEY, "my_normalizer"}, + {INVERTED_INDEX_PARSER_PHRASE_SUPPORT_KEY, INVERTED_INDEX_PARSER_PHRASE_SUPPORT_YES}}); + EXPECT_TRUE(IndexReaderHelper::is_need_similarity_score( + InvertedIndexQueryType::MATCH_PHRASE_QUERY, &normalizer)); + EXPECT_TRUE( + IndexReaderHelper::is_need_similarity_score(TExprOpcode::MATCH_PHRASE, &normalizer)); + + // ... and dropping positions takes it back out, tokenizer or not. + const TabletIndex no_positions = + meta_with({{INVERTED_INDEX_PARSER_KEY, INVERTED_INDEX_PARSER_ENGLISH}}); + EXPECT_FALSE(IndexReaderHelper::is_need_similarity_score( + InvertedIndexQueryType::MATCH_PHRASE_QUERY, &no_positions)); +} + +// The invariant persists_scoring_inputs() rests on. Dropping support_phrase from a +// non-tokenizing index is only safe because no query against such an index can ever +// produce a second term to match a position against: get_analyse_result() short-circuits +// on !should_analyzer() and hands back the whole search string as one term, for every +// analysis purpose including the phrase ones. Every phrase variant (MATCH_PHRASE, +// _PREFIX, _EDGE) sources its terms from here, so a single-term phrase degenerates to a +// term query and positions become unobservable. +// +// This matters most for ARRAY: an ARRAY column is forced to parser=none by +// InvertedIndexUtil::checkInvertedIndexParser, yet it emits one term per element and the +// writer does advance positions between them. "One term per document" is therefore the +// wrong reason; the query side is the right one, and this test is what pins it. +TEST_F(IndexReaderHelperTest, UntokenizedQueriesCannotObserveAPosition) { + using inverted_index::InvertedIndexAnalyzer; + const std::string phrase = "quick brown fox"; + + const std::vector purposes = {AnalysisPurpose::kPlainQuery, + AnalysisPurpose::kExactPhraseQuery, + AnalysisPurpose::kPhrasePrefixQuery}; + + const std::vector> untokenized = { + {}, // keyword + {{INVERTED_INDEX_PARSER_KEY, INVERTED_INDEX_PARSER_NONE}}, // ARRAY's only option + }; + + for (const auto& properties : untokenized) { + ASSERT_FALSE(InvertedIndexAnalyzer::should_analyzer(properties)); + for (auto purpose : purposes) { + const auto terms = + InvertedIndexAnalyzer::get_analyse_result(phrase, properties, purpose); + ASSERT_EQ(terms.size(), 1U) << "purpose=" << static_cast(purpose); + EXPECT_TRUE(terms[0].is_single_term()); + EXPECT_EQ(terms[0].get_single_term(), phrase); + } + } + + // Contrast: a tokenizing index really does yield several terms, which is what makes + // positions -- and therefore support_phrase -- meaningful there. + const std::map tokenizing = { + {INVERTED_INDEX_PARSER_KEY, INVERTED_INDEX_PARSER_ENGLISH}}; + ASSERT_TRUE(InvertedIndexAnalyzer::should_analyzer(tokenizing)); + EXPECT_GT(InvertedIndexAnalyzer::get_analyse_result(phrase, tokenizing, + AnalysisPurpose::kExactPhraseQuery) + .size(), + 1U); +} + TEST_F(IndexReaderHelperTest, IsNeedSimilarityScoreWithTExprOpcodeTest) { TabletIndex index_meta; auto index_meta_pb = std::make_unique(); @@ -405,6 +526,10 @@ TEST_F(IndexReaderHelperTest, IsNeedSimilarityScoreWithTExprOpcodeTest) { index_meta_pb->add_col_unique_id(1); auto* properties = index_meta_pb->mutable_properties(); + // These cases pin the QUERY TYPE filter, so the index has to be one that can + // actually be scored: tokenizing, with positions. support_phrase alone is not + // enough -- see the ...WithoutTokenizer cases below. + (*properties)[INVERTED_INDEX_PARSER_KEY] = INVERTED_INDEX_PARSER_ENGLISH; (*properties)[INVERTED_INDEX_PARSER_PHRASE_SUPPORT_KEY] = INVERTED_INDEX_PARSER_PHRASE_SUPPORT_YES; diff --git a/fe/fe-catalog/src/main/java/org/apache/doris/catalog/Index.java b/fe/fe-catalog/src/main/java/org/apache/doris/catalog/Index.java index ee85e1daf93955..c3be7321d6a674 100644 --- a/fe/fe-catalog/src/main/java/org/apache/doris/catalog/Index.java +++ b/fe/fe-catalog/src/main/java/org/apache/doris/catalog/Index.java @@ -75,15 +75,21 @@ public Index(long indexId, String indexName, List columns, this.comment = comment; if (indexType == IndexType.INVERTED) { if (this.properties != null && !this.properties.isEmpty()) { - if (this.properties.containsKey(InvertedIndexProperties.INVERTED_INDEX_PARSER_KEY) - || this.properties.containsKey(InvertedIndexProperties.INVERTED_INDEX_PARSER_KEY_ALIAS) - || this.properties.containsKey(InvertedIndexProperties.INVERTED_INDEX_ANALYZER_NAME_KEY) - || this.properties.containsKey(InvertedIndexProperties.INVERTED_INDEX_NORMALIZER_NAME_KEY)) { - String supportPhraseKey = InvertedIndexProperties - .INVERTED_INDEX_SUPPORT_PHRASE_KEY; + String supportPhraseKey = InvertedIndexProperties.INVERTED_INDEX_SUPPORT_PHRASE_KEY; + if (isTokenizedInvertedIndex(this.properties)) { if (!this.properties.containsKey(supportPhraseKey)) { this.properties.put(supportPhraseKey, "true"); } + } else { + // No analyzer on either side, so the query string is never split + // either: InvertedIndexAnalyzer::get_analyse_result returns the + // whole search string as ONE term, and every phrase variant takes + // its terms from there. A single-term phrase is a term query, so + // no query against this index can observe a position. Drop the + // option instead of carrying it down to the BE, where it would ask + // for position data nobody can read and make the index look + // scoreable to IndexReaderHelper::is_need_similarity_score. + this.properties.remove(supportPhraseKey); } if (this.properties.containsKey(InvertedIndexProperties.INVERTED_INDEX_PARSER_KEY) || this.properties.containsKey(InvertedIndexProperties.INVERTED_INDEX_PARSER_KEY_ALIAS)) { @@ -96,6 +102,30 @@ public Index(long indexId, String indexName, List columns, } } + // True when the BE will run an analyzer over this index, i.e. when it serves the + // column with a FULLTEXT reader rather than the keyword lane. This has to mirror + // InvertedIndexAnalyzer::should_analyzer EXACTLY, so it is composed from the same + // two resolvers the BE uses rather than re-reading the keys by hand: + // + // getPreferredAnalyzer <-> get_analyzer_name_from_properties + // (note both fall back to the NORMALIZER key) + // getInvertedIndexParser <-> get_parser_string_from_properties + // (both default to "none", both accept the + // "built_in_analyzer" alias) + // + // A normalizer therefore counts as analyzed even though it emits a single token: + // the BE gives such an index a FULLTEXT reader, and match.cpp rejects + // MATCH_PHRASE outright on a FULLTEXT-served index whose support_phrase is + // absent. Dropping the key there would turn a working (degenerate, single-term) + // phrase query into a hard error. + private static boolean isTokenizedInvertedIndex(Map properties) { + if (!InvertedIndexProperties.getPreferredAnalyzer(properties).isEmpty()) { + return true; + } + return !InvertedIndexProperties.INVERTED_INDEX_PARSER_NONE + .equalsIgnoreCase(InvertedIndexProperties.getInvertedIndexParser(properties)); + } + public Index() { this.indexName = null; this.columns = null; diff --git a/fe/fe-core/src/test/java/org/apache/doris/catalog/IndexTest.java b/fe/fe-core/src/test/java/org/apache/doris/catalog/IndexTest.java index 87ba17a7568946..b6736b01fdbdb3 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/catalog/IndexTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/catalog/IndexTest.java @@ -23,7 +23,9 @@ import org.junit.Test; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; +import java.util.Map; public class IndexTest { @@ -161,4 +163,69 @@ public void testGetColumnUniqueIds() { Assert.assertEquals(Integer.valueOf(102), reverseUniqueIds.get(1)); Assert.assertEquals(Integer.valueOf(101), reverseUniqueIds.get(2)); } + + private static Map invertedProperties(String... keyValues) { + Map properties = new HashMap<>(); + for (int i = 0; i < keyValues.length; i += 2) { + properties.put(keyValues[i], keyValues[i + 1]); + } + return properties; + } + + private static Index invertedIndex(Map properties) { + List columns = new ArrayList<>(); + columns.add("body"); + return new Index(1, "idx_body", columns, IndexType.INVERTED, properties, ""); + } + + // support_phrase asks the BE to store the position of every term. A position is + // only observable when a query can supply a second term to match against it, and + // below should_analyzer() the query string is never split -- so the option is kept + // exactly where the BE would run an analyzer and dropped everywhere else. Dropping + // it here keeps the option out of the tablet metadata entirely instead of having + // every BE consumer second-guess it. + @Test + public void testSupportPhraseKeptForTokenizingIndexes() { + Assert.assertEquals("true", + invertedIndex(invertedProperties("parser", "english")) + .getProperties().get("support_phrase")); + Assert.assertEquals("true", + invertedIndex(invertedProperties("analyzer", "my_analyzer")) + .getProperties().get("support_phrase")); + // An explicit value survives untouched. + Assert.assertEquals("false", + invertedIndex(invertedProperties("parser", "english", "support_phrase", "false")) + .getProperties().get("support_phrase")); + } + + // A normalizer must keep support_phrase, even though it emits a single token. + // BE's get_analyzer_name_from_properties() falls back to the "normalizer" key, + // so should_analyzer() is TRUE for such an index: it gets a FULLTEXT reader, + // and match.cpp rejects MATCH_PHRASE outright when support_phrase is absent + // from a FULLTEXT-served index. Dropping the key here would turn a working + // (degenerate, single-term) phrase query into a hard error. + @Test + public void testSupportPhraseKeptForNormalizerIndexes() { + Assert.assertEquals("true", + invertedIndex(invertedProperties("normalizer", "my_normalizer")) + .getProperties().get("support_phrase")); + // An analyzer still wins over the normalizer fallback. + Assert.assertEquals("true", + invertedIndex(invertedProperties("analyzer", "a", "normalizer", "n")) + .getProperties().get("support_phrase")); + } + + @Test + public void testSupportPhraseDroppedForNonTokenizingIndexes() { + // parser=none is the untokenized lane spelled out. + Assert.assertFalse(invertedIndex(invertedProperties("parser", "none")) + .getProperties().containsKey("support_phrase")); + Assert.assertFalse( + invertedIndex(invertedProperties("parser", "NONE", "support_phrase", "true")) + .getProperties().containsKey("support_phrase")); + // A user-written option on a keyword index is dropped as well. + Assert.assertFalse( + invertedIndex(invertedProperties("support_phrase", "true", "ignore_above", "256")) + .getProperties().containsKey("support_phrase")); + } } diff --git a/regression-test/suites/index_p0/test_index_meta.groovy b/regression-test/suites/index_p0/test_index_meta.groovy index d7647f2a9a3e17..bcaaf2922b12e9 100644 --- a/regression-test/suites/index_p0/test_index_meta.groovy +++ b/regression-test/suites/index_p0/test_index_meta.groovy @@ -71,7 +71,7 @@ suite("index_meta", "p0") { assertEquals(show_result[1][4], "name") assertEquals(show_result[1][10], "INVERTED") assertEquals(show_result[1][11], "index for name") - assertEquals(show_result[1][12], "(\"lower_case\" = \"true\", \"parser\" = \"none\", \"support_phrase\" = \"true\")") + assertEquals(show_result[1][12], "(\"lower_case\" = \"true\", \"parser\" = \"none\")") // add index on column description sql "create index idx_desc on ${tableName}(description) USING INVERTED PROPERTIES(\"parser\"=\"standard\") COMMENT 'index for description';" @@ -90,7 +90,7 @@ suite("index_meta", "p0") { assertEquals(show_result[1][4], "name") assertEquals(show_result[1][10], "INVERTED") assertEquals(show_result[1][11], "index for name") - assertEquals(show_result[1][12], "(\"lower_case\" = \"true\", \"parser\" = \"none\", \"support_phrase\" = \"true\")") + assertEquals(show_result[1][12], "(\"lower_case\" = \"true\", \"parser\" = \"none\")") assertEquals(show_result[2][2], "idx_desc") assertEquals(show_result[2][4], "description") assertEquals(show_result[2][10], "INVERTED")