-
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
Allow metrics in semantic layer filters #9773
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9773 +/- ##
=======================================
Coverage 88.04% 88.04%
=======================================
Files 178 178
Lines 22461 22461
=======================================
Hits 19776 19776
Misses 2685 2685
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
0f29ad0
to
9891660
Compare
58966bf
to
7a99fe1
Compare
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.
Question in why the version is getting bumped.
@@ -73,7 +73,7 @@ | |||
# Accept patches but avoid automatically updating past a set minor version range. | |||
"dbt-extractor>=0.5.0,<=0.6", | |||
"minimal-snowplow-tracker>=0.0.2,<0.1", | |||
"dbt-semantic-interfaces>=0.5.0,<0.6", | |||
"dbt-semantic-interfaces>=0.5.1,<0.6", |
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.
Is this not backwards compatible with 0.5.0? Should we be pinning a range here or a specific version?
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.
It is backwards compatible! Basically, I want to ensure that this feature is available for all users on core 1.8 once that gets released. We could
leave the existing version range, but it might cause confusion for users if they somehow end up on DSI 0.5.0 and wonder why this feature isn't available!
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.
In this case it looks like a feature was added in the patch version bump to 0.5.1
from 0.5.0
, specifically a feature allowing metrics to be referenced in filter attributes of semantic layer objects. Arguably, this should have constituted a 0.6.0
, but the release of 0.5.1
has already happened. This makes 0.5.1
not fully backwards compatible with 0.5.0
in that if someone were to specify a metric in a filter, parsing would break if 0.5.0
is the installed DSI version.
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.
So we'll want to get this change in prior to the final release of core 1.8.0
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.
Arguably, this should have constituted a
0.6.0
, but the release of0.5.1
has already happened.
This makes sense. As long as in future we try to follow semver more closely I'll ✅ this. Since 1.7 is on 0.4 anyways it's low risk.
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.
Just me over her conflating backwards compatibility with being a purely additive change - sorry for the confusion!
And yes @QMalcolm point taken - we can do some work on the SL side to clean up our OSS versioning story in the future 🙏
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.
Thanks for the quick review - would one of y'all mind merging? I don't have permission.
Opened a new issue in dbt-labs/docs.getdbt.com: dbt-labs/docs.getdbt.com#5142 |
resolves #9804
resolves SL-1794
Description
Bump DSI version to get a new feature into core - allowing metrics in filters for metrics, measures, and saved queries. You can see the changes to DSI here. This change is purely additive.
Checklist