-
Notifications
You must be signed in to change notification settings - Fork 44
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
refactor: Only index that are needed in indexing store #1193
base: main
Are you sure you want to change the base?
Conversation
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.
This will crash collection creation. If a user creates a schema with no search fields. Then in tenant.go
where we create the search collection it will throw an error.
The best way to test this is to create a collection with no search fields and then update the collection with a field that needs to search indexed.
Yes, this is tested with a collection with no search index and then updating the collection with a search index. This shouldn't crash or error anything. Note that we are not removing the fields from the search but rather just not indexing them when we don't need them. This is mainly optimizing indexing. Do you see any errors or crashes? |
You can try it out like this, and then, |
@@ -579,9 +556,9 @@ func (s *ImplicitSearchIndex) GetSearchDeltaFields(existingFields []*QueryableFi | |||
tsFields = append(tsFields, tsApi.Field{ |
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.
We adding to the tsFields here but all the values would be false
if we have no search fields. What would then happen for typesense?
I was testing this on the background search branch and what I did was not add the field to the tsFields
if there was no search field But this caused the crashes in collection creation
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.
All the fields
false and optional
true mean storing in search but not indexing. This is needed so we don't need to read from the database when we get search results. You should not change that because that will break the search as then search results won't have full data.
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.
Looks good. It might be worth checking if we have an integration test with no search fields just to make sure this doesn't throw errors
Yes, many of the collection tests are not adding a search index. |
Describe your changes
We only index fields in the indexing store that are tagged in the model
How best to test these changes
Issue ticket number and link