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

refactor: Only index that are needed in indexing store #1193

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

himank
Copy link
Collaborator

@himank himank commented May 15, 2023

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

@reviewpad reviewpad bot added the medium label May 15, 2023
Copy link
Contributor

@garrensmith garrensmith left a 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.

@himank
Copy link
Collaborator Author

himank commented May 16, 2023

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?

@himank
Copy link
Collaborator Author

himank commented May 16, 2023

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,
curl -H "content-type:application/json" '127.0.0.1:8081/v1/projects/p1/database/collections/test/createOrUpdate' -XPOST -d '{ "schema": { "title": "test", "properties": { "a": { "type": "string" }, "b": { "type": "string" } } } }

and then,
curl -H "content-type:application/json" '127.0.0.1:8081/v1/projects/p1/database/collections/test/createOrUpdate' -XPOST -d '{ "schema": { "title": "test", "properties": { "a": { "type": "string", "searchIndex": true }, "b": { "type": "string" } } } }

@@ -579,9 +556,9 @@ func (s *ImplicitSearchIndex) GetSearchDeltaFields(existingFields []*QueryableFi
tsFields = append(tsFields, tsApi.Field{
Copy link
Contributor

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

Copy link
Collaborator Author

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.

Copy link
Contributor

@garrensmith garrensmith left a 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

@himank
Copy link
Collaborator Author

himank commented May 16, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants