Aggregation Extension

[jamesfisher-geo]

Related Issue(s):

• Add support for the STAC API Aggregation Extension #567

Description:

Adds base support for the STAC API Aggregation Extension

PR Checklist:

• [ x ] pre-commit hooks pass locally
• [ x ] Tests pass (run make test)
• Documentation has been updated to reflect changes, if applicable, and docs build successfully (run make docs)
• Changes are added to the CHANGELOG.

[drnextgis]

That's great to see movement in that direction! I have some questions/comments.

1. AggregationExtensionPostRequest is defined but not used.
2. Is it meant to be used with the filter extension or solely with the search API?
3. Are there any changes that need to be made in stac-fastapi-pgstac or in pgstac to make it compatible with that backend?

Thanks in advance.

[jamesfisher-geo]

Thank you for the comments @drnextgis

1. AggregationExtensionPostRequest is defined but not used.

Ah, yes. The aggregation extension spec defines the methods for /aggregate and /aggregation should be get or post. I will add post to the available methods.

2. Is it meant to be used with the filter extension or solely with the search API?

The aggregation extension spec as-is relies only on the core item-search afaik. However, the one implementation in stac-server includes cql2 query parameter, meaning it relies on the Filter extension. @philvarner may have an opinion on if that is something specific to the implementation, or if there are updates needed in the Aggregation extension spec.

3. Are there any changes that need to be made in stac-fastapi-pgstac or in pgstac to make it compatible with that backend?

I am working on an implementation for elasticsearch and opensearch. I am not sure how an implementation would be handled in pgstac.

[philvarner]

can filter be included in this? I'm not sure how the extensions interact.

[jamesfisher-geo]

Yeah, I have tested with filter included on an implementation and it works great.

I am also not clear on how extension interact. For instance, could collections, datetime, bbox, and intersects be excluded here since they are part of the core search? I'm inclined to keep so the request class correlates directly to the extension spec.

[philvarner]

yeah, i think you should extend BaseSearchGetRequest and BaseSearchPostRequest and just add the aggregations param. You also pickup limit that's ignored, but that's find for a datamodel like this since there's a lot less duplication

[jamesfisher-geo]

I have changed it to be build off base Search and Filter. This makes the implementation dependent on the Filter extension. But I don't think that is an issue.

[vincentsarago]

@jamesfisher-gis sorry I'm just realizing this now but I don't think we should do this, at least I don't see in the aggregate extension why the filter attributes should be enabled with the aggregations ones.

I think it will be up to the implementers to add the filter extension and also handle them in the client.

[jamesfisher-geo]

Hey. That makes sense.

I have a couple other small bug fixes for aggregation. I will submit a PR today that removes the Filter extension dependency.

[vincentsarago]

maybe wait because I will submit a PR that takes care of #713 soon

[vincentsarago]

@jamesfisher-gis in fact, go ahead because my PR is a no-go in fact 😓

[jamesfisher-geo]

Hey @philvarner let me know what you think of the changes to this PR. I think it is close to being complete.

[jonhealy1]

Need to resolve conflicts too

[jonhealy1]

LGTM - nice work

[jonhealy1]

Let's get @vincentsarago thoughts on this

[vincentsarago]

@jamesfisher-gis thanks for the great work.

The overall code looks good, I just have some worries about the inter dependencies of extensions and types submodule.

cc @jonhealy1

[jonhealy1]

Linting

[jamesfisher-geo]

Linting

@jonhealy1 Sorry, fixed

[jonhealy1]

@vincentsarago Hi Vincent. How does this look now?

[vincentsarago]

Great work @jamesfisher-gis