airborne12 commented on code in PR #69854:
URL: https://github.com/apache/doris/pull/69854#discussion_r4430796090
##########
be/src/storage/index/inverted/reader/collection_statistics.cpp:
##########
@@ -89,6 +91,62 @@ Result<std::shared_ptr<lucene::search::IndexSearcher>>
open_optional_clucene_sta
return std::make_shared<lucene::search::IndexSearcher>(reader, true);
}
+Result<std::unique_ptr<::doris::snii::reader::LogicalIndexReader>>
open_snii_stats_reader(
+ const segment_v2::IndexFileReader& file_reader, const TabletIndex*
index,
+ const io::IOContext* io_ctx) {
+ auto logical_reader = file_reader.open_snii_index(index, io_ctx);
+ if (!logical_reader.has_value()) {
+ auto status = std::move(logical_reader.error());
+ if (status.code() == ErrorCode::INVERTED_INDEX_SNII_NOT_FOUND ||
+ status.code() == ErrorCode::INVERTED_INDEX_FILE_NOT_FOUND ||
+ status.code() == ErrorCode::INVERTED_INDEX_BYPASS) {
+ return
ResultError(Status::Error<ErrorCode::INVERTED_INDEX_NOT_SUPPORTED>(
+ "SNII scoring requires every logical index in the
collection: {}",
+ status.msg()));
+ }
+ return ResultError(std::move(status));
+ }
+ return std::move(logical_reader.value());
+}
+
+// A segment's SNII logical index as the statistics read it, and the norm
length sums kept with it.
+// Outside tests the reader is the one the query path caches in the searcher
cache, so the sums
+// live and are evicted with it, and a query whose segments are cached opens
no index.
+struct SniiStatsReader {
+#ifdef BE_TEST
+ std::unique_ptr<::doris::snii::reader::LogicalIndexReader> owned_reader;
+ segment_v2::NormLengthSums owned_norm_length_sums;
+#else
+ InvertedIndexCacheHandle cache_handle;
+#endif
+ const ::doris::snii::reader::LogicalIndexReader* reader = nullptr;
+ segment_v2::NormLengthSums* norm_length_sums = nullptr;
+};
+
+Status open_snii_stats_reader(const
std::shared_ptr<segment_v2::IndexFileReader>& file_reader,
+ const TabletIndex* index, const io::IOContext*
io_ctx,
+ SniiStatsReader* out) {
+#ifdef BE_TEST
+ out->owned_reader = DORIS_TRY(open_snii_stats_reader(*file_reader, index,
io_ctx));
+ out->reader = out->owned_reader.get();
+ out->norm_length_sums = &out->owned_norm_length_sums;
+#else
+ InvertedIndexSearcherCache::CacheKey
cache_key(file_reader->get_index_file_cache_key(index));
+ if (!InvertedIndexSearcherCache::instance()->lookup(cache_key,
&out->cache_handle)) {
+ auto opened = DORIS_TRY(open_snii_stats_reader(*file_reader, index,
io_ctx));
+ const size_t reader_size = std::max<size_t>(opened->memory_usage(), 1);
+ auto* cache_value = new segment_v2::SniiLogicalReaderCacheValue(
+ std::move(opened), reader_size, UnixMillis(), file_reader);
+ InvertedIndexSearcherCache::instance()->insert(cache_key, cache_value,
&out->cache_handle);
+ }
+ auto* cache_value =
out->cache_handle.get<segment_v2::SniiLogicalReaderCacheValue>();
Review Comment:
**[P1] A cached SNII BKD searcher reaches this `static_cast` and crashes the
BE, on every SNII table.**
The BKD reader caches `SniiBkdSearcherCacheValue` under the same key. After
`CAST(v['n'] AS BIGINT) = 1` on a VARIANT path indexed as BKD, a scoring
`CAST(v['n'] AS STRING) MATCH '1'` hits that entry here. Before this PR the
same query only failed with `logical index is not a text inverted index`.
Reproduced on ASAN: UBSan reports `object is of type
SniiBkdSearcherCacheValue`, then ASan reports a heap-buffer-overflow in
`SniiIndexSource::scoring_stats`, on MoW and DUPLICATE tables. This was with
#69790, #69901 and this PR merged and the #69790 check removed; I did not build
2934ecaaa0 alone. Unit tests cannot catch it, because they take the `BE_TEST`
branch.
**Depends on:** merge #69790 (adds `snii_field_has_no_terms`, which skips
BKD fields) and #69901 (changes the MATCH statistics key) first.
**After they merge, this PR needs to:**
1. Resolve the 2 conflicts in `collection_statistics.cpp`, keeping
`snii_field_has_no_terms` before `open_snii_stats_reader`.
2. Change `DeleteSignRowsCountWhenTheScanReturnsThem` to read `kMatchField`
instead of `L"1"`. Git merges this file cleanly, but the test fails with `Not
such column 1`.
With both done, 32 `score()` suites and 1629 BE unit tests pass on ASAN.
Reference: https://github.com/airborne12/apache-doris/tree/verify-bkd-69854
--
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]
