Extract AnalysisNode's arg_q_dict query composition into a node-free helper
- Dominant language
- Python
- Stars
- 30
- Forks
- 3
- Avg merge
- 9h 22m
- Merged PRs (30d)
- 40
Description
🤖 Written by Claude
## Goal
Extract the **annotation-aware Q composition engine** currently living inside `AnalysisNode` into a node-free helper, so that variant querysets built outside an analysis (the All Variants page, gene pages, tag grids, search results, exports) get the same correctness and performance properties that analysis nodes get today.
This is about *query composition*, and is distinct from #1639 (which is about node-free **column/export config**). The two are complementary and independent — #1639 explicitly classifies the `_get_q` / `NodeCount` filter logic as "incidental, drop for the node-free path", so it does not deliver any of this.
## The machinery worth extracting
`AnalysisNode` composes filters as an **`arg_q_dict`**: `dict[Optional[str], dict[str, Q]]`, where the outer key is the name of the *annotation kwarg the Q depends on* (and `None` means "depends on no annotation"). See `analysis/models/nodes/analysis_node.py`:
- `get_arg_q_dict()` (~line 436) — builds the dict, with caching keyed on `node_version`
- `_get_node_arg_q_dict()` (~line 576) / `merge_arg_q_dicts()` (~line 563) — per-node contribution + merge
- `get_queryset()` (~line 587) — the payoff:
```python
for k, v in a_kwargs.items():
qs = qs.annotate(**{k: v})
if q_and_list := list(arg_q_dict.pop(k, {}).values()):
q = reduce(operator.and_, q_and_list)
qs = qs.filter(q)
```
with the comment: *"If we apply the kwargs at the same time, it can join to the same table twice. We want to go through and apply each annotation then the filters that use it, so that it forces an inner query."*
Then afterwards it applies the `None`-keyed (annotation-free) Qs, and **raises** if any `arg_q_dict` key was left unapplied (`f"arg_q_dict filters {arg_q_dict.keys()} not applied"`) — i.e. it fails loudly rather than silently dropping a filter.
Bundled in the same method, and equally reusable:
- `queryset_requires_distinct` / `inner_query_distinct` (~line 632) — choose between `.distinct()` and a `pk__in` inner query, opt-in because "don't do by default as it's slow" (~line 414)
- `q_all()` / `q_none()` (~line 418) — the canonical match-everything / match-nothing idioms
- contig restriction via `node_queryset_filter_contigs` (~line 623)
- the `**kwargs` protocol threaded through `get_annotation_kwargs()` — `existing_annotation_kwargs`, `common_variants`, `annotation_gnomad_version` — which is how the CohortGenotype common-partition optimisation stays correct (see `snpdb/models/models_cohort.py:521-541`, and #1119 / #1582)
## What exists outside analyses today
`AbstractVariantGrid.get_queryset()` (`snpdb/grids.py:545-563`) is the non-analysis equivalent, and it is hardcoded rather than general:
```python
qs = self._get_base_queryset()
qs = qs.filter(Q(variantallele__isnull=True) | Q(variantallele__genome_build=self.genome_build))
qs, _ = VariantZygosityCountCollection.annotate_global_germline_counts(qs) # always
qs = self.filter_items(request, qs)
if q := self._get_q():
qs = qs.filter(q)
...
qs = qs.annotate(**self._get_grid_only_annotation_kwargs())
return qs.values(*field_names)
```
This is not *broken* for the current single-annotation case — the ordering happens to be right. The problems are structural:
1. **Annotations are unconditional.** `VariantZygosityCountCollection.annotate_global_germline_counts()` installs a `FilteredRelation` (`snpdb/models/models_zygosity_counts.py:53-60`) on *every* variant grid whether or not anything filters on it. There is no mechanism to say "only annotate this if a filter or a visible column needs it".
2. **No general contract.** Any new annotation-dependent filter has to hand-roll its annotate/filter ordering in `get_queryset()`. Adding a second annotation group (e.g. cohort genotype / per-sample zygosity outside an analysis) means either editing the base class or re-deriving the interleaving by hand.
3. **No distinct strategy.** There is no equivalent of `queryset_requires_distinct`; correctness currently relies on each call site individually avoiding row multiplication (e.g. the `variantallele` genome-build filter added for #1626, and `VariantAnnotation.get_overlapping_genes_q()` in `annotation/models/models.py:2116` returning `pk__in` specifically so it can't duplicate rows).
4. **Silent-drop risk.** There is no analogue of the "filters not applied" guard, so a Q referencing a missing annotation alias surfaces as a `FieldError` at execution time rather than a clear composition error.
The same ad-hoc `annotate_global_germline_counts()` call appears in at least six independent non-analysis places — `snpdb/variant_queries.py:48`, `variantopedia/interesting_nearby.py:63`, `genes/views/views.py:238`, `genes/views/views_hotspot_graphs.py:47`, `snpdb/signals/variant_zygosity_preview_extra.py:18`, `snpdb/grids.py:554` — each re-deriving the pattern.
## Proposed shape
A plain, model-free composer, e.g. in `snpdb/` (no dependency on `analysis`):
- a small class holding `arg_q_dict` plus the annotation kwargs, with `add_q(q, requires_annotation=None)` and `merge()`
- a `build_queryset(base_qs)` implementing the interleave / leftover-guard / distinct-strategy logic lifted from `AnalysisNode.get_queryset()`
- `AnalysisNode.get_queryset()` refactored to delegate to it, keeping the DAG-specific parts (parent traversal, node cache, `NodeCount`) where they are
- `AbstractVariantGrid.get_queryset()` refactored to delegate to it, which is what makes annotations conditional
## Candidate consumers
- All Variants grid (#1663) — contig / gene / variant-type filters
- Gene page variant grid, hotspot graphs
- Tagged variants grid, nearby variants
- Node-free CSV export (#1639) — would compose its base queryset through the same path
- Search result variant lists
## Risk / testing
This touches `AnalysisNode.get_queryset()`, which every analysis depends on, so it is materially more invasive than #1639. It needs its own plan before implementation:
- the existing analysis node tests must pass unchanged
- add tests asserting generated SQL (join count / inner-query shape) is unchanged for representative nodes, since the whole point of the interleaving is a SQL-shape property that a row-count assertion will not catch
- `analysis/management/commands/profile_analysis_nodes.py` already benchmarks several of these query patterns and should be used as a before/after check
## Out of scope
- Sample/genotype column config (that is #1639's territory)
- Making the zygosity-count annotation conditional on the All Variants page specifically — note this would *not* help there anyway, because `total_db_ref` / `total_db_unk` are in the "Default columns" set (`snpdb/migrations/0123_new_vg_columns_and_custom_columns.py:24`), so the join is needed to render the grid regardless of filters
## Related
- #1639 — node-free variant grid export (columns/export config; complementary, independent)
- #1663 — All Variants page filters (the motivating case; does **not** depend on this, since all its new filters are annotation-free)
- #1279 — All Variants Grid timeout (sorting on global zygosity counts)
- #1119, #1582 — the `common_variants` / gnomAD-version kwargs protocol that any extraction must preserve
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with AnalysisNode.get_queryset(), get_arg_q_dict(), and AbstractVariantGrid.get_queryset() in analysis/models/nodes/analysis_node.py and snpdb/grids.py. Review the existing analysis node tests and profile_analysis_nodes.py benchmarks before defining the extraction. Done means analysis behavior and SQL join/interleaving shape remain unchanged, non-analysis consumers use the shared composer, and leftover filters fail loudly.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend, backend-api-design, database
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100