Type-erase the read_options parameter of SecondaryIndex::NewIterator #13334
Conversation
Summary: The patch type-erases the `read_options` parameter of `SecondaryIndex::NewIterator` using `std::any` and renames the earlier `SecondaryIndexReadOptions` (which only contained options related to similarity search) to `FaissIVFIndexReadOptions`. This enables custom `SecondaryIndex` implementations to have their own read option structures and avoids the need to pass options which are irrelevant to them. At the same time, type erasure preserves the ability to call `NewIterator` polymorphically, which in turn enables us to hide the `FaissIVFIndex` class from the public API. Differential Revision: D68694317
|
This pull request was exported from Phabricator. Differential Revision: D68694317 |
| @@ -134,7 +134,7 @@ class SecondaryIndex { | |||
| // For vector indices, the search target might be a vector, and the iterator | |||
| // might return similar vectors from the index. | |||
| virtual std::unique_ptr<Iterator> NewIterator( | |||
| const SecondaryIndexReadOptions& read_options, | |||
| const std::any& read_options, | |||
There was a problem hiding this comment.
TIL! A quick question about the return type of this function, would it make sense to make it SecondaryIndexIterator and put the class's definition in this public header. Since only a subset of the Iterator APIs are supported, and its behavior is different from a regular iterator.
| const FaissIVFIndexReadOptions* const faiss_idx_options = | ||
| std::any_cast<FaissIVFIndexReadOptions>(&read_options); | ||
|
|
||
| if (!faiss_idx_options) { |
There was a problem hiding this comment.
This means an incorrect option type provided is a run time error as opposed to a compile time error, right? Is there a way to do static type check for an object in std::any?
There was a problem hiding this comment.
Unfortunately no; this is the tradeoff with type erasure. std::any is essentially modern C++'s safer void*.
For the record, I'm actually having second thoughts about this - it might make more sense just to bite the bullet here and remove the NewIterator virtual from the SecondaryIndex interface and just have per-implementation non-virtual NewIterator methods with their own signatures (e.g. FaissIVFIndex::NewIterator, which could take a const FaissIVFIndexReadOptions&). The downside of this approach is that it eliminates the possibility of querying indices polymorphically and requires exposing concrete types like FaissIVFIndex in the public interface, which in turn requires clients to keep track of their own concrete index objects. Having said that, it might be the lesser of two evils from an OO standpoint. WDYT?
There was a problem hiding this comment.
Sounds good, exposing FaissIVFIndex in public header is also aligned with having this FaissIVFIndexReadOptions class in public header.
Summary: The patch type-erases the
read_optionsparameter ofSecondaryIndex::NewIteratorusingstd::anyand renames the earlierSecondaryIndexReadOptions(which only contained options related to similarity search) toFaissIVFIndexReadOptions. This enables customSecondaryIndeximplementations to have their own read option structures and avoids the need to pass options which are irrelevant to them. At the same time, type erasure preserves the ability to callNewIteratorpolymorphically, which in turn enables us to hide theFaissIVFIndexclass from the public API.Differential Revision: D68694317