Bug fixes for Place Aggregation#666
Conversation
…in place aggregation
…me in place rollups
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Documentation | 4 minor |
| Security | 10 medium |
| CodeStyle | 12 minor |
| Complexity | 2 medium |
🟢 Metrics -6 complexity
Metric Results Complexity -6
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Code Review
This pull request introduces dynamic place aggregation strategies (SUM, MIN, MAX, MEAN) based on statistical variable metadata, refactors the SQL queries to handle these strategies, and updates the orchestrator and tests to support a mandatory output_import_name parameter. The review feedback highlights two important improvements: first, avoiding a JOIN inside the Spanner EXTERNAL_QUERY to prevent execution failures when Spanner Data Boost is enabled, and second, grouping by sv.aggr_strategy directly in the SQL query to eliminate redundant ANY_VALUE evaluations.
# Conflicts: # pipeline/workflow/aggregation-helper/aggregation/aggregation_test.py # pipeline/workflow/aggregation-helper/aggregation/orchestrator.py # pipeline/workflow/aggregation-helper/aggregation/orchestrator_test.py # pipeline/workflow/aggregation-helper/aggregation/place_aggregation_generator.py
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the place aggregation pipeline by introducing dynamic aggregation strategies (SUM, MIN, MAX, MEAN) based on statistical variable metadata, supporting custom output import names, and preserving multi-entity dimensions. It also refactors and expands the integration tests to cover these hierarchical and multi-facet scenarios. The review feedback correctly identifies a critical issue where a SQL JOIN is performed inside an EXTERNAL_QUERY block; because Spanner Data Boost does not support joins or subqueries within external queries, the tables must be queried separately and joined in BigQuery instead.
This PR contains a few fixes & improvements in place aggregations:
allow_multiple_to_place = False, the aggregator previously ranMIN(parent_id)across all containment edges before checking parent types. For child places contained in multiple parent types (e.g., a County contained in both State geoId/06 and Country country/USA), MIN() picked country/USA alphabetically, causing counties to drop out of State rollups.dc/Aggregateprefixing: During multi-stage chained rollups (e.g., County → State → Country), each aggregation stage blindly prepended dcAggregate/ to measurementMethod, resulting in corrupted strings likedcAggregate/dcAggregate/CensusACS5yrSurvey.output_import_name: The code was ignoring the yaml config fieldoutput_import_nameand was generating it's own value. Fixed this by reading it from the config file.CensusACS5yrSurveySubjectTable. So I updated this code to do the same.