-
Notifications
You must be signed in to change notification settings - Fork 14
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
Dev release bug fix: compatible granularities for date_part #214
Dev release bug fix: compatible granularities for date_part #214
Conversation
@@ -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.""" | |||
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()] |
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 there a test case around this check anyplace? Might be nice to illustrate what it is and why it's there.
@@ -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.""" |
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.
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.
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.
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.
Oops, please fix the version number! Requesting changes to avoid unintentional reverts of @QMalcolm's changes. Sorry!
pyproject.toml
Outdated
@@ -1,6 +1,6 @@ | |||
[project] | |||
name = "dbt-semantic-interfaces" | |||
version = "0.4.1" | |||
version = "0.4.2dev0" |
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.
Oh wait, this is wrong!
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.
🤔 This is the version that @QMalcolm tole me to use!
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.
@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.
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.
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 🤔
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.
I'll be out the rest of the week, so please move forward without me 🙂
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.
Sorry I didn't read the target branch name, this is totally fine (although maybe .dev for the version number is better).
Description
Checklist