New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-33942: Schema changes #674
Conversation
Codecov Report
@@ Coverage Diff @@
## main #674 +/- ##
==========================================
- Coverage 84.46% 84.41% -0.05%
==========================================
Files 243 243
Lines 31475 31516 +41
Branches 5882 5894 +12
==========================================
+ Hits 26584 26605 +21
- Misses 3726 3741 +15
- Partials 1165 1170 +5
Continue to review full report at Codecov.
|
5678d73
to
4de102a
Compare
d5a2405
to
f30a8d7
Compare
6207fdb
to
4bfe5fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but I'm concerned the export/import fixes are necessary-but-not-sufficient.
1b6ca21
to
5701023
Compare
If a dataset type is a calibration that uses seq_num it could be satisfied by visit or exposure. In that case always pick exposure because visit is a derived dimension and it may not yet have been defined.
A visit can now belong to multiple visit_systems, as long as the set of exposures computed by those visit_systems is the same.
It's quite inconveniet to specify a bit mask in a Registry SQL query so remove it and require that the visit_system_membership record be used instead.
* Remove visit system from dataset type dimensions. * Create visit_system_membership records.
A visit that is defined using day_obs/seq_num has an ambiguity in that it is possible for the one-to-one visit and the seq_start/seq_end visit to have the same values for day_obs/seq_num. Now explicitly catch an ambiguous visit and use the default visit system from the instrument record to determine which one is likely to be the desired one.
This has to be done for any substantive changes since it is used in caching.
Having null values in these columns already breaks the query system, so this should only break unit tests.
This does not catch a later change of primaryKey.
And use that version on import.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, all looks good with the export/import dimensions changes, and I do think keeping the bit of hard-coded translation logic you have (dispatched off the right version, as it now is) is the pragmatic thing to do now, even if it doesn't scale.
Checklist
doc/changes