-
Notifications
You must be signed in to change notification settings - Fork 14
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-43157: new query interfaces for dataset types and datasets of multiple types #1062
base: main
Are you sure you want to change the base?
Conversation
6d1163b
to
174b8fa
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1062 +/- ##
==========================================
+ Coverage 89.37% 89.38% +0.01%
==========================================
Files 362 365 +3
Lines 48288 48339 +51
Branches 5872 5872
==========================================
+ Hits 43155 43206 +51
Misses 3718 3718
Partials 1415 1415 ☔ View full report in Codecov by Sentry. |
5396bb0
to
783f3ee
Compare
"""Methods for working with the dataset types known to the Butler.""" | ||
|
||
@abstractmethod | ||
def get(self, name: str) -> DatasetType: |
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.
Are we going to immediately deprecate butler.get_dataset_type
and move to butler.dataset_type.get()
?
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.
I think that'd be best, yes, if we all like butler.dataset_types
more generally.
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 OK to me.
# - 'where' exists on other result objects because the way they are | ||
# constructed adds context (a dataset search join, some dimensions) that | ||
# can help interpret arguments to 'where'. That's not generally true | ||
# here, so calling `Query.where(...).all_datasets()` can do anything that | ||
# `Query.all_datasets().where(...)` might be able to do. |
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.
We might want to have where
just for consistency.
# - 'order_by' and 'limit' are hard to implement in the common case where | ||
# we have to run one query for each dimension group. |
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.
I think limit
is probably desirable since these queries can easily return huge numbers of results. Should be relatively straightforward to implement if we're executing the queries serially -- you just start with the original limit and subtract from it for each subsequent query.
783f3ee
to
303ba19
Compare
Rebased to current main. |
f0a0cbf
to
7e5ed85
Compare
7e5ed85
to
8bac470
Compare
Checklist
doc/changes
configs/old_dimensions