SCC-5439: Add optional include aggregations to search - #788
Conversation
| return Promise.all([ | ||
| app.resources.search(params, { baseUrl: app.baseUrl }, req), | ||
| app.resources.aggregations(params, { baseUrl: app.baseUrl }) | ||
| ]).then(([searchResp, aggsResp]) => { |
There was a problem hiding this comment.
To get full benefit, let's try to fetch the aggregations and results from ES in a single call when possible. I now see that is complicated by the fact that our aggregations calls are sometimes multiple calls, but in the simplest case, when there are no defined filters, we should be able to retrieve results and aggs in one call. Queries without filters tend to be larger result sets, so demonstrating a performance gain for that case will be instructive.
So for simplicity, let's keep the Promise.all call for cases where there are params.filters set, but let's have the app.resources.search function include the aggregations itself in other cases. The search function can support an optional include_aggregations option in its opts param. When enabled, the search function itself should include the aggregations property in the ES payload. Something like:
const body = bodyForSearch(params)
...
const aggsQueries = aggregationQueriesForParams(params)
body.aggregations = aggsQueries[0].aggregations
Doing that should allow us to see the benefit of combining queries. If we see benefit in testing, we can refactor for clarity and ensure we combine queries to the extent possible for filtered queries too.
There was a problem hiding this comment.
Gotcha was not even realizing that the aggs came back in the search payload. Updated + added tests
…ting aggs from single search
| '*Normalized' | ||
| ] | ||
|
|
||
| const AGGREGATION_PACKED_FIELDS = ['location', 'materialType', 'locationBuilding', 'language', 'carrierType', 'mediaType', 'issuance', 'status', 'owner'] |
There was a problem hiding this comment.
moved out only bc now this is repeated thrice
No description provided.