From d9576cee883140d04ec7319232f705e78919dd8b Mon Sep 17 00:00:00 2001 From: airborne12 Date: Thu, 27 Aug 2026 00:16:53 +0800 Subject: [PATCH] [fix](index) Drop support_phrase from indexes that do not tokenize Problem Summary: `support_phrase=true` asks the BE to persist term positions, and a position is only observable when a query can supply a second term to match it against. `InvertedIndexAnalyzer::get_analyse_result()` short-circuits on `!should_analyzer()` and returns the entire search string as ONE term, for every analysis purpose. Every phrase variant -- MATCH_PHRASE, MATCH_PHRASE_PREFIX, MATCH_PHRASE_EDGE -- sources its terms from there, so a phrase against an index with no analyzer is always a single-term phrase, which is a term query. The option describes something no query can reach. FE was manufacturing exactly that combination. `Index`'s constructor unconditionally defaulted `support_phrase` to `true` for every inverted index, including `parser=none` ones, so a keyword index reached the BE carrying an option it can never honour. Downstream, `IndexReaderHelper::is_need_similarity_score()` read that option alone and reported such an index as scoreable, letting relevance work be planned against positions and norms that were never written. The fix aligns both sides on the same predicate: - FE keeps `support_phrase` only where the BE would run an analyzer, and removes the key otherwise, so new tablet metadata no longer carries it. - BE gains `IndexReaderHelper::persists_scoring_inputs()`, which requires both `support_phrase=yes` and `InvertedIndexAnalyzer::should_analyzer()`. Both `is_need_similarity_score()` overloads route through it. Tablet metadata already written to disk still holds the stale option, so the FE-side change alone is not sufficient and the guard has to be repeated on the read path. `should_analyzer()` is the exact line, and it is subtler than "has a parser": `get_analyzer_name_from_properties()` falls back to the **normalizer** key, so a `normalizer`-only index IS analyzed on the BE and is served by a FULLTEXT reader. `match.cpp` rejects `MATCH_PHRASE` outright on a FULLTEXT-served index whose `support_phrase` is absent, so dropping the key there would turn a working (degenerate, single-term) phrase query into a hard error. `isTokenizedInvertedIndex` is therefore composed from `InvertedIndexProperties.getPreferredAnalyzer()` and `getInvertedIndexParser()` -- the FE mirrors of the two resolvers `should_analyzer` itself uses -- rather than re-reading the property keys by hand, so the two sides cannot drift apart. `testSupportPhraseKeptForNormalizerIndexes` and the normalizer case in `IsNeedSimilarityScoreRequiresATokenizer` pin both halves. ARRAY deserves an explicit note, because the obvious argument does not hold for it. `InvertedIndexUtil::checkInvertedIndexParser` forces every ARRAY inverted index to `parser=none`, yet an ARRAY row emits one term per element and `SniiIndexColumnWriter::_add_array_values` does advance positions between them (CLucene writes the same shape). Those positions are nevertheless unreachable, for the query-side reason above -- not because the document holds a single term. `UntokenizedQueriesCannotObserveAPosition` pins that invariant so a future analyzer change cannot silently invalidate it. `test_index_meta.groovy` asserted the exact `SHOW INDEX` property string for a `parser=none` index; `support_phrase` is no longer part of it. The `parser=standard` assertions in the same file are unchanged, which is the intended split. For reference, Elasticsearch rejects the equivalent shape at mapping time: a `keyword` field's `index_options` accepts only `docs` and `freqs`, never `positions`. Release note: None --- be/src/storage/index/index_reader_helper.h | 32 ++++- .../segment/index_reader_helper_test.cpp | 125 ++++++++++++++++++ .../java/org/apache/doris/catalog/Index.java | 42 +++++- .../org/apache/doris/catalog/IndexTest.java | 67 ++++++++++ .../suites/index_p0/test_index_meta.groovy | 4 +- 5 files changed, 256 insertions(+), 14 deletions(-) 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")