github-actions[bot] commented on code in PR #65436:
URL: https://github.com/apache/doris/pull/65436#discussion_r3576554719
##########
be/src/exprs/function/function_search.cpp:
##########
@@ -274,8 +274,13 @@ Status
FunctionSearch::evaluate_inverted_index_with_search_param(
OlapReaderStatistics* outer_stats = index_query_context ?
index_query_context->stats : nullptr;
SCOPED_RAW_TIMER(outer_stats ? &outer_stats->inverted_index_query_timer :
&query_timer_dummy);
- // DSL result cache: reuse InvertedIndexQueryCache with SEARCH_DSL_QUERY
type
- auto* dsl_cache = enable_cache ? InvertedIndexQueryCache::instance() :
nullptr;
+ const bool need_similarity_score =
Review Comment:
This score-cache gate still leaves the top-level `NESTED(...)` DSL path
without scores when NestedGroup read support is enabled. FE will push `score()`
for any `SearchExpression`, and this new `collection_similarity` check disables
the bitmap-only cache, but `FunctionSearch` then takes the `is_nested_query`
branch and returns after `VariantNestedSearchEvaluator::evaluate()` before the
normal scorer collectors run. That nested evaluator builds the inner query with
`weight(false)` and only maps matching element docs to parent rows; it never
calls `CollectionSimilarity::collect`, so score materialization later falls
back to 0.0 for the matched rows. Please either reject `score()` with top-level
`NESTED` search DSL until scoring is implemented, or run the nested inner query
with scoring enabled and aggregate/map element scores into the parent-row
`CollectionSimilarity` before returning.
##########
regression-test/suites/search/test_search_score_cache.groovy:
##########
@@ -0,0 +1,254 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+suite("test_search_score_cache", "p0") {
+ def tableName = "search_score_cache_test"
Review Comment:
This new ordinary single-table regression test should follow the repo's test
conventions from `AGENTS.md`: hardcode the table name in the SQL instead of
using `def tableName`, and keep only the pre-test `DROP TABLE IF EXISTS` rather
than dropping the table again at the end. The suite already has the setup drop
before `CREATE TABLE`, so removing the final drop preserves the debug state
after a failure while still making reruns clean.
##########
regression-test/suites/search/test_search_score_cache.groovy:
##########
@@ -0,0 +1,254 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+suite("test_search_score_cache", "p0") {
+ def tableName = "search_score_cache_test"
+
+ def assertPositiveScores = { result ->
+ assertTrue(result.size() > 0)
+ for (def row : result) {
+ assertTrue(Double.parseDouble(row[1].toString()) > 0.0)
+ }
+ }
+
+ def assertStablePositiveScoreQuery = { tag, warmSql, scoreSql ->
+ def warmResult1 = sql warmSql
+ def warmResult2 = sql warmSql
+ assertEquals(warmResult1, warmResult2)
+
+ def scoreResult1 = sql scoreSql
+ def scoreResult2 = sql scoreSql
+ assertEquals(scoreResult1.collect { it[0] as int },
scoreResult2.collect { it[0] as int })
+ assertPositiveScores(scoreResult1)
+ assertPositiveScores(scoreResult2)
+
+ quickTest(tag, """
+ SELECT id
+ FROM (${scoreSql}) t
+ ORDER BY s DESC
+ """)
+ }
+
+ sql "DROP TABLE IF EXISTS ${tableName}"
+ sql """
+ CREATE TABLE ${tableName} (
+ id INT,
+ status INT,
+ title VARCHAR(200),
+ content VARCHAR(500),
+ v VARIANT,
+ INDEX idx_title(title) USING INVERTED PROPERTIES(
+ "parser" = "english",
+ "support_phrase" = "true"
+ ),
+ INDEX idx_content(content) USING INVERTED PROPERTIES(
+ "parser" = "english",
+ "support_phrase" = "true"
+ ),
+ INDEX idx_v(v) USING INVERTED PROPERTIES(
+ "parser" = "english",
+ "support_phrase" = "true"
+ )
+ ) ENGINE=OLAP
+ DUPLICATE KEY(id)
+ DISTRIBUTED BY HASH(id) BUCKETS 1
+ PROPERTIES ("replication_num" = "1")
+ """
+
+ sql """INSERT INTO ${tableName} VALUES
+ (1, 1, 'apple apple apple banana cherry', 'red fruit sweet apple',
'{"host":"apple banana server"}'),
+ (2, 1, 'apple apple banana date', 'fresh apple banana salad',
'{"host":"apple server cluster"}'),
+ (3, 0, 'banana banana banana grape mango', 'yellow fruit tropical',
'{"host":"banana server"}'),
+ (4, 1, 'apple grape kiwi', 'green fruit fresh apple', '{"host":"green
green apple server"}'),
+ (5, 0, 'mango pineapple coconut', 'tropical fruit exotic',
'{"host":"mango server"}'),
+ (6, 1, 'apple cherry plum apricot fig', 'mixed fruit apple salad',
'{"host":"apple cherry node"}'),
+ (7, 0, 'banana banana coconut papaya', 'smoothie blend tropical',
'{"host":"banana coconut node"}'),
+ (8, 1, 'grape cherry apple apple apple apple', 'wine fruit tart
apple', '{"host":"grape apple node"}')
+ """
+ sql "sync"
+
+ sql """ set enable_common_expr_pushdown = true """
+ sql """ set enable_inverted_index_query_cache = true """
+
+ // SEARCH DSL + score() must execute scorers even when the DSL bitmap
cache is warm.
+ assertStablePositiveScoreQuery(
+ "search_dsl_score",
+ """
+ SELECT
/*+SET_VAR(enable_common_expr_pushdown=true,enable_inverted_index_query_cache=true)
*/
+ id
+ FROM ${tableName}
+ WHERE search('title:apple')
+ ORDER BY id
+ """,
+ """
+ SELECT
/*+SET_VAR(enable_common_expr_pushdown=true,enable_inverted_index_query_cache=true)
*/
+ id, score() AS s
+ FROM ${tableName}
+ WHERE search('title:apple')
+ ORDER BY s DESC
+ LIMIT 10
+ """
+ )
+
+ // MATCH_ANY + score() must bypass bitmap-only inverted index query cache.
+ assertStablePositiveScoreQuery(
+ "match_any_score",
+ """
+ SELECT
/*+SET_VAR(enable_common_expr_pushdown=true,enable_inverted_index_query_cache=true)
*/
+ id
+ FROM ${tableName}
+ WHERE title MATCH_ANY 'apple'
+ ORDER BY id
+ """,
+ """
+ SELECT
/*+SET_VAR(enable_common_expr_pushdown=true,enable_inverted_index_query_cache=true)
*/
+ id, score() AS s
+ FROM ${tableName}
+ WHERE title MATCH_ANY 'apple'
+ ORDER BY s DESC
+ LIMIT 10
+ """
+ )
+
+ // MATCH_ALL + score() should still produce collected BM25 scores after
cache warmup.
+ assertStablePositiveScoreQuery(
+ "match_all_score",
+ """
+ SELECT
/*+SET_VAR(enable_common_expr_pushdown=true,enable_inverted_index_query_cache=true)
*/
+ id
+ FROM ${tableName}
+ WHERE title MATCH_ALL 'apple banana'
+ ORDER BY id
+ """,
+ """
+ SELECT
/*+SET_VAR(enable_common_expr_pushdown=true,enable_inverted_index_query_cache=true)
*/
+ id, score() AS s
+ FROM ${tableName}
+ WHERE title MATCH_ALL 'apple banana'
+ ORDER BY s DESC
+ LIMIT 10
+ """
+ )
+
+ // MATCH_PHRASE + score() should not materialize default 0 scores from
cached bitmaps.
+ assertStablePositiveScoreQuery(
+ "match_phrase_score",
+ """
+ SELECT
/*+SET_VAR(enable_common_expr_pushdown=true,enable_inverted_index_query_cache=true)
*/
+ id
+ FROM ${tableName}
+ WHERE title MATCH_PHRASE 'apple banana'
+ ORDER BY id
+ """,
+ """
+ SELECT
/*+SET_VAR(enable_common_expr_pushdown=true,enable_inverted_index_query_cache=true)
*/
+ id, score() AS s
+ FROM ${tableName}
+ WHERE title MATCH_PHRASE 'apple banana'
+ ORDER BY s DESC
+ LIMIT 10
+ """
+ )
+
+
+ // score() with ordinary filters should still execute scorers after the
indexed bitmap cache is warm.
+ assertStablePositiveScoreQuery(
+ "match_any_with_filter_score",
+ """
+ SELECT
/*+SET_VAR(enable_common_expr_pushdown=true,enable_inverted_index_query_cache=true)
*/
+ id
+ FROM ${tableName}
+ WHERE title MATCH_ANY 'apple' AND status = 1
Review Comment:
This filter case does not currently exercise the ordinary filter path. Every
row that matches `title MATCH_ANY 'apple'` in the fixture already has `status =
1` (ids 1, 2, 4, 6, and 8), while the `status = 0` rows do not match `apple`;
the expected output is therefore identical to `match_any_score`, and `LIMIT 10`
is above the five matching rows. That means the test still passes even if score
top-k is selected before remaining predicates, which is the failure mode
already discussed on the production hunk. Please add at least one high-scoring
`apple` row with `status = 0` and use a limit below the candidate count, or
otherwise make the filtered expected result differ from the unfiltered
`MATCH_ANY 'apple'` result.
--
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]
