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

Handle ambiguous metric_time filters in YAML definitions #1342

Merged
merged 11 commits into from
Jul 22, 2024

Conversation

courtneyholcomb
Copy link
Contributor

@courtneyholcomb courtneyholcomb commented Jul 17, 2024

Handle ambiguous metric_time filters in metric YAML definitions. The expected behavior for an ambiguous metric_time filter in a metric's YAML definition is that it will use the default time granularity for the metric where the filter is defined. This means that if a filter is defined on a derived metric's input metric, that filter will use the derived metrics time granularity, not the input metric's time granularity.
Aside from that change, this adds tests for other types of ambiguous metric_time YAML filters.

@dbt-labs dbt-labs deleted a comment from github-actions bot Jul 17, 2024
@cla-bot cla-bot bot added the cla:yes label Jul 17, 2024
@courtneyholcomb courtneyholcomb changed the title Write tests for ambiguous metric_time filters in YAML definitions Handle ambiguous metric_time filters in YAML definitions Jul 18, 2024
@courtneyholcomb courtneyholcomb requested review from plypaul and removed request for plypaul July 18, 2024 21:04
@courtneyholcomb courtneyholcomb marked this pull request as ready for review July 18, 2024 23:29
filter: "{{ TimeDimension('metric_time') }} < '2012-12-20'"
---
metric:
name: bookings_dec_19_2019
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be something like bookings_between...? Also feel like there can be a more "friendly" metric that we could create for these tests, but I don't have any good ideas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's a weird use case but I figured it's really the SQL that matters here, not the usefulness of the metric. Will update the name!

Base automatically changed from court/default-granularity4 to main July 22, 2024 18:59
@courtneyholcomb courtneyholcomb enabled auto-merge (squash) July 22, 2024 19:04
@courtneyholcomb courtneyholcomb merged commit 80d4612 into main Jul 22, 2024
15 checks passed
@courtneyholcomb courtneyholcomb deleted the court/test-dg branch July 22, 2024 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants