-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat(elastic): improve indexing performance #833
feat(elastic): improve indexing performance #833
Conversation
Great, thanks! Looks like you need to include I'll review properly later today 👍 |
I see some tests are failing for search. Many of them seem to be concerning the order of results, which is just hard coded? Others are regarding the count of FacetValues. Probably the tests are not updated for the ChannelAware FacetValues? I'm not sure. |
I'll investigate when I review the changes later. Perhaps the ordering is affected by the new method of indexing? |
That is very likely. To be determined if the order in these tests is important or not? |
The order is only important if there is an explicit sort order given in the search query. |
@Ctx() ctx: RequestContext, | ||
@Parent() parent: Omit<SearchResponse, 'facetValues'>, | ||
): Promise<Array<{ facetValue: FacetValue; count: number }>> { | ||
const facetValueIds = parent.items.map(item => item.facetValueIds).flat(); |
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.
I believe this change to the way facetValues are counted is responsible for some of the test failures. This change does not replicate the old behaviour: previously, it would return the facetValues counts for the entire result set. Now it is only returning the count for the current page of results.
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.
I see. We are running this revised plugin for quite some time now but hadn't noticed as we only use this for the items in the current result set. Now I also understand why there were two calls to the Elastic instance before. Should be relatively straightforward to put the old system back in place.
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.
Alright, I'll revert that part 👍
This is now tidied up and merged. Thank you for your work on this! |
As promised, a PR with some performance upgrades for the ElasticSearch plugin.
findByIds()
function ChannelAware, which makes this function not consistent with the others. I'm open to suggestions here.I'm aware this could be further cleaned up, but if that is okay with you I'm leaving that to you for now.