github-actions[bot] commented on code in PR #69199:
URL: https://github.com/apache/doris/pull/69199#discussion_r4294093301
##########
fe/fe-catalog/src/main/java/org/apache/doris/catalog/Index.java:
##########
@@ -75,10 +75,9 @@ public Index(long indexId, String indexName, List<String>
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))
{
+ if
(!InvertedIndexProperties.INVERTED_INDEX_PARSER_NONE.equalsIgnoreCase(getInvertedIndexParser())
Review Comment:
[P2] Preserve normalizer scoring when changing position defaults
Removing the default `true` also makes
`IndexReaderHelper::is_need_similarity_score()` false for normalizer-only
indexes. For a new lowercase-normalizer index containing `FOO`, `SELECT id,
score() AS s FROM t WHERE v MATCH 'foo' AND score()>0 ORDER BY s DESC LIMIT 10`
previously returned a positive-scoring hit. Now `MatchPredicateCollector` skips
the field and `TextIndexReader` disables scoring; `CollectionSimilarity`
substitutes zero for missing entries, so the score filter silently removes that
matching row. This query has no phrase predicate, so fixing the phrase gates
does not address it. Preserve scoring independently from phrase-position
storage, or keep the previous effective setting until this case is supported,
and cover normalizer scoring/min-score queries.
##########
fe/fe-catalog/src/main/java/org/apache/doris/catalog/Index.java:
##########
@@ -75,10 +75,9 @@ public Index(long indexId, String indexName, List<String>
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))
{
+ if
(!InvertedIndexProperties.INVERTED_INDEX_PARSER_NONE.equalsIgnoreCase(getInvertedIndexParser())
+ || StringUtils.isNotEmpty(this.properties.get(
+
InvertedIndexProperties.INVERTED_INDEX_ANALYZER_NAME_KEY))) {
Review Comment:
[P2] Preserve phrase predicates on normalizer-only indexes
For `PROPERTIES("normalizer"="lowercase")` with omitted `support_phrase`,
this removes the previously injected `true`. The BE still classifies that index
as `FULLTEXT` because `should_analyzer()` includes normalizers, so a query such
as `v MATCH_PHRASE 'foo'` over a stored `FOO` now fails with "phrase queries
require setting support_phrase = true" in `match.cpp` /
`inverted_index_iterator.cpp`; prefix/edge predicates are gated too.
`INDEX_INVALID_PARAMETERS` is terminal rather than a scan fallback. A
normalizer emits one keyword, and singleton phrase lowering already produces a
term/expansion query that needs no positions. Please adapt those capability
gates for docs-only normalizers and cover these predicates alongside the new
default.
##########
be/src/storage/index/inverted/analysis/inverted_index_parser.cpp:
##########
@@ -121,7 +121,11 @@ std::string
get_parser_phrase_support_string_from_properties(
it != properties.end()) {
return it->second;
}
- return INVERTED_INDEX_PARSER_PHRASE_SUPPORT_NO;
+ const auto analyzer = properties.find(INVERTED_INDEX_ANALYZER_NAME_KEY);
+ return get_parser_string_from_properties(properties) !=
INVERTED_INDEX_PARSER_NONE ||
Review Comment:
[P2] Keep TOKENIZE's omitted-position default unchanged
`FunctionTokenize::execute_impl` also calls this helper
(`be/src/exprs/function/function_tokenize.cpp:247`), and `_do_tokenize` uses
the result to include `"position"` in every JSON object. An existing call such
as `TOKENIZE('alpha beta', '"parser"="english"')` therefore changes from
token-only output to output with positions, although this PR is scoped to index
defaults. The unchanged `test_tokenize` and custom-analyzer regression goldens
still expect token-only objects. Give TOKENIZE its own omitted=`false`
behavior, preserving explicit `true`, so the physical-index default does not
silently change this SQL output contract.
##########
be/test/storage/index/inverted/analysis/inverted_index_parser_test.cpp:
##########
@@ -130,6 +130,29 @@ TEST_F(InvertedIndexParserTest,
TestGetParserPhraseSupportStringFromProperties)
EXPECT_EQ(get_parser_phrase_support_string_from_properties(properties),
INVERTED_INDEX_PARSER_PHRASE_SUPPORT_NO);
+ properties[INVERTED_INDEX_PARSER_KEY] = INVERTED_INDEX_PARSER_NONE;
+ EXPECT_EQ(get_parser_phrase_support_string_from_properties(properties),
+ INVERTED_INDEX_PARSER_PHRASE_SUPPORT_NO);
+
+ properties[INVERTED_INDEX_PARSER_KEY] = INVERTED_INDEX_PARSER_ENGLISH;
+ EXPECT_EQ(get_parser_phrase_support_string_from_properties(properties),
+ INVERTED_INDEX_PARSER_PHRASE_SUPPORT_YES);
Review Comment:
[P2] Update existing BE fixtures for the new phrase default
The new helper expectation is correct, but
`SniiWriterTest.BuiltinAnalyzerNameStillInitialises`
(`be/test/storage/index/inverted/snii/snii_writer_test.cpp:696-712`) still
passes only `parser=english` and expects `kDocsOnly` because `support_phrase`
is absent. `SniiIndexColumnWriter::init` now selects `kDocsPositions` for that
exact input, so the existing assertion fails.
`CollectionStatisticsTest.CollectSkipsIndexWithoutSimilarityScore` likewise
omits the flag and expects the old false scoring gate. Update the
default-sensitive expectations, and set explicit `support_phrase=false` in
fixtures intended to exercise disabled phrase/scoring behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
