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-43845: Implement default data ID for RemoteButler #1003

Merged
merged 4 commits into from Apr 26, 2024
Merged

Conversation

dhirving
Copy link
Contributor

@dhirving dhirving commented Apr 25, 2024

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 Apr 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.05%. Comparing base (df7acb9) to head (df8ee59).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1003   +/-   ##
=======================================
  Coverage   89.04%   89.05%           
=======================================
  Files         347      347           
  Lines       44473    44507   +34     
  Branches     9146     9149    +3     
=======================================
+ Hits        39601    39635   +34     
  Misses       3550     3550           
  Partials     1322     1322           

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

Convert the exceptions used by expandDataId to ButlerUserError, so they can be propagated to the client side.
Like other non-required metadata fields in a dimension, region is allowed to be NULL in the database.  That means that we also need to allow region to be NULL in serialized forms to be able to roundtrip dimension records through JSON.
Default data ID values are now passed to the server from RemoteButler, to augment data ID lookups in get() and find_dataset().
@dhirving dhirving marked this pull request as ready for review April 26, 2024 16:51
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

👍

class ExpandDataIdResponseModel(pydantic.BaseModel):
"""Response model for expand_data_id."""

data_coordinate: SerializedDataCoordinate
Copy link
Member

Choose a reason for hiding this comment

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

It's odd, but we never really use data_coordinate in this context. It would normally be called a data_id (and the docs call it expanded). I'm not asking you to change it, I'm commenting that we use data_id and "data coordinate" interchangeably but never use coordinate in variable names...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a clearer distinction for the serialized ones than the unserialized ones... 'SerializedDataCoordinate' means "with dimension records" and 'SerializedDataId' means "no dimension records". It's a lot fuzzier in the business objects because a DataCoordinate could be either of those, and a DataId can be a DataCoordinate or a dict or something else.

@dhirving dhirving merged commit f490737 into main Apr 26, 2024
18 checks passed
@dhirving dhirving deleted the tickets/DM-43845 branch April 26, 2024 17: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.

None yet

2 participants