Skip to content

refactor: Update StatVarAggregator to use procedural BigQuery SQL features - #702

Open
SandeepTuniki wants to merge 2 commits into
masterfrom
refactor-stat-var-aggregator
Open

refactor: Update StatVarAggregator to use procedural BigQuery SQL features#702
SandeepTuniki wants to merge 2 commits into
masterfrom
refactor-stat-var-aggregator

Conversation

@SandeepTuniki

Copy link
Copy Markdown
Contributor

This is a purely refactoring PR. No functionality changes, or bug fixes.

This PR refactors the statvar aggregation SQL query. Presently the query is constructed through several python helpers as small snippets, and then combined together. But it's hard to logically follow the query's order and flow due to python's loops and conditions. So I refactored it to take advantage of BQ's native procedural features (i.e; variables, conditions, loops, and temp functions). Now the query is linear and easy to follow.

@codacy-production

Copy link
Copy Markdown

Not up to standards ⛔

🔴 Issues 1 medium

Alerts:
⚠ 1 issue (≤ 0 issues of at least minor severity)

Results:
1 new issue

Category Results
Security 1 medium

View in Codacy

🟢 Metrics -8 complexity

Metric Results
Complexity -8

View in Codacy

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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the StatVarAggregator to consolidate the TimeSeries and Observation queries into a single multi-statement SQL script using temporary tables and BigQuery query parameters, alongside adding corresponding unit tests. The feedback suggests keeping the entities and facet columns as JSON type throughout the temporary tables to avoid expensive serialization/deserialization overhead, and simplifying the UpdateFacet temporary function by using a single JSON_SET call with multiple path-value pairs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant