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.

Sign up for GitHub

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

Dev release bug fix: compatible granularities for date_part #214

Merged
merged 3 commits into from
Nov 21, 2023
Merged

Dev release bug fix: compatible granularities for date_part #214

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1ffd635
Bug fix: compatible granularities for date_part (#212)
courtneyholcomb Nov 20, 2023
5d0ff0c
Dev version bump
courtneyholcomb Nov 21, 2023
6e01875
Add period to version number
courtneyholcomb Nov 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion dbt_semantic_interfaces/type_enums/date_part.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,4 @@ def to_int(self) -> int:
@property
def compatible_granularities(self) -> List[TimeGranularity]:
"""Granularities that can be queried with this date part."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is so confusing, can we explain it here?

We are switching this to allow TimeGranularity.DAY for DatePart.MONTH, correct? So it'd now look like:

SELECT date_part(month, metric_time__day), count(*)
FROM ...
GROUP BY date_part(month, metric_time__day)

Whereas before we only allowed something like this:

SELECT date_part(day, metric_time__month), count(*)
FROM ...
GROUP BY date_part(day, metric_time__month)

Ok, that makes sense to me now.

Maybe we can document this as:

We support querying granularities that are the same underlying granularity or coarser than the requested date_part. This is because extracting the "day" part from a monthly granularity will always produce a date_part value of `1`, which is probably not what the user wants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tlento this is just the dev release for a PR that already merged to main, you can see here.
This is not actually a behavior change for queries, this was only used in an error message.

return [granularity for granularity in TimeGranularity if granularity.to_int() >= self.to_int()]
return [granularity for granularity in TimeGranularity if granularity.to_int() <= self.to_int()]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a test case around this check anyplace? Might be nice to illustrate what it is and why it's there.

2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[project]
name = "dbt-semantic-interfaces"
version = "0.4.1"
version = "0.4.2dev0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh wait, this is wrong!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 This is the version that @QMalcolm tole me to use!

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tlento Courtney and I talked about this. The goal is to do a dev release so that it can be tested in MetricFlow without doing a production patch release. What we do is a 0.4.2.dev0, and if all goes well we then follow up with a 0.4.2 release. I don't anticipate anything bad in doing this. The ~= will automatically ignore versions ending in .dev, rc, b, and a so long as none of those specifications exist in the version specified on the right hand side of ~= specification.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh is this that it should be 0.4.2.dev0 instead of 0.4.2dev0. That is likely on me. . are expected with dev versions but not rc, b, and a strangely 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll be out the rest of the week, so please move forward without me 🙂

description = 'The shared semantic layer definitions that dbt-core and MetricFlow use'
readme = "README.md"
requires-python = ">=3.8"
Expand Down
Loading