Copilot commented on code in PR #65916: URL: https://github.com/apache/doris/pull/65916#discussion_r3630166258
########## regression-test/suites/ann_index_p0/ann_topn_residual_predicate.groovy: ########## @@ -0,0 +1,66 @@ +// 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("ann_topn_residual_predicate") { + sql "unset variable all;" + sql "set enable_segment_limit_pushdown=true;" + sql "set experimental_enable_virtual_slot_for_cse=true;" + sql "set enable_no_need_read_data_opt=true;" + sql "set parallel_pipeline_task_num=1;" + sql "set enable_sql_cache=false;" + sql "set enable_condition_cache=false;" + sql "set enable_ann_index_result_cache=false;" + sql "set ann_index_candidate_rows_threshold=0;" + + sql "drop table if exists ann_topn_residual_predicate" + sql """ + create table ann_topn_residual_predicate ( + id int not null, + embedding array<float> not null, + index ann_embedding(`embedding`) using ann properties( + "index_type"="hnsw", + "metric_type"="l2_distance", + "dim"="3" + ) + ) duplicate key(id) + distributed by hash(id) buckets 1 + properties("replication_num"="1"); + """ + + sql """ + insert into ann_topn_residual_predicate values + (1, [0.0, 0.0, 0.0]), + (2, [0.1, 0.0, 0.0]), + (3, [0.2, 0.0, 0.0]), + (4, [0.3, 0.0, 0.0]), + (5, [0.4, 0.0, 0.0]), + (6, [0.5, 0.0, 0.0]), + (7, [0.6, 0.0, 0.0]), + (8, [0.7, 0.0, 0.0]), + (9, [0.8, 0.0, 0.0]), + (10, [0.9, 0.0, 0.0]); + """ + sql "sync" + + qt_residual_predicate_topn """ + select id + from ann_topn_residual_predicate + where abs(id) > 2 + order by l2_distance_approximate(embedding, [0.0, 0.0, 0.0]) + limit 2; + """ Review Comment: The PR description says a “debug-point regression case” verifies residual predicates bypass the storage ANN TopN path, but this test only validates final query results and doesn’t assert the internal path/flag. Either (a) update the PR description to match what the test actually verifies, or (b) add an explicit assertion (e.g., via an explain/profile/debug-point hook used by this test framework) that the ANN TopN pushdown path is not taken when residual conjuncts exist. ########## be/src/exec/operator/olap_scan_operator.cpp: ########## @@ -495,6 +495,9 @@ Status OlapScanLocalState::_process_conjuncts(RuntimeState* state) { } auto& p = _parent->cast<OlapScanOperatorX>(); RETURN_IF_ERROR(validate_residual_scan_conjuncts(state, p._push_down_agg_type, _conjuncts)); + if (!_conjuncts.empty()) { + _ann_topn_runtime.reset(); + } Review Comment: This correctness safeguard is subtle (it prevents under-return when residual predicates are applied after an ANN TopN pre-limit). Please add a short comment explaining why any remaining `_conjuncts` must disable ANN TopN pushdown, and (if applicable in this codepath) clarify whether runtime filters are included in `_conjuncts` and are part of the rationale. -- 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]
