-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add custom granularities to YAML spec #10664
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10664 +/- ##
==========================================
- Coverage 88.94% 88.90% -0.05%
==========================================
Files 180 180
Lines 22778 22856 +78
==========================================
+ Hits 20260 20319 +59
- Misses 2518 2537 +19
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
This looks really great! Thank you for doing this work ❤️ My only request is that I believe we're doing some unnecessary class duplication across artifiacts/resources/v1/model.py
and contracts/graph/unparsed.py
. Tests look good and everything else looks good.
@dataclass | ||
class TimeSpine(dbtClassMixin): | ||
standard_granularity_column: str | ||
custom_granularities: List[CustomGranularity] = field(default_factory=list) |
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.
Perfect, thank you for the defaulting behavior
custom_granularities=[ | ||
PydanticTimeSpineCustomGranularityColumn( | ||
name=custom_granularity.name, column_name=custom_granularity.column_name | ||
) | ||
for custom_granularity in time_spine.custom_granularities | ||
], |
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.
The more we do this, the more I wish we had a better way to automatically translate them. Alas unless we're literally using the same object (or subclassing) we'll need the translation logic somewhere
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 good to me 🚀
Resolves #9265
Description
The semantic layer is in the process of adding a custom calendar feature that will allow users to query using custom granularities like
retail_month
. This requires an addition to the YAML spec for models that looks like this:The only new piece here is
custom_granularities
and everything nested under that key. This spec has been approved by the core product team.The corresponding change to schemas.getdbt.com is here.
Checklist