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 can reach this cast, which can crash the
BE.**
`InvertedIndexCacheHandle::get<T>()` is a `static_cast`, and
`snii_bkd_index_reader.cpp` stores `SniiBkdSearcherCacheValue` under a key from
the same `get_index_file_cache_key()`. On a cache hit, nothing here checks that
the value is a logical reader.
Trigger, on an SNII table with an analyzed index on a VARIANT column, where
a segment holds the dynamic path `v.n` as numbers and so indexes it with BKD:
1. `CAST(v['n'] AS BIGINT) = 1` puts the BKD searcher of `v.n` in the
searcher cache.
2. A `MATCH` on `v['n']` with `score()` makes the statistics visit that
segment. The lookup here hits the BKD entry.
3. The `BkdSearcher` pointer is used as a `LogicalIndexReader`. On a
merge-on-write table, `norm_length_sums` also lies past the end of the smaller
`SniiBkdSearcherCacheValue`.
Before this PR the same query failed cleanly: the statistics always called
`open_snii_index`, and `open_index` rejects the entry in `require_inverted`.
The cache lookup skips that check, so the error becomes undefined behavior once
step 1 has run. Unit tests do not reach this code, because
`open_snii_stats_reader` takes the `BE_TEST` branch.
I found this by reading the code at 2934ecaaa0 and have not reproduced it on
this PR. The same case on CLucene did crash the BE in #69790's Cloud P0
(`std::bad_variant_access`, fixed there in df74dd4ca4).
Suggested fix: #69790 adds `snii_field_has_no_terms`, which skips such a
field by reading the segment's directory. Keep that check before the cache
lookup when the two PRs are merged; they conflict in this part of
`process_segment`. If this PR merges first, it needs an equivalent check of its
own. Either way, please add a regression case that runs the numeric predicate
first, so that the cache hit path is tested.
--
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]
