-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add parsing method to GeoHashGrid aggregation #24589
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
Add parsing method to GeoHashGrid aggregation #24589
Conversation
javanna
left a comment
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.
left a small comment, LGTM otherwise
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.
can it be null in practice?
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.
Not really, unless the key field was missing in the parsed XContent. I added the non null check just to avoid a NPE in this case.
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.
if it can happen only due to a bug, I would not handle it then. Rather make sure that it is always set through tests or assertions? Or even leave the NPE.
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 left the NPE
cbuescher
left a comment
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.
LGTM
207cf3f to
b9da59c
Compare
|
Thanks @javanna and @cbuescher ! |
This PR adds the parsing logic for the InternalGeoHashGrid aggregation.