fix: replicate VEP merge_features HGNC_ID propagation in cache builder

[mwiewior]

Summary

• Replicate VEP's merge_features() HGNC_ID propagation in the Parquet cache builder (storable_to_parquet)
• Uses COALESCE + FIRST_VALUE IGNORE NULLS OVER (PARTITION BY gene_symbol) window function in the transcript dedup query
• Closes 82 HGNC_ID gaps across 7 gene symbols genome-wide (e.g., FGF7P3 30/46 missing, GUSBP5 39/76 missing)
• Guards against cross-pollination for NULL gene_symbol transcripts

Resolves biodatageeks/datafusion-bio-functions#105

Changes

• storable_to_parquet.rs: Modified build_dedup_query() to build an explicit column list for transcripts, replacing gene_hgnc_id with the propagation expression
• hgnc_propagation_tests.rs: 6 new unit tests using synthetic MemTable data covering: basic propagation, no-overwrite, NULL symbol, all-NULL HGNC, multiple symbols, mixed scenario
• real_vep_cache_tests.rs: 1 new integration test against real VEP 115 chr22 fixture verifying zero remaining gaps after propagation

Performance

The window function adds one hash partition over gene_symbol (~20K distinct values). On ~3M transcript rows this is negligible (+1-2s) compared to the existing ROW_NUMBER window + ORDER BY + Parquet write.

Test plan

• All 6 synthetic HGNC propagation tests pass
• Real-data test (hgnc_propagation_fills_all_siblings) passes on chr22 fixture
• All 29 existing ensembl-cache tests pass (no regressions)
• cargo clippy clean
• cargo fmt --check clean
• Rebuild chr9 parquet and verify FGF7P3 all 46 transcripts have HGNC:26671

🤖 Generated with Claude Code

[mwiewior]

@claude pls review

[claude]

Claude finished @mwiewior's task in 1m 39s —— View job

---

Code Review

• Gather context and read changed files
• Review storable_to_parquet.rs changes
• Review hgnc_propagation_tests.rs (new test file)
• Review real_vep_cache_tests.rs changes
• Post comprehensive review feedback

---

Overall this is a clean, focused fix that correctly replicates VEP's merge_features() propagation. The window-function approach is idiomatic and the NULL-symbol guard is the right call. A few things worth noting:

Correctness ✅

The core SQL expression is correct:

COALESCE(gene_hgnc_id,
    CASE WHEN gene_symbol IS NOT NULL
         THEN FIRST_VALUE(gene_hgnc_id) IGNORE NULLS OVER (PARTITION BY gene_symbol)
         ELSE NULL END) AS gene_hgnc_id

• COALESCE ensures existing non-null values are never overwritten.
• The gene_symbol IS NOT NULL guard correctly prevents NULL-symbol transcripts from receiving a propagated ID.
• FIRST_VALUE ... IGNORE NULLS without ORDER BY is non-deterministic in general SQL, but here all siblings sharing a gene_symbol should carry the same HGNC ID, so any non-null value picked is the correct one. Fine as-is.

The _rn handling change at storable_to_parquet.rs:542 is correct — the explicit column list built from the schema naturally excludes _rn, so the post-query drop only needs to apply to Exon now.

Issues

1. Doc comment typo — _gene_hgnc_id vs gene_hgnc_id
storable_to_parquet.rs:81 says _gene_hgnc_id (with leading underscore) but the actual column is gene_hgnc_id. Minor but misleading.

Fix this →

2. Option<&SchemaRef> API immediately panics for Transcript — consider tightening
build_dedup_query accepts Option<&SchemaRef> but calls .expect() on line 100 when kind == Transcript. The Option wrapper communicates "optional" but the function treats it as mandatory for one variant. A cleaner design would either always pass a &SchemaRef or return a Result. Since this is an internal example binary the current approach is acceptable, but it's a hidden contract worth documenting explicitly (or asserting only in debug).

3. Integration test duplicates the production expression
real_vep_cache_tests.rs:1131–1156 reimplements the column-building loop and the COALESCE expression verbatim instead of calling into build_dedup_query. If someone updates the expression in production but forgets to update the test copy, the integration test will keep passing despite testing different logic.

Suggested fix: extract the HGNC propagation SQL expression as a const or helper (fn hgnc_propagation_expr() -> &'static str) inside the example file, and import/use that constant in both build_dedup_query and the integration test.

Fix this →

Minor Observations

• The chrom value is directly interpolated into the SQL string (format!(" WHERE chrom = '{chrom}'")). This is a pre-existing issue and out of scope here, but worth tracking since this is an example binary that reads user CLI input.
• Test coverage in hgnc_propagation_tests.rs is comprehensive — all key scenarios (basic fill, no-overwrite, NULL symbol, all-NULL HGNC, multiple symbols, mixed) are covered with clear assertions. Well done.
• The schema.expect(...) message on line 100 is descriptive enough to diagnose a bug quickly.

Summary

The fix is correct and the approach is sound. Issue #3 (test/prod expression divergence) is the most actionable — the rest are minor polish items. The performance impact assessment in the PR description is reasonable given the data sizes involved.