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

Explicit 'aggregation_unit' for all spatial aggregates #5141

Closed
jc-harrison opened this issue May 10, 2022 · 0 comments · Fixed by #5278
Closed

Explicit 'aggregation_unit' for all spatial aggregates #5141

jc-harrison opened this issue May 10, 2022 · 0 comments · Fixed by #5278
Labels
enhancement New feature or request FlowAPI Issues related to the FlowKit API FlowMachine Issues related to FlowMachine permissions

Comments

@jc-harrison
Copy link
Member

All spatially-aggregated queries necessarily have a spatial aggregation unit associated with them. For some aggregate query kinds (e.g. unique_subscriber_counts), these are explicitly specified by an aggregation_unit parameter, but for others (e.g. spatial_aggregate, flows) the aggregation unit is specified as a parameter of a location sub-query, rather than a parameter of the top-level aggregate query itself. Nonetheless, it remains the case that any spatial_aggregate or flows query has an associated aggregation unit, which could be deduced by looking at the full query spec.

From the perspective of checking permissions, it would be useful for FlowAPI to know the aggregation unit associated with each aggregate query. It would be preferable if FlowAPI could always extract this information from a top-level aggregation_unit parameter, rather than needing to understand the sub-structure of specific query kinds.

While this could be achieved by adding a top-level aggregation_unit parameter to each query kind along with a validator that checks the top-level aggregation_unit matches the aggregation_unit of the location sub-query, this would add redundancy to the query specification which is not ideal from a usability perspective. I think there is probably a way to do this more elegantly such that the top-level aggregation_unit parameter is not required when specifying a query, but is included (derived from sub-queries as appropriate) when serialising the query, so that FlowAPI can easily find out the spatial aggregation level while the logic for extracting this information resides in the flowmachine server.

Side note: it's not necessarily the case that the aggregation unit can be extracted by recursing through the full query spec and finding any aggregation_unit parameters - e.g. for a joined_spatial_aggregate of a daily_location location sub-query and a displacement metric sub-query, the top-level aggregation unit corresponds to the aggregation_unit parameter of the daily_location sub-query, but the displacement sub-query itself has a location sub-query with an aggregation_unit parameter that may be different from the top-level aggregation unit. I.e. the method for extracting the top-level aggregation unit from sub-query parameters is necessarily query-kind-specific, and therefore the logic for this should live in flowmachine, not in FlowAPI.

@jc-harrison jc-harrison added enhancement New feature or request FlowMachine Issues related to FlowMachine FlowAPI Issues related to the FlowKit API permissions P-Now labels May 10, 2022
@jc-harrison jc-harrison mentioned this issue Jun 1, 2022
8 tasks
@mergify mergify bot closed this as completed in #5278 Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request FlowAPI Issues related to the FlowKit API FlowMachine Issues related to FlowMachine permissions
Projects
None yet
1 participant