-
Notifications
You must be signed in to change notification settings - Fork 15
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
Aggregation #276
Aggregation #276
Conversation
This is ready for review @jonhealy1. Kind of a big PR 😄. I know there are some class formatting changes coming down the line from stac-fastapi. I'm open to making any required changes in a separate PR when we bump the stac-fastapi version to 3.0.0b2 |
FYI @philvarner |
# if "geometry_geohex_grid_frequency" in aggregations: | ||
# search_body["aggregations"]["geometry_geohex_grid_frequency"] = { | ||
# "geohex_grid": { | ||
# "field": "geometry", | ||
# "precision": geometry_geohex_grid_precision, | ||
# } | ||
# } |
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.
Hi. Is this meant to be commented out?
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.
Yeah, It is a placeholder for when OS/ES add geohex aggregation for geo_shape geometries
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.
It is a bit confusing to have it in the code, though. I do not know when that feature is coming to OS/ES, so I'll remove the commented out code.
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.
You can leave the code in if you want - just add a comment explaining why it's commented out
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.
Okay. I don't see it on any scheduled release for opensearch. So I'll leave it out
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.
it's not AFAIK -- maybe file another GH issue and pop the code that was removed in there, so it's available in future.
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.
Really nice work here. I think everything looks really good 💯
Related Issue(s):
Description:
Adds support for The Aggregation Extension with an added dependency on the Filter Extension. This enables geo-aggregation of geometries and points, taking advantage of Opensearch and Elasticsearch's aggregation engines.
Note that some of the geo-aggregation features will have to be left untested on the Elasticsearch backend implementation because they require a commercial license.
TO DO:
Need to support collection aggregations using the<collection ID>/aggregate
route. Add support in the aggregate function in the core aggregations extension.PR Checklist:
pre-commit run --all-files
)make test
)