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.
There was a problem hiding this comment.
I corrected the README waveform section and added a staging-data contract for waveform_files and waveform_channels, including identifier/uniqueness expectations, required and optional fields, timestamp interpretation (MIMIC date-shifted local clinical time, not UTC), and availability of channel_index.
|
|
||
| #### 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.
There was a problem hiding this comment.
The README now points users to the waveform workflow config(s) rather than listing individual SQL script paths.
| - 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.
There was a problem hiding this comment.
The README now documents the expected waveform vocabulary inputs, lists the hardcoded waveform concept dependencies used by the ETL, and notes that those dependencies are validated by waveform QA.
| {"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.
There was a problem hiding this comment.
The waveform extension tables are now defined only in the central DDL workflow, and the waveform ETL scripts populate those existing tables via full-refresh loads.
| [ | ||
| {"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.
There was a problem hiding this comment.
Added a dedicated workflow_waveforms_qa.conf for blocking waveform validation, cleaned up the workflow formatting/indentation, and updated waveform workflows to run as complete BigQuery scripts (type: "sql_script") so the new DECLARE / ASSERT / temp-table QA logic executes correctly.
| 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.
There was a problem hiding this comment.
Removed the parsed synonym-prefix heuristic from automatic production channel mapping.
| 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
There was a problem hiding this comment.
Unit mapping is now restricted to valid standard Unit-domain concepts, preserves the original source value in unit_source_value, and keeps unit-code matching case-sensitive to avoid false collisions such as mV versus MV.
| (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.
There was a problem hiding this comment.
Ambiguous unit matches now fail the build instead of being resolved by concept_id ordering.
| ) | ||
| ; | ||
|
|
||
| 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
There was a problem hiding this comment.
The unpivot retains person/visit only for QA. Added a preflight that validates meta.person_id and meta.visit_occurrence_id equal the occurrence values reached via registry. Any mismatch fails the build.
…d grain; use NULL for preceding ids
…struction; update num_of_files post-registry
…registry; fail on mismatches
…or metadata identity
…empty metadata rows
…rve unit_source_value; fail on ambiguity
|
Thank you for the detailed review. I addressed the waveform comments across a series of focused commits and will reply inline where relevant. Two higher-level notes that are not tied to a single line:
Here is a summary of updates related to the major and minor points raised: Major
Updated in commits 0c0ddb4 and b7dcba8. I replaced
Updated in commit 0c0ddb4. I changed
Updated in commits e1cf63a and b7dcba8.
Updated in commits e1cf63a and b7dcba8. I removed the prior numerics-specific duplicate-row behavior and now create one registry row per canonical target file. I also added blocking QA for duplicate
Updated in commit e1cf63a.
Updated in commits 609e657 and b7dcba8.
Updated in commits 609e657 and b7dcba8.
Updated in commits 33ca3a2 and b7dcba8. I added
Updated in commits c92579d, b7dcba8, and 30748b4. I removed the silent production fallback to concept ID 0 for required channel_concept_id / metadata_concept_id assignment. In the intended production logic, unresolved channel or metadata mappings should fail the load. The remaining vocabulary gaps are tracked separately in CVB #26, #31 and #32.
Updated in commit 5f58004. Channel mapping no longer selects the lowest
Updated in commits c3ec93c and 70d90e2. Unit mapping is now restricted to valid standard Unit-domain concepts (
Updated in commits b7dcba8, fca5992, and 99502e5. Converted the waveform QA draft into a blocking ASSERT-based BigQuery script covering PK uniqueness, orphan foreign keys, timestamp validity, registry/occurrence consistency, duplicate target file URIs, file-count consistency, invalid unit concepts, and duplicate channel metadata at the intended natural grain. I also added a dedicated waveform QA workflow config and a separate waveform_feature QA script for the requested feature PK/FK checks. Checks tied to currently unresolved required channel/metadata concept dependencies remain temporarily disabled pending CVB #31 and CVB #26. Minor
Updated in commit 81d03a2. I corrected the README waveform paths and expanded the waveform staging/source-schema documentation, including expected identifiers, required/optional fields, timestamp interpretation (MIMIC date-shifted local clinical time, not UTC), vocabulary dependencies, hardcoded concept dependencies, and availability of
Updated in commits 81d03a2 and c9f7871. I centralized waveform table definitions in
Updated in commits e1cf63a, 5f58004, and c3ec93c. I tightened the registry/channel/unit vocabulary joins so extension relationships exclude invalid mappings, channel candidates require valid standard concepts, and unit candidates require valid standard Unit-domain concepts.
Updated in commit c92579d. I added branch-specific filters so channel metadata rows are only created when the relevant source metadata is present (
Updated in commits c9f7871, fca5992, and f5a1a25. I cleaned up workflow formatting/indentation, added the dedicated waveform QA workflow, updated waveform workflow execution to support |
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.