Skip to content

Join new entry variants in clickhouse - #5582

Open
hanars wants to merge 56 commits into
devfrom
join-entry-variants-at-write
Open

hanars wants to merge 56 commits into
devfrom
join-entry-variants-at-write

Conversation

@hanars

@hanars hanars commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

This pull request refactors how variant level annotations are added to the entries table - currently existing and new variants are joined with the entries in hail and exported to the entries parquet that is then loaded. With this change, the entries parquet includes no variant level data, and it is all added at INSERT time by joining the relevent tables before writing to clickhouse.

1. ClickHouse Insert and Export Logic Updates

  • Refactored insert_new_entries in clickhouse.py to standardize column selection, ensure inclusion of key and xpos, and update the logic for populating geneId_ids from the correct gene consequence field depending on dataset type.
  • Updated the export logic for existing variants to always use the KEY_LOOKUP table when appropriate and to select the correct fields for different dataset types, improving compatibility and downstream processing.

2. Test Data and Test Logic Improvements

  • Enhanced test data for both SNV/INDEL and GCNV pipelines in clickhouse_test.py, including more realistic gene IDs, updated keys, and comprehensive GCNV entry records to better reflect production scenarios.
  • Adjusted test assertions to align with the new data and logic, ensuring tests accurately verify the revised pipeline behavior.

3. GCNV Annotation Logic Simplification

  • Simplified the GCNV annotation functions in gcnv.py by removing conditional logic and always returning the relevant fields directly, which reduces complexity and potential for inconsistent annotations. Previously, values were being mapped to missing and repopulated later in the pipeline flow, now they are computed properly up front

4. Export Fields and Annotation Refactoring

  • Refactored export field logic in fields.py to remove dataset-specific field selection functions and instead directly reference the correct annotation fields, further simplifying the export code and reducing redundancy.

5. Documentation Update

  • Updated the pipeline diagram in Diagrams.md to reflect the new data flow and task dependencies, ensuring documentation matches the revised pipeline structure.

@hanars
hanars changed the base branch from no-dataproc-clickhouse-access to dev September 14, 2026 19:20

Copilot AI 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.

🟡 Changes recommended

Qualify the ambiguous key reference in the ClickHouse query before approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR moves variant enrichment from Hail exports into ClickHouse during entry loading.

Changes:

  • Simplifies new-entry parquet generation.
  • Adds ClickHouse variant and gene joins.
  • Updates GCNV handling, fixtures, tests, and documentation.

Blocking finding: loading_pipeline/lib/misc/clickhouse.py uses an ambiguous unqualified key; it must be qualified as e.key or v.key.

File summaries
File Summary
loading_pipeline/lib/test/fixtures/clickhouse_test.json Updates ClickHouse lookup and annotation fixtures.
loading_pipeline/lib/tasks/write_existing_variants_parquet.py Adapts existing-variant export invocation.
loading_pipeline/lib/tasks/write_existing_variants_parquet_test.py Updates existing-variant export expectations.
loading_pipeline/lib/tasks/exports/write_new_entries_parquet.py Removes pre-export Hail annotation joins.
loading_pipeline/lib/tasks/exports/write_new_entries_parquet_test.py Updates new-entry export expectations.
loading_pipeline/lib/tasks/exports/fields.py Updates exported variant and call-level fields.
loading_pipeline/lib/misc/clickhouse.py Implements ClickHouse entry and variant joins.
loading_pipeline/lib/misc/clickhouse_test.py Updates ClickHouse integration tests.
loading_pipeline/lib/annotations/gcnv.py Preserves raw sample-level GCNV annotations.
loading_pipeline/docs/Diagrams.md Documents revised pipeline dependencies.
Review details
  • Files reviewed: 10/10 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread loading_pipeline/lib/misc/clickhouse.py

Copilot AI 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.

🔵 Needs a closer look

The changes span exports, ClickHouse joins, annotations, tests, and documentation, warranting final human review.

Review details
  • Files reviewed: 10/10 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

{'key_': 7, 'variant_id': '7-1234567-AGT-A', 'geneIds': []},
{'key_': 10, 'variant_id': '10-987654-G-A', 'geneIds': []},
{'key_': 1, 'variant_id': '1-878314-G-C'},
{'key_': 3, 'variant_id': '3-133456789-A-G'},

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

reflects updated fixture data

'start': 100017585,
'end': 100023213,
'numExon': 1,
'numExon': 3,

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

the numExon for a couple of calls was being overridden by the annotations.ht added in test setup, which is not reflective of real world behavior and is now removed for this test. The actual behavior for this value is unchanged

@hanars
hanars marked this pull request as ready for review September 16, 2026 18:12
@hanars
hanars requested a review from larrybabb September 16, 2026 18:13
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.

2 participants