Copilot commented on code in PR #64555:
URL: https://github.com/apache/doris/pull/64555#discussion_r3425088037
##########
be/test/storage/index/ann/ann_range_search_test.cpp:
##########
@@ -188,7 +188,8 @@ TEST_F(VectorSearchTest, TestEvaluateAnnRangeSearch) {
ASSERT_TRUE(range_search_ctx
->evaluate_ann_range_search(cid_to_index_iterators,
idx_to_cid,
column_iterators,
common_expr_to_slotref_map,
- row_bitmap, stats,
&ann_range_search_executed)
+ row_bitmap.cardinality(),
row_bitmap, stats,
+ &ann_range_search_executed)
Review Comment:
The new `rows_of_segment` argument should represent total segment rows, not
`row_bitmap.cardinality()` (which is 0 here because the bitmap is never
initialized). With the default percent threshold (0.3), this masks/changes the
small-candidate fallback behavior and makes the unit test less representative.
Initialize `row_bitmap` to a realistic candidate set and pass a stable
`rows_of_segment` value instead.
##########
be/test/storage/index/ann/ann_range_search_test.cpp:
##########
@@ -396,10 +398,10 @@ TEST_F(VectorSearchTest,
TestEvaluateAnnRangeSearchStateDoesNotLeakAcrossClones)
segment_v2::AnnIndexStats no_ann_stats;
bool no_ann_range_search_executed = true;
ASSERT_TRUE(segment_without_ann_ctx
- ->evaluate_ann_range_search(no_ann_index_iterators,
idx_to_cid,
- no_ann_column_iterators,
-
common_expr_to_slotref_map, no_ann_row_bitmap,
- no_ann_stats,
&no_ann_range_search_executed)
+ ->evaluate_ann_range_search(
+ no_ann_index_iterators, idx_to_cid,
no_ann_column_iterators,
+ common_expr_to_slotref_map,
no_ann_row_bitmap.cardinality(),
+ no_ann_row_bitmap, no_ann_stats,
&no_ann_range_search_executed)
Review Comment:
Same parameter issue for the non-ANN segment clone: passing
`no_ann_row_bitmap.cardinality()` (0) as `rows_of_segment` makes the
percent-based fallback check meaningless. Initializing the bitmap + passing a
stable segment row count keeps the test semantics aligned with production usage.
##########
be/test/storage/index/ann/ann_range_search_test.cpp:
##########
@@ -372,10 +374,10 @@ TEST_F(VectorSearchTest,
TestEvaluateAnnRangeSearchStateDoesNotLeakAcrossClones)
common_expr_to_slotref_map;
bool ann_range_search_executed = false;
ASSERT_TRUE(segment_with_ann_ctx
- ->evaluate_ann_range_search(ann_index_iterators,
idx_to_cid,
- ann_column_iterators,
-
common_expr_to_slotref_map, ann_row_bitmap,
- ann_stats,
&ann_range_search_executed)
+ ->evaluate_ann_range_search(
+ ann_index_iterators, idx_to_cid,
ann_column_iterators,
+ common_expr_to_slotref_map,
ann_row_bitmap.cardinality(),
+ ann_row_bitmap, ann_stats,
&ann_range_search_executed)
Review Comment:
`rows_of_segment` should be the segment row count (denominator for the
percent threshold), but here it’s passed as `ann_row_bitmap.cardinality()` (0).
Initialize the bitmap to a realistic candidate set and pass a stable segment
row count so the fallback logic isn’t accidentally bypassed.
##########
be/test/storage/index/ann/ann_range_search_test.cpp:
##########
@@ -477,7 +479,8 @@ TEST_F(VectorSearchTest,
TestEvaluateAnnRangeSearchUsesSourceColumnIndexForSlotM
ASSERT_TRUE(range_search_ctx
->evaluate_ann_range_search(cid_to_index_iterators,
idx_to_cid,
column_iterators,
common_expr_to_slotref_map,
- row_bitmap, stats,
&ann_range_search_executed)
+ row_bitmap.cardinality(),
row_bitmap, stats,
+ &ann_range_search_executed)
Review Comment:
`rows_of_segment` is meant to be the segment row count, but this passes
`row_bitmap.cardinality()` while `row_bitmap` is empty. This can
unintentionally change behavior under the default percent threshold (0.3).
Consider initializing `row_bitmap` to a realistic candidate set and pass a
fixed segment row count here.
##########
be/test/storage/index/ann/ann_range_search_test.cpp:
##########
@@ -868,7 +871,7 @@ TEST_F(VectorSearchTest,
TestEvaluateAnnRangeSearch_DimensionMismatch) {
auto st = range_search_ctx->evaluate_ann_range_search(
cid_to_index_iterators, idx_to_cid, column_iterators,
common_expr_to_slotref_map,
- row_bitmap, stats, nullptr);
+ row_bitmap.cardinality(), row_bitmap, stats, nullptr);
Review Comment:
The newly added `rows_of_segment` parameter should be the full segment row
count; passing `row_bitmap.cardinality()` (0) makes the fallback logic behave
differently than it would in production. Initializing the bitmap and passing a
stable segment-row value makes this dimension-mismatch test more realistic.
##########
be/test/storage/index/ann/ann_range_search_test.cpp:
##########
@@ -291,7 +292,8 @@ TEST_F(VectorSearchTest, TestEvaluateAnnRangeSearch2) {
ASSERT_TRUE(range_search_ctx
->evaluate_ann_range_search(cid_to_index_iterators,
idx_to_cid,
column_iterators,
common_expr_to_slotref_map,
- row_bitmap, stats,
&ann_range_search_executed)
+ row_bitmap.cardinality(),
row_bitmap, stats,
+ &ann_range_search_executed)
Review Comment:
Same issue here: `rows_of_segment` is currently passed as
`row_bitmap.cardinality()` even though `row_bitmap` hasn’t been initialized to
represent the candidate set. This can inadvertently affect the fallback
decision under the default percent threshold and weakens the test’s fidelity.
--
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]
