Skip to content

SCC-5439: Add optional include aggregations to search - #788

Open
7emansell wants to merge 4 commits into
mainfrom
SCC-5439/add-aggs-to-search
Open

SCC-5439: Add optional include aggregations to search#788
7emansell wants to merge 4 commits into
mainfrom
SCC-5439/add-aggs-to-search

Conversation

@7emansell

@7emansell 7emansell commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread routes/resources.js
return Promise.all([
app.resources.search(params, { baseUrl: app.baseUrl }, req),
app.resources.aggregations(params, { baseUrl: app.baseUrl })
]).then(([searchResp, aggsResp]) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha was not even realizing that the aggs came back in the search payload. Updated + added tests

'*Normalized'
]

const AGGREGATION_PACKED_FIELDS = ['location', 'materialType', 'locationBuilding', 'language', 'carrierType', 'mediaType', 'issuance', 'status', 'owner']

@7emansell 7emansell Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved out only bc now this is repeated thrice

@7emansell
7emansell requested a review from nonword July 27, 2026 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants