DM-34247: simplify dataset subquery logic and fix edge-case bugs #670
Conversation
This file contains 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
Codecov Report
@@ Coverage Diff @@
## main #670 +/- ##
==========================================
+ Coverage 84.29% 84.32% +0.02%
==========================================
Files 242 242
Lines 31019 31055 +36
Branches 5219 5219
==========================================
+ Hits 26149 26187 +38
Misses 3706 3706
+ Partials 1164 1162 -2
Continue to review full report at Codecov.
|
TallJimbo
force-pushed
the
tickets/DM-34247
branch
from
198df4b
to
68e4855
Compare
TallJimbo
force-pushed
the
tickets/DM-34247
branch
2 times, most recently
from
cdf20cd
to
94de327
Compare
andy-slac
approved these changes
Apr 6, 2022
Comment on lines
220
to
243
| else: | ||
| # We can never find a non-calibration dataset in a | ||
| # CALIBRATION collection. |
There was a problem hiding this comment.
Re-reading pre-existing code - I think this comment is not quite right, in this else branch dataset type can still be calibration (if collectionRecord.name is not in explicitCollections).
There was a problem hiding this comment.
Yes, I believe this was an edge-case bug that is now fixed, and the comment is now correct.
There was a problem hiding this comment.
Oh, nevermind; this wasn't fixed - I had started, and then decided to the refactor first, and then forgot to come back to it. Fixed on another commit.
| ingestDate=SimpleQuery.Select if isResult else None, | ||
| # If this dataset type has no dimensions, we're in danger of | ||
| # generating an invalid subquery that has no columns in the | ||
| # SELECT clause. Any easy fix is to just select some arbitrary |
TallJimbo
force-pushed
the
tickets/DM-34247
branch
2 times, most recently
from
80709d5
to
35424d9
Compare
andy-slac
approved these changes
Apr 7, 2022
| # find-first search is just a regular result subquery. | ||
| if len(collections) == 1: | ||
| # find-first search is just a regular result subquery. Same is true | ||
| # if this is a doomed query with no collectoins to search. |
TallJimbo
force-pushed
the
tickets/DM-34247
branch
2 times, most recently
from
c36db32
to
db1303e
Compare
This used to be one massive method; now it's two medium-large methods and two very simple ones, and it's much easier to follow. This shouldn't change the query behavior at all, but it does collapse some UNION ALL constructs by merging their IN clauses when they are otherwise the same - the original code had an unnecessary distinction between CALIBRATION and other collection types (as well as some necessary distinctions, which this preserves).
...which I just merged without a changelog entry.
This prevents query construction from failing when someone queries for a skypix-dimension dataset type in collections where none exist. When the datasets do exist, we rely on the dataset table to bring in the skypix dimension, since those don't have tables of their own, and we need to do the same when the query is doomed to avoid triggering an exception about skypix dimensions not being directly joinable.
TallJimbo
force-pushed
the
tickets/DM-34247
branch
from
db1303e
to
10b2f76
Compare
andy-slac
approved these changes
Apr 10, 2022
| @@ -511,6 +514,7 @@ def transaction( | |||
| # `Connection.in_nested_transaction()` method. | |||
| savepoint = savepoint or connection.info.get(_IN_SAVEPOINT_TRANSACTION, False) | |||
| connection.info[_IN_SAVEPOINT_TRANSACTION] = savepoint | |||
| trans: Union[sqlalchemy.engine.Transaction, sqlalchemy.engine.NestedTransaction] | |||
There was a problem hiding this comment.
NestedTransaction should be a subclass of Transaction, do you need explicit NestedTransaction here?
There was a problem hiding this comment.
MyPy didn't seem to think they had a direct subclass relationship, but the docs claim they do, so given the uncertainty about whether the stubs package I used is official, I'll just replace this with Transaction until we're ready to start depending on sqlalchemy stubs.
| else: | ||
| rejections.append( | ||
| f"Not searching for dataset {datasetType.name!r} in CALIBRATION collection " | ||
| f"{collectionRecord.name!r} because temporal calibration queries are't " |
There was a problem hiding this comment.
Typo: are't, same below.
This fixes an edge-case bug involving mixed CALIBRATION/non-CALIBRATION searches introduced in the previous refactoring commit. This involves a small internal API change for DatasetRecordStorage - it now returns a SQLAlchemy object directly instead of one of our SimpleQuery instances, since it sometimes now returns a UNION.
These were revealed by installing the sqlalchemy-stubs package locally. That's something we should consider doing more broadly (e.g. in CI), as it revealed a lot more than just these, but it seems tangled up with migrating more fully to SQLAlchemy 2.0 APIs (there is a different stubs package for SQLAlchemy 2), and since that's not even fully released yet doing much more seems premature.
TallJimbo
force-pushed
the
tickets/DM-34247
branch
from
10b2f76
to
d168fa6
Compare
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.
Checklist
doc/changes