Repository navigation
Filtered top-k search on the dynamic Vamana index via the search builder - #442
yuejiaointel wants to merge 48 commits into
Conversation
Reuse the MutableBuffer valid/invalid mechanism: a FilteredValidBuilder marks an entry invalid if it is deleted or the predicate rejects its external id. New MutableVamanaIndex::search overload taking a predicate, padding rows with id -1 / +inf and returning per-query found counts. Supplement path checks the predicate before computing the distance. Orchestrator virtual appended last.
Replace the separate filtered search overload with two optional trailing arguments on MutableVamanaIndex::search (an id predicate and a per-query result count). Without a predicate the existing code path runs unchanged. check_and_supplement_search_buffer gets an optional predicate that is checked before the distance is computed. The filter predicate type is now a type-erased std::function, so FilteredValidBuilder is no longer a template. The orchestrator keeps one appended virtual that forwards to the index search.
The optional filter on MutableVamanaIndex::search now only chooses which builder the existing greedy search closure uses; both cases call the same per_thread_batch_search. FilteredValidBuilder derives from ValidBuilder. The buffer top-up in single_search applies the filter when the search closure carries one. The per-query result count out-parameter is removed (callers count non-padding ids). per_thread_batch_search now pads result rows with fewer than k entries (id size_t(-1), distance +inf) instead of reading past the end of the search buffer, and the post-search id translation skips the padding.
Give ValidBuilder an optional predicate and pass it down through internal_search_builder, greedy_search_closure and search() as optional arguments, instead of a separate builder and search path. Filtered-only checks: the supplement fallback skips failing ids, copy-out pads short rows with id -1 and the worst score, translation skips padded ids.
The orchestrator reports filtered search as unsupported for indexes whose search has no filter argument (the concurrent index), instead of failing to compile. In the filtered test, drop the unfiltered-padding section (padding is filtered-only) and add an inner-product case checking padding uses -inf.
ValidBuilder can be given the search buffer. With a filter, it first asks the buffer whether the entry would be dropped anyway (can_skip) and, if so, returns it as invalid without looking up the external id or calling the filter. Results are unchanged.
… drop" This reverts commit 1ecd4e8.
This reverts commit a389cf8.
This reverts commit 73d7128.
rfsaliev
left a comment
There was a problem hiding this comment.
Filtered search for static Vamana to be implemented as well
| // Template so the compiler builds a separate regular-search copy with no filter check. | ||
| template <bool Filtered = false> class ValidBuilder { | ||
| public: | ||
| ValidBuilder(const std::vector<SlotMetadata>& status) | ||
| : status_{status} {} | ||
| ValidBuilder( | ||
| const std::vector<SlotMetadata>& status, | ||
| const IDTranslator* translator = nullptr, | ||
| const std::function<bool(size_t)>* filter = nullptr | ||
| ) |
There was a problem hiding this comment.
- It is not necessary to restrict
filterargument type to std::function, instead, replacebool Filtered = falsetemplate argument withtypename Filter = AcceptAll. - In case if IDTranslator would have a method
try_get_external(id)which may returninvalid_external_idconstant (for deleted/invalid ids), we do not need to work withstatus_anymore:
struct AcceptAll {
// optimized-out by compiler:
template <typename Idx> constexpr bool operator()(Idx) const { return true; }
};
template <typename Filter = AcceptAll> ValidBuilder {
ValidBuilder(
const IDTranslator& translator,
Filter filter = {}
)
...
template <typename I>
PredicatedSearchNeighbor<I> operator()(I i, float distance) const {
const auto external_id = translator_.try_get_external(i);
bool valid = external_id != IDTranslator::invalid_external_id &&
filter_(external_id);
return PredicatedSearchNeighbor<I>(i, distance, valid);
}
...
};Anyway, it would be better to add a new one template class aka FilteredValidBuilder and return it from internal_search_buffer(Filter) overload.
There was a problem hiding this comment.
Good ideas, added a separate FilteredValidBuilder, should not need AcceptAll filter change, and still kept status_. My understanding for the use of status_ is after soft delete, it is actually marked as deleted, but translator table still has the user id. If we just read from translator table we would return deleted points?
There was a problem hiding this comment.
MutableVamanaIndex::delete_entries()callstranslator.delete_external()which invalidates entries inIDTranslator::internal_to_external.MutableVamanaIndex::delete_entry()is called fromdelete_entries()only - can be moved toprivate:visibility for safety.
So, we can be sure, that ;soft-deleted' entries are removed/invalidated in translator
There was a problem hiding this comment.
Right, that is the case for the dynamic index. For the concurrent index, soft delete does not erase the translator entry until consolidate(). Changed on dynamic FilteredValidBuilder to use translator only, and made delete_entry() private. Kept same code for concurrent index
| if constexpr (requires { search.filter; }) { | ||
| if (j >= search_buffer.size()) { // Filtered: fewer than k passed. | ||
| result.index(i, j) = static_cast<I>(-1); | ||
| result.distance(i, j) = distance::comparator(distance)(0.0f, 1.0f) | ||
| ? std::numeric_limits<float>::infinity() | ||
| : -std::numeric_limits<float>::infinity(); | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
IIMHO, it is not necessary to introduce search.filter dependency here:
auto num_results = std::min(num_neighbors, search_buffer.size());
for (size_t j = 0; j < num_results; ++j) {
result.set(search_buffer[j], i, j);
}
for (size_t j = num_results; j < num_neighbors; ++j) {
result.index(i, j) = static_cast<I>(-1);
result.distance(i, j) = distance::comparator(distance)(0.0f, 1.0f)
? std::numeric_limits<float>::infinity()
: -std::numeric_limits<float>::infinity();
}There was a problem hiding this comment.
It is mainly for check_and_supplement_search_buffer. It rarely triggers (only when the search finds fewer valid results than the window), but without the filter the fill would add ids that fail the filter. But I guess we would deal with this function in a separate followup, so removed for now.
| template <typename Buffer> void skip_if_dropped_by(const Buffer& buffer) { | ||
| skip_buffer_ = &buffer; | ||
| skip_ = [](const void* b, float d) { | ||
| return static_cast<const Buffer*>(b)->can_skip(d); | ||
| }; | ||
| } |
There was a problem hiding this comment.
I do not like the design of this pre-validation: there are too many cross-dependencies and injections.
What about just adding can_skip() call to greedy_search():
// Compute distance and update search buffer.
auto dist = distance::compute(distance_function, query, accessor(dataset, id));
if (!search_buffer.can_skip(dist)) {
search_buffer.insert(builder(id, dist));
}There was a problem hiding this comment.
Good idea. Done, greedy_search now skips candidates with can_skip()
| const std::function<bool(size_t)>* filter = nullptr; | ||
| if constexpr (requires { search.filter; }) { | ||
| filter = search.filter; | ||
| } | ||
| check_and_supplement_search_buffer(index, search_buffer, query, filter); |
There was a problem hiding this comment.
It seems like "dirty hack" on "dirty hack":
check_and_supplement_search_buffer() was introduced to resolve cases when a graph is 'splitted' by 'deleted' vectors and BatchIterator was not able to traverse through the whole index without this hack - because 'deleted' vectors are cleaned-out from the search buffer between iterations.
Seems like, due to availability of this hack, you have to introduce the another hack via struct FilteredSearch.
IMHO, we should resolve this, elsewhere, there is the risk of new hacks-on-hack-on-hack in future.
There was a problem hiding this comment.
Agreed. Removed changes from check_and_supplement_search_buffer, can deal with it in a separate pr
| if (!invalid) { | ||
| // Lock-free read; an erased slot has no external id and fails. | ||
| size_t external = translator_->get_external_or(i, no_external_id); | ||
| invalid = external == no_external_id || !(*filter_)(external); |
There was a problem hiding this comment.
What is the desired behavior in case of concurrent thread remaps the entry during filtered search? I.e. what if the (*filter_)(external) was true at the beginning of the operation, but becomes false by the end.
There was a problem hiding this comment.
Good concern! Current behavior:
- greedy search, inline filter checking
- buffer.cleanup() remove invalid entries
- copy back to result table
- translate internal id to external user id
- just added: check filter again for results, and can return less than k results
So if remap happens during search step 5 could catch it
| }; | ||
|
|
||
| // Separate template so the regular search compiles without any filter check. | ||
| template <typename Filter> class FilteredValidBuilder { |
There was a problem hiding this comment.
If Filter somehow interacts with the index, UB is possible.
search already takes comaction_mutex_ shared, if Filter calls a method that also takes this mutex. It is double lock and UB.
Is it a possible case?
There was a problem hiding this comment.
Def a valid concern and is a problem for inline search. This PR will call user's filter in search, and if that filter calls index somehow, that could trigger dead lock because index holds the same lock. But it is an edge case? I have added a few lines to fail in this case + doc in comment
Done. Note: this could break ABI, because the new virtual |
Drop the FilteredSearch closure wrapper: single_search and per_thread_batch_search take the filter as a trailing argument and hand it to the buffer top-up. A filtered concurrent search re-checks the filter on the external ids it returns, and the filter contract is documented.
Reject the reserved external id, make the filter re-entry guard per index, mark translated ids that vanish as invalid, drop identical filtered/unfiltered branches, share the query-type dispatch for filtered_search (now void), and fix the metadata estimate comment.
Filtered top-k search on the static, dynamic and concurrent Vamana index, done inside the search buffer builder.
Idea
The search buffer builder already marks deleted slots as invalid. In filtered search a separate builder also marks a slot invalid when it fails the user filter. Invalid candidates are still searched through but never returned. So the search window counts only points that pass the filter, and no batch round trips are needed.
FilteredNeighborBuilderFilteredValidBuilderFilteredValidBuilderMain changes
ValidBuilderis unchanged from main.FilteredValidBuilder<Filter>takes the filter type as a template parameter. The dynamic builder reads only the translator: a slot with no external id (deleted or empty) is invalid, otherwise the filter decides. The concurrent builder also checks the slot status (deleted or pending). Aninternal_search_builder(filter)overload returns it. Dynamic: L112-L130, L701-L704. Concurrent: L192-L225, L937-L940.greedy_search_closure<Filtered>creates the filtered builder. Static: L506-L540. Dynamic: L706-L740. Concurrent: L961-L1000.size_t(-1)and the worst score, whether or not a filter is set (L587-L596).translate_to_externalleaves the padding id alone. Dynamic: L647-L664. Concurrent: L852-L871.MutableBuffer(it can hold invalid entries), only on the filtered path and chosen at compile time (L622-L629). The invalid entries left in the buffer are removed afterwards (L536-L537).filtered_searchin the baseVamanaInterface. The virtual is declared in the base interface and implemented once inVamanaImpl, so the static and dynamic orchestrators both use it. Interface: L113-L119. Implementation: L296-L307. Public entry points:Vamana::filtered_search(L801-L814) andDynamicVamana::filtered_search(L613-L626).The filter is the optional
filterargument ofsearch()on all three indexes.Optimizations
greedy_searchno longer builds or inserts such a candidate (can_skip), so the filter is not called for it; this applies to all searches. Static and dynamic: L198-L202. Concurrent: L146-L151.search()runs a separatenoinlinebranch; the regular path is the code from main. Static: L620-L651. Dynamic: L785-L803. Concurrent: L1054-L1072.Concurrent index
Extra work in the concurrent index:
load_external: L540-L561). This is the same idea as refactor(dynamic_index): IDTranslator by simple vector for internal to external ID translation #447 (a vector-based internal-to-external table), needed here for the concurrent index. Since Reduce code duplication between concurrent and non-concurrent indexes #439 both indexes shareIDTranslator, so this table change also applies to the dynamic index. This PR carries a copy for now, marked TODO; it can be moved to refactor(dynamic_index): IDTranslator by simple vector for internal to external ID translation #447 and should not be reviewed here.Vamana::filtered_search): L801-L803.get_distance) would take the storage lock again and could deadlock. A thread-local pointer holds the index whose filter is running, and the storage locks of that same index throwANNException(calling another index from the filter is fine): L174-L190, L948-L951.Not in this PR / still to do
filtered_search. Today it uses the batch iterator inbindings/c/src/filtered_search.hpp. C API users (for example Valkey) would get the new path.check_and_supplement_search_buffer) does not apply the filter yet; it rarely runs (only when the search finds fewer valid results than the window), to be handled in a follow-up.Open review items:
🤖 Generated with Claude Code