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

Minor updates #136

Merged
merged 2 commits into from
May 3, 2023
Merged

Minor updates #136

merged 2 commits into from
May 3, 2023

Conversation

max-moser
Copy link
Contributor

This PR cherry-picks some minor updates from the closed "bucket aggregations" PR.
Nothing dramatic, just nice-to-haves.

Comment on lines +342 to +344
doc = aggregation.top_hit.hits.hits[0]["_source"].to_dict()
aggregation = aggregation.to_dict()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue linked in the commit won't happen with the code as is right now because we're only using "leaf node" values (integers, strings, etc.).
If one of the values we reuse should be a dictionary however, it would be a DSL object (e.g. AttrList) instead, and cause issues when trying to reindex.

* because we don't need the actual query hits, we just need the
  aggregations
* also, remove the unused temporary variable assignment
* because we're reusing parts of the aggregation query results directly,
  and per default they're wrapped in DSL objects (e.g. AttrList) that
  aren't JSON-serializable
* see: elastic/elasticsearch-dsl-py#913
Copy link

@fenekku fenekku left a comment

Choose a reason for hiding this comment

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

I am honestly not the most familiar with this, but it seems fine by me.

@max-moser
Copy link
Contributor Author

image

@max-moser max-moser merged commit 6a0e561 into inveniosoftware:master May 3, 2023
@max-moser max-moser deleted the mm/fixes branch May 3, 2023 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Released | Done 🚀
Development

Successfully merging this pull request may close these issues.

3 participants