Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Share prefiltering struct #2858

Merged
merged 6 commits into from
Feb 12, 2025
Merged

Share prefiltering struct #2858

merged 6 commits into from
Feb 12, 2025

Conversation

javitonino
Copy link
Contributor

@javitonino javitonino commented Feb 7, 2025

Instead of cloning the prefiltering results a bunch of times, pass a pointer to it.

Notes:

  • Mixing resource_filters with any other kind of filter was kind of broken. The rust query planner just appended the results of the prefilter query to the existing key_filters, so it made an OR of them (it should be an AND). This PR fixes it by moving the resource filter to the prefilter.
  • The prefilter cannot be easily passed inside the Paragraph/VectorSearchRequest due to being a pointer. This is an implementation detail of the QueryPlanner and could be fixed, but this PR is big enough as it is to add a refactor of the QueryPlanner.
  • Removed key_filters from suggest, it was not exposed to the nucliadb API, so removing it from nidx API as well.
  • Moved "fields" filter to the prefilter. The is it to make future indexing in nidx_vector easier (no longer have to implement this). All resource filters should be in the prefilter.

Copy link

codecov bot commented Feb 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.84%. Comparing base (06c0287) to head (7ddacd4).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2858      +/-   ##
==========================================
- Coverage   86.85%   86.84%   -0.01%     
==========================================
  Files         382      382              
  Lines       24161    24161              
==========================================
- Hits        20984    20983       -1     
- Misses       3177     3178       +1     
Flag Coverage Δ
nucliadb 75.37% <ø> (ø)
nucliadb-ingest 40.97% <ø> (ø)
nucliadb-reader 42.73% <ø> (+0.01%) ⬆️
nucliadb-search 44.30% <ø> (ø)
nucliadb-standalone 47.36% <ø> (ø)
nucliadb-train 45.50% <ø> (ø)
nucliadb-writer 46.89% <ø> (+0.01%) ⬆️
nucliadb_dataset 55.45% <ø> (ø)
nucliadb_models 84.86% <ø> (ø)
nucliadb_sdk 89.67% <ø> (ø)
nucliadb_telemetry 86.56% <ø> (ø)
nucliadb_utils 83.79% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@javitonino javitonino force-pushed the share_prefiltering_struct branch from ca94e79 to bc6864e Compare February 7, 2025 14:08
@javitonino javitonino merged commit 42c4a36 into main Feb 12, 2025
41 checks passed
@javitonino javitonino deleted the share_prefiltering_struct branch February 12, 2025 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants