[ENH] Implement `MetadataProvider`, `RoaringMetadataFilter`, and refactor `MetadataFilteringOperator` #2847

Sicheng-Pan · 2024-09-24T23:17:33Z

Description of changes

Summarize the changes made by this PR.

Improvements & Bug fixes
- Refactors the MetadataFilteringOperator. Now it should produce the exact set of offset ids that should be contained in the result, so that the next operator can only focus on hydrating the offset ids.
New functionality
- Implement the MetadataProvider enum, which represents an universal interface for metadata access. Currently it can be constructed either from materialized logs or a MetadataSegmentReader.
- Introduce the RoaringMetadataFilter trait, which helps to evaluate a Where clause given a MetadataProvider. $ne and $nin are supported as part of this process

Test plan

How are these changes tested?

This is guaranteed to break existing tests. Only the last PR in the stack addresses the breaking changes, and should pass the existing tests. In particular, any test involving the query node should fail because of compilation errors.

Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Documentation Changes

Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?

N/A

github-actions · 2024-09-24T23:17:42Z

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

Can you think of any use case in which the code does not behave as intended? Have they been tested?
Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
If appropriate, are there adequate property based tests?
If appropriate, are there adequate unit tests?
Should any logging, debugging, tracing information be added or removed?
Are error messages user-friendly?
Have all documentation changes needed been made?
Have all non-obvious changes been commented?

System Compatibility

Are there any potential impacts on other parts of the system or backward compatibility?
Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

Sicheng-Pan · 2024-09-24T23:17:46Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @Sicheng-Pan and the rest of your teammates on Graphite

rust/worker/src/execution/operators/metadata_filtering.rs

HammadB · 2024-09-25T20:25:50Z

rust/worker/src/execution/operators/metadata_filtering.rs

+ }
+}
+
+pub(crate) struct MetadataLogReader<'me> {


nit - A quick comment on this struct would be helpful

rescrv · 2024-09-27T20:41:03Z

rust/worker/src/execution/operators/metadata_filtering.rs

+ .collect()
+ );
+
+ // A $lte check on metadata should yield matching records


Can we break this test into pieces? Each of these check comments should/could be a separate unit test.

Will break it up in the future when we refactor the unit tests here. Currently the unit tests here have a lot of duplication to setup the test, which could be avoided with some shared utility functions.

sanketkedia · 2024-09-30T19:28:13Z

rust/worker/src/execution/operators/metadata_filtering.rs

+ .filter_by_metadata(&self.key, metadata_value, &Equal)
+ .await?,
+ ),
+ _ => Include(


nit: prefer enumerating all the cases here (even if code is duplicated). This ensure that in future when someone adds a new type of primitive_operator, the compiler forces them to add it here also otherwise it could be missed

I expanded the case here

sanketkedia · 2024-09-30T19:40:36Z

rust/worker/src/execution/operators/metadata_filtering.rs

- return Err(MetadataFilteringError::MetadataFilteringIndexError(e));
- }
+
+ let filtered_compact_oids = if let Some(clause) = input.where_clause.as_ref() {


nit: add comment here that this takes care of filtering the deletes on the log from the metadata segment

sanketkedia · 2024-09-30T20:03:27Z

rust/worker/src/execution/operators/metadata_filtering.rs

+ compact_metadata: BTreeMap<&'me str, BTreeMap<&'me MetadataValue, RoaringBitmap>>,
+ document: BTreeMap<u32, &'me str>,
+ domain: RoaringBitmap,
+ uid_to_oid: BTreeMap<&'me str, u32>,


does this struct need to be BTreeMap or can it be a HashMap? In general, a btreemap has higher overhead than a hash map so prefer using hash map wherever possible

Changed to hashmap when possible

sanketkedia · 2024-09-30T20:04:26Z

rust/worker/src/execution/operators/metadata_filtering.rs

+}
+
+pub(crate) struct MetadataLogReader<'me> {
+ compact_metadata: BTreeMap<&'me str, BTreeMap<&'me MetadataValue, RoaringBitmap>>,


same as below - the outer map can be a hashmap instead of a btreemap

Even the inner map does not have to be strictly a btreemap as we don't have a use-case for dynamically inserting in a sorted fashion. We could keep a Vec<(&MetadataValue, RoaringBitmap)>, push all the data to it, sort it and keep it here. Then on demand, we could search using binary search

Just to add more color to not prefering a btreemap here - btreemaps generally have overhead for e.g. they replicate the key 6 ways. No strong opinion here though

changed to hashmap

sanketkedia · 2024-09-30T20:04:38Z

rust/worker/src/execution/operators/metadata_filtering.rs

+
+pub(crate) struct MetadataLogReader<'me> {
+ compact_metadata: BTreeMap<&'me str, BTreeMap<&'me MetadataValue, RoaringBitmap>>,
+ document: BTreeMap<u32, &'me str>,


same as below - this can be a hashmap instead of a btreemap

Changed to hashmap

sanketkedia

Good abstractions. Nice!

Sicheng-Pan · 2024-09-30T23:32:03Z

Merge activity

Sep 30, 7:32 PM EDT: @Sicheng-Pan started a stack merge that includes this pull request via Graphite.
Sep 30, 7:34 PM EDT: Graphite rebased this pull request as part of a merge.
Sep 30, 7:35 PM EDT: @Sicheng-Pan merged this pull request with Graphite.

…ctor `MetadataFilteringOperator`

Sicheng-Pan mentioned this pull request Sep 24, 2024

[ENH] Implement SignedRoaringBitmap and update where/where_document AST #2846

Merged

1 task

This was referenced Sep 24, 2024

[CLN] Refactor MergeMetadataResultsOperator #2848

Merged

[CLN] Cleanup codebase with the refactored metadata filtering pipeline #2849

Merged

Sicheng-Pan marked this pull request as ready for review September 24, 2024 23:27

Sicheng-Pan force-pushed the 09-24-_enh_implement_metadataprovider_roaringmetadatafilter_and_refactor_metadatafilteringoperator_ branch from 6877cbd to 1567e7f Compare September 25, 2024 18:48

Sicheng-Pan force-pushed the 09-24-_enh_implement_signedroaringbitmap_and_update_where_where_document_ast branch from c079998 to a5b3575 Compare September 25, 2024 19:24

Sicheng-Pan force-pushed the 09-24-_enh_implement_metadataprovider_roaringmetadatafilter_and_refactor_metadatafilteringoperator_ branch from 1567e7f to 0724e2e Compare September 25, 2024 19:24

HammadB reviewed Sep 25, 2024

View reviewed changes

rust/worker/src/execution/operators/metadata_filtering.rs Outdated Show resolved Hide resolved

HammadB reviewed Sep 25, 2024

View reviewed changes

rust/worker/src/execution/operators/metadata_filtering.rs Outdated Show resolved Hide resolved

HammadB reviewed Sep 25, 2024

View reviewed changes

rescrv reviewed Sep 27, 2024

View reviewed changes

sanketkedia reviewed Sep 30, 2024

View reviewed changes

sanketkedia approved these changes Sep 30, 2024

View reviewed changes

Sicheng-Pan force-pushed the 09-24-_enh_implement_signedroaringbitmap_and_update_where_where_document_ast branch from a5b3575 to 6f7a56f Compare September 30, 2024 22:55

Sicheng-Pan force-pushed the 09-24-_enh_implement_metadataprovider_roaringmetadatafilter_and_refactor_metadatafilteringoperator_ branch from 5296fc9 to fd3d48c Compare September 30, 2024 22:55

Sicheng-Pan changed the base branch from 09-24-_enh_implement_signedroaringbitmap_and_update_where_where_document_ast to graphite-base/2847 September 30, 2024 23:32

Sicheng-Pan changed the base branch from graphite-base/2847 to main September 30, 2024 23:32

[ENH] Implement MetadataProvider, RoaringMetadataFilter, and refa…

1e4d67f

…ctor `MetadataFilteringOperator`

Sicheng-Pan force-pushed the 09-24-_enh_implement_metadataprovider_roaringmetadatafilter_and_refactor_metadatafilteringoperator_ branch from fd3d48c to 1e4d67f Compare September 30, 2024 23:33

Sicheng-Pan merged commit 3c63ddc into main Sep 30, 2024
53 of 69 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Implement `MetadataProvider`, `RoaringMetadataFilter`, and refactor `MetadataFilteringOperator` #2847

[ENH] Implement `MetadataProvider`, `RoaringMetadataFilter`, and refactor `MetadataFilteringOperator` #2847

Sicheng-Pan commented Sep 24, 2024 •

edited

Loading

github-actions bot commented Sep 24, 2024

Sicheng-Pan commented Sep 24, 2024 •

edited

Loading

HammadB Sep 25, 2024

rescrv Sep 27, 2024

Sicheng-Pan Sep 30, 2024

sanketkedia Sep 30, 2024

Sicheng-Pan Sep 30, 2024

sanketkedia Sep 30, 2024

sanketkedia Sep 30, 2024 •

edited

Loading

Sicheng-Pan Sep 30, 2024

sanketkedia Sep 30, 2024

sanketkedia Sep 30, 2024

sanketkedia Sep 30, 2024

Sicheng-Pan Sep 30, 2024

sanketkedia Sep 30, 2024

Sicheng-Pan Sep 30, 2024

sanketkedia left a comment

Sicheng-Pan commented Sep 30, 2024 •

edited

Loading

[ENH] Implement MetadataProvider, RoaringMetadataFilter, and refactor MetadataFilteringOperator #2847

[ENH] Implement MetadataProvider, RoaringMetadataFilter, and refactor MetadataFilteringOperator #2847

Conversation

Sicheng-Pan commented Sep 24, 2024 • edited Loading

Description of changes

Test plan

Documentation Changes

github-actions bot commented Sep 24, 2024

Reviewer Checklist

Testing, Bugs, Errors, Logs, Documentation

System Compatibility

Quality

Sicheng-Pan commented Sep 24, 2024 • edited Loading

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

sanketkedia Sep 30, 2024 • edited Loading

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

sanketkedia left a comment

Choose a reason for hiding this comment

Sicheng-Pan commented Sep 30, 2024 • edited Loading

Merge activity

[ENH] Implement `MetadataProvider`, `RoaringMetadataFilter`, and refactor `MetadataFilteringOperator` #2847

[ENH] Implement `MetadataProvider`, `RoaringMetadataFilter`, and refactor `MetadataFilteringOperator` #2847

Sicheng-Pan commented Sep 24, 2024 •

edited

Loading

Sicheng-Pan commented Sep 24, 2024 •

edited

Loading

sanketkedia Sep 30, 2024 •

edited

Loading

Sicheng-Pan commented Sep 30, 2024 •

edited

Loading