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

Implemented date_part in where filter #852

Merged
merged 2 commits into from
Nov 17, 2023
Merged

Implemented date_part in where filter #852

merged 2 commits into from
Nov 17, 2023

Conversation

DevonFulcher
Copy link
Contributor

@DevonFulcher DevonFulcher commented Nov 8, 2023

Description

This PR allows users to specify date_part in the where filter. This is useful for the Jinja syntax as well as integrations such as Tableau.

@DevonFulcher DevonFulcher force-pushed the time_dimension_filter branch from 0f19319 to 8d9c031 Compare November 8, 2023 00:54
Copy link
Contributor

@tlento tlento left a comment

Choose a reason for hiding this comment

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

This looks like two PRs in one. Is there something wonky in the underlying branch, or should this be split up?

@DevonFulcher
Copy link
Contributor Author

This looks like two PRs in one. Is there something wonky in the underlying branch, or should this be split up?

I fixed it now. I'm sorry about that.

Copy link
Contributor

@tlento tlento left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! I'll hold this open until after 0.203.0 is out (unless that drags past Monday, in which case maybe we just merge this).

self.time_granularity_name: Optional[str] = None

@property
def time_dimension_spec(self) -> TimeDimensionSpec:
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so this returns a TimeDimension with DEFAULT_TIME_GRANULARITY if neither grain nor date_part are set. Interesting. I think that makes the most sense with where we're heading with time dimension expressions.

Comment on lines +129 to +131
("{{ TimeDimension('metric_time', 'WEEK', date_part_name='year') }} = '2020'"),
("{{ Dimension('metric_time').date_part('year').grain('WEEK') }} = '2020'"),
("{{ Dimension('metric_time').grain('WEEK').date_part('year') }} = '2020'"),
Copy link
Contributor

Choose a reason for hiding this comment

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

I hope these aren't case-sensitive. I may add some additional tests on after we merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had fixed the case sensitivity here dbt-labs/dbt-semantic-interfaces#207. So, date_part was case-sensitive as of this PR, but that change to DSI fixes it.

@tlento tlento merged commit a28a552 into main Nov 17, 2023
6 checks passed
@tlento tlento deleted the time_dimension_filter branch November 17, 2023 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants