github-actions[bot] commented on code in PR #67024:
URL: https://github.com/apache/doris/pull/67024#discussion_r3840364110
##########
be/test/storage/index/ann/faiss_vector_index_test.cpp:
##########
@@ -1465,6 +1466,81 @@ TEST_F(VectorSearchTest, IVFOnDiskSaveLoadAndSearch) {
EXPECT_GT(result.ivf_on_disk_cache_miss_cnt, 0);
}
+// Searching WITHOUT a list cache has to read, not recurse.
+//
+// faiss documents RandomAccessReader::borrow()'s default as "allocates a
buffer
+// and calls read_at()", and CachedRandomAccessReader::read_at() is
implemented by
+// calling borrow(). Falling back to the base borrow() when
+// AnnIndexIVFListCache::instance() is null is therefore unbounded mutual
+// recursion. Every other IVF-on-disk test above installs the cache first,
which
+// is exactly why nothing has ever hit it.
+//
+// exec_env_init installs the cache unconditionally at BE startup, so a
running BE
+// does not reach this today. It is a trap for any unit test that does not
know to
+// install one -- it does not fail, it never returns -- and for any change that
+// makes the cache optional.
+//
+// The search runs on a worker with a deadline because before the fix it does
not
+// return at all: called inline it would hang the whole suite instead of
failing
+// this one case. Everything the worker touches is shared-owned so a hung
thread
+// cannot outlive its arguments.
+TEST_F(VectorSearchTest, IVFOnDiskSearchWithoutTheListCacheStillReads) {
+ ASSERT_EQ(AnnIndexIVFListCache::instance(), nullptr)
+ << "this case is about the no-cache path, but something installed
a cache";
+
+ auto builder = std::make_unique<FaissVectorIndex>();
+ FaissBuildParameter params;
+ params.dim = 32;
+ params.ivf_nlist = 4;
+ params.quantizer = FaissBuildParameter::Quantizer::FLAT;
+ params.index_type = FaissBuildParameter::IndexType::IVF_ON_DISK;
+ builder->build(params);
+
+ const int num_vectors = 200;
+ std::vector<float> vectors;
+ vectors.reserve(static_cast<size_t>(num_vectors) * params.dim);
+ for (int i = 0; i < num_vectors; i++) {
+ auto tmp = vector_search_utils::generate_random_vector(params.dim);
+ vectors.insert(vectors.end(), tmp.begin(), tmp.end());
+ }
+ ASSERT_TRUE(builder->train(num_vectors, vectors.data()).ok());
+ ASSERT_TRUE(builder->add(num_vectors, vectors.data()).ok());
+
+ auto dir = std::make_shared<lucene::store::RAMDirectory>();
+ ASSERT_TRUE(builder->save(dir.get()).ok());
+
+ auto index = std::make_shared<FaissVectorIndex>();
+ index->set_type(AnnIndexType::IVF_ON_DISK);
+ index->set_ivfdata_cache_key_prefix("ut_no_list_cache");
+ ASSERT_TRUE(index->load(dir.get()).ok());
+
+ auto query_vec = std::make_shared<std::vector<float>>(
+ vector_search_utils::generate_random_vector(params.dim));
+ auto roaring = std::make_shared<roaring::Roaring>();
+ for (int i = 0; i < num_vectors; ++i) {
+ roaring->add(i);
+ }
+ auto result = std::make_shared<IndexSearchResult>();
+
+ auto done = std::make_shared<std::promise<Status>>();
+ auto ready = done->get_future();
+ std::thread worker([index, dir, query_vec, roaring, result, done,
num_vectors] {
+ IVFSearchParameters search_params;
+ search_params.nprobe = 4;
+ search_params.roaring = roaring.get();
+ search_params.rows_of_segment = num_vectors;
+ done->set_value(index->ann_topn_search(query_vec->data(), 10,
search_params, *result));
+ });
+ const bool returned = ready.wait_for(std::chrono::seconds(60)) ==
std::future_status::ready;
+ if (!returned) {
+ worker.detach();
Review Comment:
[P1] Isolate the non-returning regression instead of detaching it
This deadline does not make the failure safe. The old recursion allocates on
every frame and can stack-overflow/OOM before 60 seconds; for a hang that does
reach this branch, `FAIL()` returns after `detach()` while the worker continues
reading the raw `AnnIndexIVFListCache` singleton. The immediately following
test creates and destroys that singleton, so the leaked search can race it, and
any exception escaping the worker calls `std::terminate` before the promise is
satisfied. Please run the potentially non-returning call in a killable
subprocess (or expose a bounded/direct reader seam) so timeout/crash is
isolated and the worker is never left in the shared test process.
##########
be/test/storage/index/ann/faiss_vector_index_test.cpp:
##########
@@ -1465,6 +1466,81 @@ TEST_F(VectorSearchTest, IVFOnDiskSaveLoadAndSearch) {
EXPECT_GT(result.ivf_on_disk_cache_miss_cnt, 0);
}
+// Searching WITHOUT a list cache has to read, not recurse.
+//
+// faiss documents RandomAccessReader::borrow()'s default as "allocates a
buffer
+// and calls read_at()", and CachedRandomAccessReader::read_at() is
implemented by
+// calling borrow(). Falling back to the base borrow() when
+// AnnIndexIVFListCache::instance() is null is therefore unbounded mutual
+// recursion. Every other IVF-on-disk test above installs the cache first,
which
+// is exactly why nothing has ever hit it.
+//
+// exec_env_init installs the cache unconditionally at BE startup, so a
running BE
+// does not reach this today. It is a trap for any unit test that does not
know to
+// install one -- it does not fail, it never returns -- and for any change that
+// makes the cache optional.
+//
+// The search runs on a worker with a deadline because before the fix it does
not
+// return at all: called inline it would hang the whole suite instead of
failing
+// this one case. Everything the worker touches is shared-owned so a hung
thread
+// cannot outlive its arguments.
+TEST_F(VectorSearchTest, IVFOnDiskSearchWithoutTheListCacheStillReads) {
+ ASSERT_EQ(AnnIndexIVFListCache::instance(), nullptr)
+ << "this case is about the no-cache path, but something installed
a cache";
+
+ auto builder = std::make_unique<FaissVectorIndex>();
+ FaissBuildParameter params;
+ params.dim = 32;
+ params.ivf_nlist = 4;
+ params.quantizer = FaissBuildParameter::Quantizer::FLAT;
+ params.index_type = FaissBuildParameter::IndexType::IVF_ON_DISK;
+ builder->build(params);
+
+ const int num_vectors = 200;
+ std::vector<float> vectors;
+ vectors.reserve(static_cast<size_t>(num_vectors) * params.dim);
+ for (int i = 0; i < num_vectors; i++) {
+ auto tmp = vector_search_utils::generate_random_vector(params.dim);
+ vectors.insert(vectors.end(), tmp.begin(), tmp.end());
+ }
+ ASSERT_TRUE(builder->train(num_vectors, vectors.data()).ok());
+ ASSERT_TRUE(builder->add(num_vectors, vectors.data()).ok());
+
+ auto dir = std::make_shared<lucene::store::RAMDirectory>();
+ ASSERT_TRUE(builder->save(dir.get()).ok());
+
+ auto index = std::make_shared<FaissVectorIndex>();
+ index->set_type(AnnIndexType::IVF_ON_DISK);
+ index->set_ivfdata_cache_key_prefix("ut_no_list_cache");
+ ASSERT_TRUE(index->load(dir.get()).ok());
+
+ auto query_vec = std::make_shared<std::vector<float>>(
+ vector_search_utils::generate_random_vector(params.dim));
+ auto roaring = std::make_shared<roaring::Roaring>();
+ for (int i = 0; i < num_vectors; ++i) {
+ roaring->add(i);
+ }
+ auto result = std::make_shared<IndexSearchResult>();
+
+ auto done = std::make_shared<std::promise<Status>>();
+ auto ready = done->get_future();
+ std::thread worker([index, dir, query_vec, roaring, result, done,
num_vectors] {
Review Comment:
[P2] Initialize the Doris thread context in this worker
`run_all_tests.cpp` initializes `ThreadContext` only on the main test
thread, and TLS is not inherited by this new `std::thread`. This lambda enters
the full FAISS/CLucene search path without `SCOPED_INIT_THREAD_CONTEXT()`,
contrary to the BE runtime thread-entry invariant; allocator-aware work on this
path would otherwise be charged as orphan/unattached memory. Please include
`runtime/thread_context.h` and initialize the context at the start of the
worker, as the other raw-thread storage tests do.
--
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]
