-
Notifications
You must be signed in to change notification settings - Fork 236
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
fix: full text search index broken after optimize_indices() #3145
Conversation
Signed-off-by: BubbleCal <bubble-cal@outlook.com>
ACTION NEEDED The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. For details on the error please inspect the "PR Title Check" action. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3145 +/- ##
==========================================
+ Coverage 77.93% 77.98% +0.05%
==========================================
Files 242 242
Lines 81736 81798 +62
Branches 81736 81798 +62
==========================================
+ Hits 63698 63794 +96
+ Misses 14849 14812 -37
- Partials 3189 3192 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
dataset | ||
.optimize_indices(&OptimizeOptions { | ||
num_indices_to_merge: 0, | ||
index_names: None, | ||
}) | ||
.await | ||
.unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's no new data, does optimize indices do anything? Will it in the future? Should you append some new data before running this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's possible, like merging existing indices. but i think it should do nothing for scalar index & FTS, because they always have only 1 delta index. added adding new data here and make the tests cover more cases
this can be reproduced by optimizing FTS without any new data.
the existing tokens could be reordered then the
offsets
broken, leads to the index would return random results.existing tests only verify whether the new data can be queried, so we didn't find this bug.
added a test to query existing data