FilterStore: unifying filter specific logic #452

yashpatel007 · 2023-09-11T20:40:05Z

Does this PR have a descriptive title that could go in our release notes?
Does this PR add any new dependencies?
Does this PR modify any existing APIs?
- Is the change to the API backwards compatible?
Should this result in any changes to our documentation, either updating existing docs or adding new ones?

What does this implement/fix? Briefly explain your changes.

This is an effort to unify all filter specific operations and also abstract away detection of common filters.

Idea is to have a class "FilterManager" that takes charge of all the filter specific data structures, their operations and also make this class responsible for its own synchronization. Makes code in index even more readable and organized.

Any other comments?

…ered_index now takes raw universal label

… accordingly + filter build takes raw label file now

include/in_mem_filter_store.h

NeelamMahapatro · 2023-10-17T16:03:19Z

include/in_mem_filter_store.h

+    // universal label
+    bool _use_universal_label = false;
+    tsl::robin_set<label_type> _mapped_universal_labels;
+    std::set<std::string> _raw_universal_labels;


Label_type , string for single filter.
structure for multiple filter will be decided later.

I mean, it does not hurt to keep this. the logic works fine. but if you think we just need single filter for now. feel free to change the logic.

gopalrs · 2023-10-21T06:43:48Z

apps/build_stitched_index.cpp

@@ -343,7 +344,9 @@ int main(int argc, char **argv)
    path labels_file_to_use = final_index_path_prefix + "_label_formatted.txt";
    path labels_map_file = final_index_path_prefix + "_labels_map.txt";

-    convert_labels_string_to_int(label_data_path, labels_file_to_use, labels_map_file, universal_label);
+    std::set<std::string> raw_universal_label_set = {universal_label};
+    diskann::InMemFilterStore<uint32_t>::convert_labels_string_to_int(label_data_path, labels_file_to_use,


Shouldn't it be the responsibility of filter store to convert labels from strings to int?

I added this here because, the end user does not know if we have an internal mapping and how its done, similarly Index does not know that string labels is a thing, it just expects number labels. So filter store seems like an ideal place to do this conversion. That way it can also keep the mapping logic and also the map itself.

include/abstract_filter_store.h

gopalrs · 2023-10-21T13:25:16Z

include/abstract_filter_store.h

+    DISKANN_DLLEXPORT virtual void save_medoids(const std::string &save_path) = 0;
+    DISKANN_DLLEXPORT virtual void save_label_map(const std::string &save_path) = 0;
+    DISKANN_DLLEXPORT virtual void save_universal_label(const std::string &save_path) = 0;
+


I think we also need a public load() which calls the protected load* methods?

Yes, need to do this, and perhaps remove the "friend" relation between filter store and index class?

gopalrs · 2023-10-21T13:26:12Z

include/abstract_index.h

@@ -103,7 +103,8 @@ class AbstractIndex
    // memory should be allocated for vec before calling this function
    template <typename tag_type, typename data_type> int get_vector_by_tag(tag_type &tag, data_type *vec);

-    template <typename label_type> void set_universal_label(const label_type universal_label);
+    virtual void set_universal_labels(const std::vector<std::string> &raw_universal_labels,


I'm not sure if Index class should get involved in setting universal labels. Why not set it directly in the filter store when it is created?

include/abstract_filter_store.h

gopalrs · 2023-10-21T16:28:31Z

include/abstract_filter_store.h

+// This class is responsible for filter actions in index, and should not be used outside.
+template <typename label_type> class AbstractFilterStore
+{
+  public:


I also didn't find methods to expand and shrink the filter store.

NeelamMahapatro · 2023-10-31T11:49:39Z

include/in_mem_filter_store.h

+    std::set<std::string> _raw_universal_labels;
+
+    // populates _loaction_to labels and _labels from given label file
+    size_t parse_label_file(const std::string &label_file);


rename filter_utils to gt_filter_utils

NeelamMahapatro · 2023-10-31T12:12:11Z

include/in_mem_filter_store.h

+    bool detect_common_filters(uint32_t point_id, bool search_invocation,
+                               const std::vector<label_type> &incoming_labels,
+                               const FilterMatchStrategy filter_match_strategy) override;
+


define 2 detect_common_filters for base points and queries
type of incoming_labels vector<vector<>>
search_invocation - not required if 2 functions are defined

src/in_mem_filter_store.cpp

NeelamMahapatro · 2023-11-15T08:38:43Z

include/index.h

@@ -369,18 +372,14 @@ template <typename T, typename TagT = uint32_t, typename LabelT = uint32_t> clas
    // Filter Support

    bool _filtered_index = false;
-    // Location to label is only updated during insert_point(), all other reads are protected by


insert_point should take string formatted labels (since exposed to user). conversion to label type should happen internally

…ft/DiskANN into patelyash/filter_handler

1. fix SSD label mapping 2. Removed dynamic index parameter from set_universal_label funtion

- Get converted label to Get Numeric Label and convert_label_string_to_int to convert_label_to_numeric - SSD Label mapping fix - Calculate global_string_to_label in entire large file

rakri

Once we address the comments (mostly about who owns medoids), and test the streaming filtered index, I am ok with this PR, good to go!

rakri · 2024-01-23T06:52:26Z

apps/test_insert_deletes_consolidate.cpp

@@ -311,12 +308,11 @@ void build_incremental_index(const std::string &data_path, diskann::IndexWritePa
    {
        const auto save_path_inc = get_save_filename(save_path + ".after-delete-", points_to_skip,
                                                     points_to_delete_from_beginning, last_point_threshold);
-        std::string labels_file_to_use = save_path_inc + "_label_formatted.txt";
+        std::string labels_file_to_use = save_path_inc + "_label_numeric.txt";


if numeric labels are not exposed, why are we using numeric labels here?

include/abstract_filter_store.h

rakri · 2024-01-23T07:12:00Z

include/abstract_filter_store.h

+    DISKANN_DLLEXPORT virtual void save_medoids(const std::string &save_path) = 0;
+    DISKANN_DLLEXPORT virtual void save_label_map(const std::string &save_path) = 0;
+    DISKANN_DLLEXPORT virtual void save_universal_label(const std::string &save_path) = 0;
+


Yes, need to do this, and perhaps remove the "friend" relation between filter store and index class?

rakri · 2024-01-23T07:22:44Z

src/disk_utils.cpp

@@ -755,9 +768,16 @@ int build_merged_vamana_index(std::string base_file, diskann::Metric compareMetr
    diskann::cout << timer.elapsed_seconds_for_step("building indices on shards") << std::endl;

    timer.reset();
+    const std::string disk_index_path = mem_index_path.substr(0, mem_index_path.size() - 9) + "disk.index";


make this hardcoding easier to parse, what is -9?

rakri · 2024-01-23T07:26:15Z

src/in_mem_filter_store.cpp

+template <typename label_type>
+void InMemFilterStore<label_type>::update_medoid_by_label(const label_type &label, const uint32_t new_medoid)
+{
+    _label_to_medoid_id[label] = new_medoid;


if label_to_medoid_id is property of index, why is it here?

rakri · 2024-01-23T07:28:42Z

src/in_mem_filter_store.cpp

+}
+
+template <typename label_type>
+size_t InMemFilterStore<label_type>::load_medoids(const std::string &labels_to_medoid_file)


check if needed here or not..

rakri · 2024-01-23T07:30:26Z

src/in_mem_filter_store.cpp

+    }
+    map_writer.close();
+}
+


we need a uber-method called save which can call the necessary saves the store requires like label map, etc.

rakri · 2024-01-23T07:32:58Z

src/index.cpp

+// Find common filter between a node's labels and a given set of labels, while
+// taking into account universal label
+template <typename T, typename TagT, typename LabelT>
+bool Index<T, TagT, LabelT>::detect_common_filters(uint32_t point_id, bool search_invocation,


can we remove index::detect_common_filters, and directly call filter_Store?

rakri · 2024-01-23T07:39:20Z

src/index.cpp

@@ -1758,6 +1697,7 @@ void Index<T, TagT, LabelT>::build(const std::string &data_file, const size_t nu
 template <typename T, typename TagT, typename LabelT>
 std::unordered_map<std::string, LabelT> Index<T, TagT, LabelT>::load_label_map(const std::string &labels_map_file)


index need not have a load_label_map itself.. all label related loads should occur in filter_store->load()

rakri · 2024-01-23T07:42:15Z

src/index.cpp

-    diskann::cerr << stream.str() << std::endl;
-    throw diskann::ANNException(stream.str(), -1, __FUNCSIG__, __FILE__, __LINE__);
-}
-
 template <typename T, typename TagT, typename LabelT>
 void Index<T, TagT, LabelT>::parse_label_file(const std::string &label_file, size_t &num_points)


we should remove index::parse_label_file, and call load_labels in the calling function.. _filter_store->populate_labels can be called in build_filtered_index function, or whereever the label file is given in the build pipeline?

yashpatel007 added 4 commits September 11, 2023 16:19

setting up filter handler class

c445d20

build error fix minor

cf389f8

replacing code in index.cpp with filter handler code

2c07ffc

resolving labels build failure

497abca

yashpatel007 changed the title ~~A FilterHandler unifying filter specific logic~~ A FilterManager unifying filter specific logic Sep 12, 2023

yashpatel007 added 3 commits September 12, 2023 13:06

renaming from filter_handler to filter_manager

b5dfd90

fixing build error

bc92303

small error fix

77c5444

yashpatel007 requested review from harsha-simhadri and gopalrs September 12, 2023 17:33

yashpatel007 added 3 commits September 12, 2023 14:00

error fix for disk build

0fa86c2

some minor chnages + small update

1faa5b8

build error fix

ff7955a

yashpatel007 requested a review from rakri September 13, 2023 14:53

yashpatel007 added 4 commits September 13, 2023 11:43

finally fixing disk build & search error

c86f085

seperating .h and .cpp file for filter manager

275afc1

adding DLLEXPORT to methods

91eb6c0

fixing build err

a0cd607

yashpatel007 changed the title ~~A FilterManager unifying filter specific logic~~ FilterManager: unifying filter specific logic Sep 13, 2023

yashpatel007 added 5 commits September 14, 2023 16:35

adding abstract class + renaming filter manager to store + build_filt…

9402f01

…ered_index now takes raw universal label

refac to support multiple universal labels + disk index is refactored…

26aa806

… accordingly + filter build takes raw label file now

minor update

0c73589

minor update

9bcedad

resolving some errors

887e644

yashpatel007 force-pushed the patelyash/filter_handler branch from 9bcedad to 887e644 Compare September 18, 2023 15:09

yashpatel007 changed the title ~~FilterManager: unifying filter specific logic~~ FilterStore: unifying filter specific logic Sep 18, 2023

yashpatel007 added 4 commits September 19, 2023 10:07

small naming changes

a9ab92f

internally updating code to use multiplr universal labels

119ee63

resolving build error with stitched vamana index

332de43

experiment : Matching filetrs based on filter match stratagy

e471cf9

small fix

1cb4aae

NeelamMahapatro reviewed Oct 17, 2023

View reviewed changes

include/in_mem_filter_store.h Outdated Show resolved Hide resolved

NeelamMahapatro reviewed Oct 17, 2023

View reviewed changes

include/in_mem_filter_store.h Show resolved Hide resolved

NeelamMahapatro reviewed Oct 17, 2023

View reviewed changes

gopalrs requested changes Oct 21, 2023

View reviewed changes

NeelamMahapatro reviewed Oct 31, 2023

View reviewed changes

NeelamMahapatro reviewed Nov 15, 2023

View reviewed changes

src/in_mem_filter_store.cpp Outdated Show resolved Hide resolved

NeelamMahapatro reviewed Nov 15, 2023

View reviewed changes

NeelamMahapatro and others added 12 commits November 22, 2023 14:59

Merge branch 'patelyash/filter_handler' of https://github.com/microso…

7edc594

…ft/DiskANN into patelyash/filter_handler

Merge latest main

8ba9475

Merge latest main branch and fix compile error

11f8be4

fix SSD label mapping

e978f98

1. fix SSD label mapping 2. Removed dynamic index parameter from set_universal_label funtion

changing insert_point interface for exposing to user

ccb187c

test it later

9ab9445

fix build errors

3cfaf3e

Renaming Converted label functions and SSD label to medoid fix

4f1e81b

- Get converted label to Get Numeric Label and convert_label_string_to_int to convert_label_to_numeric - SSD Label mapping fix - Calculate global_string_to_label in entire large file

move medoid calculation to index class

a7f6b44

merge main branch

6f0f8f5

fix

68e1dbf

clang formatted

615247a

rakri requested changes Jan 23, 2024

View reviewed changes

rakri and others added 8 commits January 24, 2024 04:37

minor changes as per PR discussion

0b9118f

remove medoid related data and methods to index class

7462bd0

clang format fix

84174bd

fix disk index build

86159e8

fix in builder

ce7db4d

clang fix

82f7182

change raw_labels to populate_labels

e669521

combine load and save method

df38242

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FilterStore: unifying filter specific logic #452

FilterStore: unifying filter specific logic #452

yashpatel007 commented Sep 11, 2023 •

edited

Loading

NeelamMahapatro Oct 17, 2023

yashpatel007 Nov 21, 2023

gopalrs Oct 21, 2023

yashpatel007 Nov 21, 2023

gopalrs Oct 21, 2023

rakri Jan 23, 2024

gopalrs Oct 21, 2023

gopalrs Oct 21, 2023

NeelamMahapatro Oct 31, 2023 •

edited

Loading

NeelamMahapatro Oct 31, 2023 •

edited

Loading

NeelamMahapatro Nov 15, 2023

rakri left a comment

rakri Jan 23, 2024

rakri Jan 23, 2024

rakri Jan 23, 2024

rakri Jan 23, 2024

rakri Jan 23, 2024

rakri Jan 23, 2024

rakri Jan 23, 2024

rakri Jan 23, 2024

rakri Jan 23, 2024

		@@ -1758,6 +1697,7 @@ void Index<T, TagT, LabelT>::build(const std::string &data_file, const size_t nu
		template <typename T, typename TagT, typename LabelT>
		std::unordered_map<std::string, LabelT> Index<T, TagT, LabelT>::load_label_map(const std::string &labels_map_file)

FilterStore: unifying filter specific logic #452

Are you sure you want to change the base?

FilterStore: unifying filter specific logic #452

Conversation

yashpatel007 commented Sep 11, 2023 • edited Loading

What does this implement/fix? Briefly explain your changes.

Any other comments?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NeelamMahapatro Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

NeelamMahapatro Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yashpatel007 commented Sep 11, 2023 •

edited

Loading

NeelamMahapatro Oct 31, 2023 •

edited

Loading

NeelamMahapatro Oct 31, 2023 •

edited

Loading