Skip to content
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-47980: Add an option to include dimension records into general query result #1135

Merged
merged 2 commits into from
Dec 19, 2024

Conversation

andy-slac
Copy link
Contributor

The GeneralQueryResults.iter_tuples method returned DataIds without dimension records. In some cases (e.g. for obscore export) it would be very useful to include records in the same result to avoid querying them separately. New method with_dimension_records is added to the class to trigger adding fields from all dimension records into returned page. This will produce many duplicates for some dimensions (e.g. instrument) but it keeps page structure simple.

This adds one attribute to the GeneralResultSpec class, will need some care with Butler server compatibility.

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes
  • (if changing dimensions.yaml) make a copy of dimensions.yaml in configs/old_dimensions

Copy link

codecov bot commented Dec 14, 2024

Codecov Report

Attention: Patch coverage is 88.09524% with 15 lines in your changes missing coverage. Please review.

Project coverage is 89.50%. Comparing base (d1c5477) to head (df7a350).
Report is 3 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
.../lsst/daf/butler/queries/_general_query_results.py 83.05% 8 Missing and 2 partials ⚠️
...tler/direct_query_driver/_result_page_converter.py 88.46% 1 Missing and 2 partials ⚠️
...hon/lsst/daf/butler/remote_butler/_query_driver.py 83.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1135      +/-   ##
==========================================
- Coverage   89.51%   89.50%   -0.01%     
==========================================
  Files         366      366              
  Lines       48894    48998     +104     
  Branches     5916     5943      +27     
==========================================
+ Hits        43766    43855      +89     
- Misses       3719     3729      +10     
- Partials     1409     1414       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andy-slac andy-slac force-pushed the tickets/DM-47980 branch 3 times, most recently from 825f7fe to 4bbe699 Compare December 16, 2024 20:41
Copy link
Member

@TallJimbo TallJimbo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you've made this more difficult and less memory-efficient than it needed to be by making the cached/skypix columns appear like regular result columns (a couple of line comments on that subject), but it's possible I just didn't see the motivation. Looks good aside from that question.

@andy-slac andy-slac force-pushed the tickets/DM-47980 branch 2 times, most recently from 82da72b to 8be90c3 Compare December 17, 2024 22:06
…(DM-47980)

The `GeneralQueryResults.iter_tuples` method returned DataIds without
dimension records. In some cases (e.g. for obscore export) it would be
very useful to include records in the same result to avoid querying
them separately. New method `with_dimension_records` is added to the class
to trigger adding fields from all dimension records into returned page.
This will produce many duplicates for some dimensions (e.g. `instrument`)
but it keeps page structure simple.

This adds one attribute to the `GeneralResultSpec` class, will need some
care with Butler server compatibility.
GeneralResultPage now stores cached dimension records in a separate structure
which avoids massive duplication of the records.
@andy-slac andy-slac merged commit 797dd73 into main Dec 19, 2024
17 of 19 checks passed
@andy-slac andy-slac deleted the tickets/DM-47980 branch December 19, 2024 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants