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

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

Description of changes

Summarize the changes made by this PR.

Improvements & Bug fixes
- Address all breaking changes in the previous PRs.
New functionality
- N/A

Test plan

How are these changes tested?

The existing tests should pass

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

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

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

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

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

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)

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

rust/worker/src/segment/metadata_segment.rs

 }

+ // DEPRECATED: This exists only for the legacy testing. Please checkout `MetadataFilteringOperator` for the up to date implementation.


curious, why did we not remove it? Is it just more work or are there other considerations?

Simply because there's a lot unit tests depending on this and I'm not planning to refactor them altogether in this stack of PRs.

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

rust/worker/src/segment/metadata_segment.rs

@@ -1164,10 +1164,11 @@ impl MetadataSegmentReader<'_> {
 })
 }

+ // DEPRECATED: This exists only for the legacy testing. Please checkout `MetadataFilteringOperator` for the up to date implementation.
 pub async fn query(


curious, why did we not remove it? Is it just more work or are there other considerations?

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:40 PM EDT: Graphite rebased this pull request as part of a merge.
Sep 30, 7:41 PM EDT: @Sicheng-Pan merged this pull request with Graphite.

*Summarize the changes made by this PR.* - Clean up warnings under rust/types. *How are these changes tested?* - `cargo clippy --all-targets` - [ ] Tests pass locally with `pytest` for python, `yarn test` for js, `cargo test` for rust *Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the [docs repository](https:/chroma-core/docs)?*

@rescrv

*Summarize the changes made by this PR.* - Improvements & Bug fixes - This switches to `debug_struct` over `f.write` as per @rescrv's suggestion. - Fixes a spurious warning about an unused struct field - New functionality - None *How are these changes tested?* We do not test the debug behavior. - [x] Tests pass locally with `pytest` for python, `yarn test` for js, `cargo test` for rust None

…2858) ## Description of changes *Summarize the changes made by this PR.* - Clean up warnings under rust/types. ## Test plan *How are these changes tested?* - `cargo clippy --all-targets` - [ ] 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](https:/chroma-core/docs)?*

*Summarize the changes made by this PR.* - clean up cache, distance, index, storage clippy warnings *How are these changes tested?* - cargo clippy locally - [ ] Tests pass locally with `pytest` for python, `yarn test` for js, `cargo test` for rust *Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the [docs repository](https:/chroma-core/docs)?*

Left some of worker untouched for concurrent PRs. *Summarize the changes made by this PR.* - Cleans up warnings. *How are these changes tested?* - [ ] cargo clippy && cargo test w/ tilt up. *Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the [docs repository](https:/chroma-core/docs)?*

This was referenced Sep 24, 2024

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

Merged

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

Merged

Sicheng-Pan mentioned this pull request Sep 24, 2024

[CLN] Refactor MergeMetadataResultsOperator #2848

Merged

1 task

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

Sicheng-Pan force-pushed the 09-24-_cln_refactor_mergemetadataresultsoperator_ branch from 90b6d82 to 25b4e7a Compare September 25, 2024 01:02

Sicheng-Pan force-pushed the 09-24-_cln_cleanup_codebase_with_the_refactored_metadata_filtering_pipeline branch from 034df37 to 61bd467 Compare September 25, 2024 01:02

Sicheng-Pan force-pushed the 09-24-_cln_refactor_mergemetadataresultsoperator_ branch from 25b4e7a to 19d282c Compare September 25, 2024 18:40

Sicheng-Pan force-pushed the 09-24-_cln_cleanup_codebase_with_the_refactored_metadata_filtering_pipeline branch from 61bd467 to 20da0c5 Compare September 25, 2024 18:40

Sicheng-Pan force-pushed the 09-24-_cln_refactor_mergemetadataresultsoperator_ branch from 19d282c to b0ea68d Compare September 25, 2024 18:48

Sicheng-Pan force-pushed the 09-24-_cln_cleanup_codebase_with_the_refactored_metadata_filtering_pipeline branch from 20da0c5 to 936d0f6 Compare September 25, 2024 18:48

Sicheng-Pan force-pushed the 09-24-_cln_refactor_mergemetadataresultsoperator_ branch from b0ea68d to e5c7aec Compare September 25, 2024 19:25

Sicheng-Pan force-pushed the 09-24-_cln_cleanup_codebase_with_the_refactored_metadata_filtering_pipeline branch from 936d0f6 to 4f86f02 Compare September 25, 2024 19:25

Sicheng-Pan force-pushed the 09-24-_cln_refactor_mergemetadataresultsoperator_ branch from e5c7aec to 0f4d278 Compare September 26, 2024 22:12

Sicheng-Pan force-pushed the 09-24-_cln_cleanup_codebase_with_the_refactored_metadata_filtering_pipeline branch from 4f86f02 to eecff23 Compare September 26, 2024 22:13

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-_cln_refactor_mergemetadataresultsoperator_ branch from 0f4d278 to 5135e49 Compare September 30, 2024 22:56

Sicheng-Pan force-pushed the 09-24-_cln_cleanup_codebase_with_the_refactored_metadata_filtering_pipeline branch from 696d0ad to 95af42d Compare September 30, 2024 22:56

Sicheng-Pan changed the base branch from 09-24-_cln_refactor_mergemetadataresultsoperator_ to graphite-base/2849 September 30, 2024 23:36

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

Sicheng Pan and others added 6 commits September 30, 2024 23:39

[CLN] Cleanup codebase with the refactored metadata filtering pipeline

c0c7d3d

Address comments

d22e6bb

Address comments

d601cd0

rescrv and others added 5 commits September 30, 2024 23:39

[ENH]: 21x faster full text querying (#2833)

afa8d72

Resolve conflicts

0709de3

Add more comments

57505da

Sicheng-Pan force-pushed the 09-24-_cln_cleanup_codebase_with_the_refactored_metadata_filtering_pipeline branch from 200ce7f to 57505da Compare September 30, 2024 23:39

Sicheng-Pan merged commit 14b11e4 into main Sep 30, 2024
69 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Sicheng-Pan commented Sep 24, 2024 •

edited

Loading

Sicheng-Pan commented Sep 24, 2024 •

edited

Loading

github-actions bot commented Sep 24, 2024

sanketkedia Sep 30, 2024

Sicheng-Pan Sep 30, 2024

sanketkedia Sep 30, 2024

Sicheng-Pan Sep 30, 2024

Sicheng-Pan commented Sep 30, 2024 •

edited

Loading

		}

		// DEPRECATED: This exists only for the legacy testing. Please checkout `MetadataFilteringOperator` for the up to date implementation.

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

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

Conversation

Sicheng-Pan commented Sep 24, 2024 • edited Loading

Description of changes

Test plan

Documentation Changes

Sicheng-Pan commented Sep 24, 2024 • edited Loading

github-actions bot commented Sep 24, 2024

Reviewer Checklist

Testing, Bugs, Errors, Logs, Documentation

System Compatibility

Quality

sanketkedia Sep 30, 2024

Choose a reason for hiding this comment

Sicheng-Pan Sep 30, 2024

Choose a reason for hiding this comment

sanketkedia Sep 30, 2024

Choose a reason for hiding this comment

Sicheng-Pan Sep 30, 2024

Choose a reason for hiding this comment

Sicheng-Pan commented Sep 30, 2024 • edited Loading

Merge activity

Sicheng-Pan commented Sep 24, 2024 •

edited

Loading

Sicheng-Pan commented Sep 24, 2024 •

edited

Loading

Sicheng-Pan commented Sep 30, 2024 •

edited

Loading