Allow metrics in filters #274
Conversation
|
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
courtneyholcomb
force-pushed
the
court/metric-filter
branch
from
ed608fb to
f8c6234
Compare
courtneyholcomb
force-pushed
the
court/metric-filter
branch
from
cec1498 to
88be4db
Compare
courtneyholcomb
requested review from
QMalcolm,
graciegoheen and
a team
as code owners
There was a problem hiding this comment.
Looks good to me! If you haven't already I think we'll want to do some local testing against the jaffle project to make sure parsing the metric filter is working as expected with the dbt integration for 1.8. It should - as far as I know dbt's parser just passes the where filter expressions into these components - but I've never actually tried it.
| def descending(self, _is_descending: bool) -> QueryInterfaceMetric: # noqa: D | ||
| raise InvalidQuerySyntax("descending is invalid in the where parameter and filter spec") |
There was a problem hiding this comment.
Ok, I see why this has to be here. We really need to carve out some time to clean up the underlying protocol specs so we don't have these runtime exceptions floating around all over the place.
There was a problem hiding this comment.
Definitely agree!
| @@ -108,6 +108,8 @@ def create_entity(entity_name: str, entity_path: Sequence[str] = ()) -> EntityCa | |||
| @staticmethod | |||
| def create_metric(metric_name: str, group_by: Sequence[str] = ()) -> MetricCallParameterSet: | |||
| """Gets called by Jinja when rendering {{ Metric(...) }}.""" | |||
| if not group_by: | |||
| raise ParseWhereFilterException("`group_by` parameter is required for Metric in where filter.") | |||
There was a problem hiding this comment.
Can we make this a little more descriptive as to why? Maybe something like:
"`group_by` parameter is required for filtering on Metrics, since a filter on a Metric does not make sense for queries where there are no sub-groups against which to apply the filter expression."
I don't know if normal people would understand that, but maybe it's better?
| ] | ||
|
|
||
| manifest_validator = SemanticManifestValidator[PydanticSemanticManifest]([SavedQueryRule()]) | ||
| manifest_validator.validate_semantic_manifest(manifest), |
There was a problem hiding this comment.
Ah, ok, so this should also run our internal check for missing group-bys and will throw a validation error if someone messes that up, correct?
There was a problem hiding this comment.
It didn't quite do that, but now I added some more detail so that it will do that.
Resolves #273 Resolves SL-1849 <!--- Include the number of the issue addressed by this PR above if applicable. PRs for code changes without an associated issue *will not be merged*. See CONTRIBUTING.md for more information. --> ### Description Allow users to reference metrics in where filters for metrics, measures, and saved queries. This uses syntax like: `{{ Metric('metric_name', group_by=['entity_name', 'dimension_name']) }} = 10` This unlocks new types of metrics that users have been asking for. Some examples can be found in the linked issue and in [this design doc](https://www.notion.so/dbtlabs/Metrics-as-Dimensions-55718e9516a7462787ffd6e3e8c1237e?pvs=4). ### Checklist - [x] I have read [the contributing guide](https://github.com/dbt-labs/dbt-semantic-interfaces/blob/main/CONTRIBUTING.md) and understand what's expected of me - [x] I have signed the [CLA](https://docs.getdbt.com/docs/contributor-license-agreements) - [x] This PR includes tests, or tests are not required/relevant for this PR - [x] I have run `changie new` to [create a changelog entry](https://github.com/dbt-labs/dbt-semantic-interfaces/blob/main/CONTRIBUTING.md#adding-a-changelog-entry)
Resolves #273
Resolves SL-1849
Description
Allow users to reference metrics in where filters for metrics, measures, and saved queries. This uses syntax like:
{{ Metric('metric_name', group_by=['entity_name', 'dimension_name']) }} = 10This unlocks new types of metrics that users have been asking for. Some examples can be found in the linked issue and in this design doc.
Checklist
changie newto create a changelog entry