DM-45933: new query system changes prompted by integration with QG generation #1071
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1071 +/- ##
==========================================
+ Coverage 89.64% 89.65% +0.01%
==========================================
Files 359 359
Lines 46727 46885 +158
Branches 9609 9637 +28
==========================================
+ Hits 41888 42036 +148
- Misses 3478 3482 +4
- Partials 1361 1367 +6 ☔ View full report in Codecov by Sentry. |
dhirving
approved these changes
Sep 6, 2024
| from ..queries.tree import DimensionFieldReference | ||
|
|
||
| return DimensionFieldReference( | ||
| element=cast(DimensionElement, endpoint), |
There was a problem hiding this comment.
It might be safer to narrow the type with an assert instead of casting.
| record.definition, db_rows.overlap_insert_rows, overlap_delete_rows=[] | ||
| record.definition, | ||
| db_rows.overlap_insert_rows, | ||
| overlap_delete_rows=[], |
There was a problem hiding this comment.
A few of these files seem to have a bunch of places where unchanged code was randomly reformatted... not sure if there is a mismatched black version in play or what.
There was a problem hiding this comment.
Thanks. I did have a misconfigured editor early on in my work on this branch and I had thought I fixed all of the fallout, but it seems I did miss some.
TallJimbo
force-pushed
the
tickets/DM-45933
branch
3 times, most recently
from
5adaf49
to
befce85
Compare
This is a complex query pattern that's very important for QG generation, so it's very useful to have a targeted test in daf_butler.
Dimensions are now expanded to include those referenced by dataset types and correctly expanded to include those referenced by dimension fields (the keys of 'dimension_fields' can be elements like 'visit_detector_region' that aren't dimensions). 'dimensions' can now be an iterable of str, as in other query methods. 'find_first' no longer defaults to False, since it defaults to True in other query methods. But we can't actually support find_first=True in some general-query (though we might be able to in the future). So now it's required if there are any dataset fields in the results, even if sometimes you have no choice but to pass `False`; it makes what you're getting explicit rather than possible hard to infer. The special '...' value for dataset_fields now means 'all fields needed to make a DatasetRef', not 'all possible dataset fields'.
It turns out we really only ever want to pass the dimensions of a query here, even when the TopologicalFamily doesn't represent dimensions (a problem that is so far hypothetical, but won't be an later commits to this branch).
This will save us from some isinstance/match checks down the road.
This requires reordering some of the query planning logic so we can get a list of calibration dataset types to the overlap-resolution logic.
We want to raise when somebody says "visit = 42" as a query constraint
without also specifying the instrument that makes that visit ID
meaningful. But a multi-instrument "visit.region OVERLAPS {region}"
query (for example) should be perfectly fine.
TallJimbo
force-pushed
the
tickets/DM-45933
branch
from
cdf387f
to
8b11755
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/changesconfigs/old_dimensions