Initial beta build for OMOP Waveform Extension ETL#65
Conversation
|
@briangow , thanks for the work! will plan to review soon. Any firm deadlines I should be aware of? Should be well before next WG meeting, but just grounding in known milestones |
(cherry picked from commit 448f8b386796d8e83b046aaf506c9eb608211563)
Smoke test: runs end-to-end on BigQuery ✅Stood up a synthetic smoke test of this branch on BigQuery to confirm it executes and emits sane rows. Setup:
All three ETL scripts ran clean in ~20s. No changes to the PR's SQL. Output (synthetic data):
PK uniqueness: 0 duplicates on all three tables. Follow-up Questions/ConcernsA couple of things worth noting from the run:
One thing to confirm before scale-up:
Reproducing this from start (for anyone not steeped in the repo layout)This PR's head branch is git fetch origin
# 1. Start from dev (the PR's intended base), not main
git reset --hard origin/dev
# 2. Layer PR #65 on top — it's purely additive, merges clean
git merge --no-edit origin/bg_waveform_extension_1
# 3. Pull ONLY the two synthetic CSVs off Brian's data branch
# (checkout <branch> -- <paths> grabs just those files, not the whole branch)
git checkout origin/bg_syn_waveform_metadata -- \
data/waveform_files_all.csv \
data/waveform_channels_all.csvHappy to hand over the full runbook + driver script (one |
|
@odikia thank you for testing this and for your review!
Good catch, I've changed
Good point, this ID is at high risk for collisions since there are numerous entries in the Waveform Channel Metadata table for every recording. I've updated this to use 64 bit space. |
p-talapova
left a comment
p-talapova
left a comment
There was a problem hiding this comment.
@briangow, thank you so much for implementing the initial Waveform Extension ETL workflow. The overall population sequence is correct. The table structures are also broadly aligned with the current Waveform Extension specification, and it is reasonable for WAVEFORM_FEATURE population to remain outside the scope of this beta PR.
However, I recommend requesting changes before merge.
Major issues
- WAVEFORM_OCCURRENCE uses ANY_VALUE() for person, visit, and timestamp fields, which can hide inconsistent source data and produce nondeterministic results.
- preceding_waveform_occurrence_id uses 0 instead of NULL.
- num_of_files is calculated from staging rows rather than from the generated registry records.
- The numerics logic appears to create two WAVEFORM_REGISTRY records for one transformed CSV target.
- Some source and target URIs are reconstructed rather than taken from the authoritative staging values.
- Registry person and visit fields are independently copied from staging instead of inherited from WAVEFORM_OCCURRENCE.
- WAVEFORM_CHANNEL_METADATA independently regenerates the registry foreign key instead of joining to the populated registry.
- Channel metadata identifiers do not include a channel index and may collide for duplicate channel labels.
- Required channel and metadata concepts silently default to concept ID 0.
- Channel mapping relies on broad name and synonym matching and resolves ambiguity by selecting the lowest concept ID.
- Unit mappings are not restricted to valid standard Unit-domain concepts.
- No blocking QA is included for duplicate IDs, orphan foreign keys, inconsistent timestamps, or unresolved mandatory mappings (added test/qa/qa_wf_extension.sql as a draft)
Minor issues
- The README contains incorrect file paths, and an incomplete staging-data contract.
- DDL definitions are duplicated in both the central DDL file and individual ETL scripts.
- Several vocabulary joins do not filter non-standard concepts or invalid relationships.
- Some metadata records are created even when no metadata value or unit is available.
- Workflow formatting is a bit inconsistent
| * set the project root (location of this file) as the current directory | ||
|
|
||
| #### Waveforms | ||
| - Generate your source data in the format of the `data/waveform_files_all.csv` and `data/waveform_channels_all.csv` or use those files them when performing a dummy build. Upload to BigQuery as waveform_files_all and waveform_channels_all tables, respectively. |
There was a problem hiding this comment.
• 'use those files them' is a typo
• The expected grain and uniqueness rules for both staging tables are not documented.
• It is not stated whether timestamps are UTC or local.
• It is not stated whether group_id is globally unique.
• It is not stated whether trg_file uniquely identifies one registry file.
• It is not stated whether one staging row may generate more than one registry record.
If possible, please document the staging contract, including at least:
• natural key and row grain of waveform_files_all;
• natural key and row grain of waveform_channels_all;
• uniqueness expectations for group_id, src_file, and trg_file;
• timestamp timezone and precision;
• nullable and required fields;
• meaning of session_start, session_end, file_start, and file_end;
• whether channel order or channel index is available.
|
|
||
| #### Waveforms | ||
| - Generate your source data in the format of the `data/waveform_files_all.csv` and `data/waveform_channels_all.csv` or use those files them when performing a dummy build. Upload to BigQuery as waveform_files_all and waveform_channels_all tables, respectively. | ||
| - Run `etl/cdm_waveform_occurrence.sql`, `cdm_waveform_registry.sql`, `cdm_waveform_channel_metadata.sql` with `python scripts/run_workflow.py -e conf/<env>.etlconf -c conf/workflow_waveforms.conf` |
There was a problem hiding this comment.
Actual paths:
etl/etl/cdm_waveform_occurrence.sql
etl/etl/cdm_waveform_registry.sql
etl/etl/cdm_waveform_channel_metadata.sql
Because the scripts are executed through workflow_waveforms.conf, listing each SQL path here may be unnecessary. The README could simply instruct users to execute the workflow configuration.
| - Generate your source data in the format of the `data/waveform_files_all.csv` and `data/waveform_channels_all.csv` or use those files them when performing a dummy build. Upload to BigQuery as waveform_files_all and waveform_channels_all tables, respectively. | ||
| - Run `etl/cdm_waveform_occurrence.sql`, `cdm_waveform_registry.sql`, `cdm_waveform_channel_metadata.sql` with `python scripts/run_workflow.py -e conf/<env>.etlconf -c conf/workflow_waveforms.conf` | ||
|
|
||
| NOTE: the standard process for combining Athena and custom vocab (_delta talbes) going forward |
There was a problem hiding this comment.
• _delta talbes → _delta tables
• BQ VIEW → preferably BigQuery view
the ETL relies on custom Waveform vocabulary concepts, but the README does not identify:
• the required CVB vocabulary package (link can be added)
• the expected vocabulary version from Athena (Feb 2026)
• the required custom concept IDs (those which are hardcoded)
• a preflight check confirming that those concepts exist and are valid.
Please add a reproducible vocabulary dependency statement and validation query for all hard-coded custom concepts used by this PR.
| {"script": "etl/ddl/ddl_voc_5_4_2.sql", "comment": ""}, | ||
| {"script": "etl/ddl/ddl_cdm_5_4_2.sql", "comment": ""} | ||
| {"script": "etl/ddl/ddl_voc_5_4_2.sql", "comment": ""}, | ||
| {"script": "etl/ddl/ddl_cdm_5_4_2.sql", "comment": ""}, |
There was a problem hiding this comment.
Adding ddl_cdm_waveform_extension.sql to the DDL workflow is appropriate. However, because the individual ETL scripts recreate the same tables, the schema currently has two separate definitions.
| [ | ||
| {"script": "etl/etl/cdm_waveform_occurrence.sql", "comment": ""}, | ||
| {"script": "etl/etl/cdm_waveform_registry.sql", "comment": ""}, | ||
| {"script": "etl/etl/cdm_waveform_channel_metadata.sql", "comment": ""} |
There was a problem hiding this comment.
Please add a waveform-specific validation script or workflow step.
At minimum, it should test:
• primary-key uniqueness
• orphan registry records
• orphan channel metadata records
• person and visit consistency
• timestamp validity
• registry timestamps outside occurrence timestamps
• unresolved mandatory concepts
• duplicate target file URIs
• file-count consistency
• duplicate channel metadata at the intended natural grain.
Also, there is minor formatting issue - indent the script entries consistently with the rest of the JSON configuration.
| FROM @etl_project.@etl_dataset.voc_concept | ||
| WHERE vocabulary_id IN ('WAVEFORM', 'MIMIC4') | ||
| AND domain_id IN ('Waveform Metadata', 'Measurement', 'Observation') | ||
| QUALIFY ROW_NUMBER() OVER (PARTITION BY concept_code ORDER BY concept_id) = 1 |
There was a problem hiding this comment.
The numerically lowest concept ID has no semantic priority. This can silently select an incorrect target when multiple candidate concepts share a code, name, or synonym. Please:
• require a unique valid mapping
• report ambiguous matches
• resolve them through an explicit curated mapping rule
There was a problem hiding this comment.
@p-talapova , I'm a bit stuck regarding how to resolve this issue. I agree this is not a good production rule. Selecting the numerically lowest concept_id is arbitrary and I don’t want to hard-code that behavior here. This seems to overlap with a broader upstream vocabulary-governance issue that is already being discussed in TuftsCTSI/CVB#27. My main question is what the intended production policy should be when multiple eligible concepts are present across the custom waveform/MIMIC vocabularies and Athena. For example, should the ETL prefer exact custom concept_code matches only, fail on multiple candidates, or follow some other reviewed precedence rule? Once that policy is clarified, I can align the ETL logic to it.
| -- Extracts abbreviation from synonyms like "ABP - Arterial blood pressure" by matching before the " - " | ||
| LEFT JOIN | ||
| (SELECT DISTINCT | ||
| UPPER(TRIM(SPLIT(syn.concept_synonym_name, ' - ')[OFFSET(0)])) AS parsed_value, |
There was a problem hiding this comment.
Current logic extracts the text before " - " from every synonym and treats it as a channel abbreviation.
This assumes that every synonym formatted as" BBREVIATION - description" defines a globally safe abbreviation. That is not guaranteed and may create collisions across unrelated concepts.
Please remove this heuristic from automatic production mapping or constrain it to a reviewed allow-list.
| WHERE vocabulary_id IN ('WAVEFORM', 'MIMIC4', 'UCUM', 'SNOMED') | ||
| QUALIFY ROW_NUMBER() OVER (PARTITION BY UPPER(concept_code) ORDER BY concept_id) = 1 | ||
| ) vc_unit_code | ||
| ON vc_unit_code.concept_code = UPPER(meta.unit_source_value) |
There was a problem hiding this comment.
Please restrict normal unit mapping to standard Unit-domain concepts
WHERE domain_id = 'Unit'
AND standard_concept = 'S'
There was a problem hiding this comment.
also consider preserving the unmodified source value in unit_source_value
| (SELECT DISTINCT concept_id, UPPER(concept_code) AS concept_code | ||
| FROM @etl_project.@etl_dataset.voc_concept | ||
| WHERE vocabulary_id IN ('WAVEFORM', 'MIMIC4', 'UCUM', 'SNOMED') | ||
| QUALIFY ROW_NUMBER() OVER (PARTITION BY UPPER(concept_code) ORDER BY concept_id) = 1 |
There was a problem hiding this comment.
If multiple valid unit concepts match one normalized source value, the mapping should be treated as ambiguous and escalated rather than resolved by concept ID order.
| ) | ||
| ; | ||
|
|
||
| INSERT INTO @etl_project.@etl_dataset.cdm_waveform_channel_metadata |
There was a problem hiding this comment.
The unpivoted metadata retains person_id and visit_occurrence_id, but these values are not used in the final query. After joining to the registry, validate that metadata staging person_id = registry person_id
and metadata staging visit_occurrence_id = registry visit_occurrence_id
3 participants
Overview
This PR introduces an initial (beta) build path for OMOP Waveform Extension ETL tables using pre-extracted waveform metadata inputs.
Context
The Python waveform metadata extraction process is currently maintained in a separate repository. Once that process is finalized, it may be incorporated into this repository.
Source metadata inputs
The ETL expects two source tables populated by the extraction script:
waveform_files_all
subject_id, person_id, hadm_id, visit_occurrence_id, visit_detail_id, location, waveform_folders, record_id, group_id, mimic_start, mimic_end, session_start, session_end, file_start, file_end, src_file, trg_filewaveform_channels_all
person_id, visit_occurrence_id, visit_detail_id, group_id, session_start, session_end, file_start, file_end, src_file, trg_file, channel_name, sample_units, sample_rate, sample_rate_units, gain, gain_units, segment_lengthWhat this PR includes:
waveform_files_allandwaveform_channels_allas source metadata interfaces.Scope and maturity
This PR establishes the initial/beta end-to-end waveform ETL framework and will be expanded and refined in subsequent updates.
Out of scope
This PR does not build the
WAVEFORM_FEATUREtable.