feat(aggregation): materialize topic hierarchy list edges in postprocessing - #727
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Documentation | 8 minor |
| Security | 1 medium |
| CodeStyle | 2 minor |
| Complexity | 1 medium |
🟢 Metrics 28 complexity
Metric Results Complexity 28
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 adds support for materializing topic list edges (relevantVariableList and memberList) during linked edge generation, controlled by a new generate_topic_list_edges configuration flag. Feedback focuses on maintaining the intended 'opt-in safety' design where this feature is disabled by default; the reviewer recommends changing the fallback defaults in several getattr calls from True to False across linked_edge_generator.py, orchestrator.py, and main.py to prevent the feature from being unexpectedly enabled.
…d default fallback to False
…ist edges by default
…for Base DC opt-in safety
…m container builds
069650f to
210a6b2
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the capability to materialize relevantVariableList on Topics and memberList on SVPGs via a new generate_topic_list_edges configuration option, alongside corresponding updates to the orchestrator, CLI, and tests. It also simplifies the deletion logic in deleter.py by removing the thread pool in favor of sequential execution. Feedback on the changes highlights two critical issues: first, a bug in delete_linked_edges where unconditionally deleting generated/TopicLists leads to accidental global data loss of topic list edges across unrelated imports; second, a performance bottleneck in run_topic_list_edges due to an unfiltered global query on the Edge table instead of scoping the query by import_names.
…ecution in orchestrator
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new global calculation step to materialize relevantVariableList on Topics and memberList on StatVarPeerGroups (SVPGs). It updates the orchestrator, deleter, and linked edge generator to support generating and deleting these topic list edges, and exposes this feature via a new --generate_topic_list_edges CLI flag. Additionally, deletions in deleter.py are simplified to run sequentially. Feedback on the changes points out that the import_names parameter in run_topic_list_edges is unused and should be removed to clean up the method signature.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to generate and materialize consolidated topic lists (relevantVariableList and memberList edges) as a global calculation step, including configuration options, orchestrator integration, and corresponding deletion logic in Spanner. It also refactors AggregationDeleter to run deletions sequentially rather than in parallel. The review feedback suggests expanding the fallback namespace checks for topics and peer groups to support direct prefixes like topic/% and svpg/% for custom Data Commons installations.
…rn matching to schema-driven typeOf and predicate domain
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for materializing consolidated topic list edges (relevantVariableList and memberList) as a global calculation step. It updates the LinkedEdgeGenerator to aggregate and export these list edges to Spanner, adds corresponding deletion logic in AggregationDeleter (while also refactoring the deletion process to run sequentially), and exposes this feature via orchestrator configuration and CLI arguments. The feedback recommends ensuring that the generate_topic_list_edges setting is checked within individual calculation steps in addition to the global orchestrator configuration, preventing configuration settings in YAML files from being silently ignored during both generation and deletion phases.
…eps for generate_topic_list_edges
…unrelated stat_var_group_generator edit
gmechali
left a comment
There was a problem hiding this comment.
LGTM from my perspective. pls make sure to also get Vishal's approval :)
# Conflicts: # pipeline/workflow/aggregation-helper/aggregation/deleter_test.py
…th type-based resolution
…mprove literal node variable names
…global edge orchestration
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new global materialized edge generation feature (MATERIALIZED_EDGES) to consolidate topic and peer group list edges into sorted CSV strings and export them to Spanner for O(1) lookups. Feedback highlights a critical runtime issue in deleter.py where Cloud Spanner Partitioned DML is incorrectly used with an array parameter, and suggests refactoring it to use a read-write transaction with chunked deletes. Additionally, an optimization is recommended in materialized_edge_generator.py to avoid redundant and expensive STRING_AGG computations in BigQuery by using an intermediate temporary table.
…iteral node deletion
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new global materialized edge generation feature (MATERIALIZED_EDGES) to materialize consolidated CSV list edges on Topics and StatVarPeerGroups globally (such as relevantVariableList and memberList) along with their corresponding literal nodes in Spanner, enabling O(1) query lookups. It adds the MaterializedEdgeGenerator class, updates the AggregationDeleter to handle deletions of these new edges and literal nodes, integrates the new stage into the AggregationOrchestrator, and exposes a CLI flag to enable it. The review feedback suggests increasing the chunk size for deleting literal nodes in Spanner from 500 to 5000 to reduce sequential transactions and network round-trips, along with updating the corresponding unit test to match this change.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces global materialized edge generation for topic and peer group lists to optimize query lookups in Spanner. It adds a new MaterializedEdgeGenerator to compile child variables into sorted CSV string literals, updates the AggregationDeleter to handle cleanup of these edges and nodes, and integrates this step into the AggregationOrchestrator with new CLI and configuration options. The feedback highlights a high-severity optimization opportunity in delete_topic_list_edges to replace client-side memory buffering and chunked transactions with two Partitioned DML statements using subqueries, which would significantly reduce network roundtrips and memory overhead.
Summary
When importing topics and peer groups, Data Commons writes individual 1-to-1 relationships (
relevantVariableandmember). To make topic lookups fast for Mixer and Explore NL, this PR adds a global postprocessing step that rolls up direct children into sorted comma-separated string lists (relevantVariableListandmemberList) and stores them in Spanner.What Changed
MaterializedEdgeGenerator: Added a dedicated generator for global edge materializations. It aggregates child variables into sorted CSV strings, creates content-hashed string literal records in Spanner'sNodetable, and writes the list edges to Spanner'sEdgetable under provenancegenerated/TopicHierarchyLists.LinkedEdgeGenerator: Cleaned uprun_linked_memberto use schema-driven container type lookups (typeOf) instead of relying on hardcoded DCID prefix matching.AggregationOrchestrator: RegisteredMATERIALIZED_EDGESas a standard global postprocessing step. It can be configured in YAML or enabled via the--generate_topic_list_edgesCLI flag.AggregationDeleter: Addeddelete_topic_list_edgesto cleanly delete generated topic list edges and their referenced literal nodes from Spanner. This pre-run deletion is strictly gated and only executes when the topic list generator is enabled.Follow-ups
run_linked_member: When topics are defined in a base import and custom variables are added in a separate dataset import, support cross-provenance tree traversal while attributing generated edges to the leaf variable's provenance.Testing
uv run pytest(71/71 unit tests passing).calinc-dev.