github-actions[bot] commented on code in PR #66877:
URL: https://github.com/apache/doris/pull/66877#discussion_r3819635071
##########
be/test/storage/index/snii/snii_index_reader_count_fallback_test.cpp:
##########
@@ -1423,6 +1527,46 @@ TEST_F(SniiIndexReaderCountFallback,
PublicSingleTermCountFastPathLeavesPrxStats
verify_query("failed ~1");
}
+TEST_F(SniiIndexReaderCountFallback,
CountFastPathRejectsDfBeyondDocumentDomain) {
+ CorruptDfLogicalIndex corrupt;
+ assert_ok(build_corrupt_df_logical_index(/*nullable=*/false, &corrupt));
+ QueryExecutionContext execution(/*enable_query_cache=*/false,
+ /*count_on_index_fastpath=*/true);
+ InvertedIndexQueryInfo query_info;
+ query_info.term_infos.emplace_back("alpha", 0);
+ const std::vector<std::string> terms {"alpha"};
+ bool handled = false;
+ std::shared_ptr<roaring::Roaring> bitmap;
+
+ const Status status = _index_reader->_try_count_only_fastpath(
+ execution.context, InvertedIndexQueryType::MATCH_PHRASE_QUERY,
query_info, terms,
+ &handled, &bitmap, &corrupt.reader);
+
+ EXPECT_TRUE(status.is<ErrorCode::INVERTED_INDEX_FILE_CORRUPTED>()) <<
status;
+ EXPECT_FALSE(handled);
+ EXPECT_EQ(bitmap, nullptr);
+}
+
+TEST_F(SniiIndexReaderCountFallback,
CountFastPathRejectsDfBeyondNonNullDomain) {
Review Comment:
[P2] Exercise the indexed-document bound independently
Both corruption tests currently reuse `kCorruptDf = 100` while `doc_count`
is 10. In the nullable case that means `count > stats.doc_count` is already
true and short-circuits, so this test still passes if the new `count >
stats.indexed_doc_count` check is removed. Please parameterize the corrupt df
(or otherwise make this case use 9 with `doc_count = 10` and `indexed_doc_count
= 8`) so the test actually protects the nullable-domain half of the fix.
##########
be/src/storage/index/snii/snii_index_reader.cpp:
##########
@@ -1094,6 +1096,13 @@ Status SniiIndexReader::_try_count_only_fastpath(
RETURN_IF_ERROR(
::doris::snii::query::count_only_term_df(*logical_reader,
physical_term, &count));
}
+ const auto& stats = logical_reader->stats();
+ if (count > stats.doc_count || count > stats.indexed_doc_count) {
Review Comment:
[P1] Disable this shortcut for existing nullable ARRAY indexes
`df <= indexed_doc_count` does not prove that ARRAY postings exclude
outer-null rows. A vectorized `if(cond, array_value, NULL)` keeps the full
nested array payload and applies only a null map;
`ArrayColumnWriter::append_nullable` then calls `add_array_values` for every
row before `add_array_nulls`, so tokens from an outer-null row remain in `df`.
For two rows with nested `[["alpha"], ["beta"]]` and outer null map `{1,0}`,
valid stats are `doc_count=2`, `indexed_doc_count=1`, and `df(alpha)=1`: this
guard passes, fabrication moves the one id away from null row 0 to row 1, and
COUNT returns 1, whereas normal posting `{0}` is masked to 0. Please gate the
df-only path for nullable ARRAY indexes unless an on-disk capability guarantees
null-row payload was skipped, fix new writes, and add a public-path regression
for this shape.
##########
be/src/storage/index/snii/snii_index_reader.cpp:
##########
@@ -1094,6 +1096,13 @@ Status SniiIndexReader::_try_count_only_fastpath(
RETURN_IF_ERROR(
::doris::snii::query::count_only_term_df(*logical_reader,
physical_term, &count));
}
+ const auto& stats = logical_reader->stats();
Review Comment:
[P1] Bind this check to the actual segment row domain
Both limits here come from the CRC-valid Core metadata in the same index,
and the ordinary query-open path never compares Core `doc_count` with the
segment's real row count. For an actual 10-row segment with posting `{2, 7}`, a
CRC-valid image with DICT `df = 100` and Core `doc_count = indexed_doc_count =
100` passes both strict comparisons, fabricates 100 ids, and SegmentIterator
only clamps that to its real 10-row bitmap; `COUNT_ON_INDEX` can then return 10
instead of 2. Please bind Core `doc_count` to an independently supplied segment
row count before trusting these bounds (and validate the indexed/null
relationship), with a public-path test for the jointly inflated case.
--
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]
