Relax value-presence constraint per model (Observation/Measurement/Metadata)#33
Open
nicoloesch wants to merge 1 commit into
Open
Relax value-presence constraint per model (Observation/Measurement/Metadata)#33nicoloesch wants to merge 1 commit into
nicoloesch wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ValueMixinno longer unconditionally requires a value columnObservationnow acceptsvalue_as_stringas a valid standalone value alongsidevalue_as_number/value_as_concept_idMeasurementandMetadatano longer require any value column, matching OMOP CDM v5.4.Breaking Change
The old shared constraint had a latent bug (single
CheckConstraintobject reused across three declarative classes) that meant only one table ever actually got it at the DB level. Verify which, if any, apply to your database rather than assume.IMPORTANT: I cannot test any of this as I don't have the clinical tables locally present. Run this commands with care and verify if they are correct. See additional context below for information.
PostgreSQL
SQlite changes (probably not super relevant)
Additional context
No migration framework exists in this repo. Verified directly that the old shared
ValueMixin.__table_args__constraint has a latent bug:Measurement,Observation, andMetadataall referenced the sameCheckConstraintPython object rather than three independent ones. SQLAlchemy can only bind a constraint object to one table, so it silently re-parents to whichever of the three is constructed last during import.In this repo's canonical import order (
omop_alchemy/cdm/model/__init__.py:clinical, which loadsmeasurementthenobservation, before several other packages, thenmetadatalast), onlymetadataever physically got the DB-level constraint, under the nameck_measurement_ck_value_present(frozen from its first binding, tomeasurement).measurementandobservationnever had a DB-level constraint at all; only the Python-side@validateshook (a decorated method, not a shared object, so it genuinely was active on all three) enforced anything for them. This is import-order-dependent, not guaranteed identical across every deployment/entry point. This requires verification against the actual target database before dropping anything (see above).