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

Dev release bug fix: compatible granularities for date_part #214

Merged
merged 3 commits into from
Nov 21, 2023

Conversation

courtneyholcomb
Copy link
Contributor

Description

Checklist

  • I have read the contributing guide and understand what's expected of me
  • I have signed the CLA
  • This PR includes tests, or tests are not required/relevant for this PR

@cla-bot cla-bot bot added the cla:yes label Nov 21, 2023
@courtneyholcomb courtneyholcomb changed the title Bug fix: compatible granularities for date_part (#212) Dev release bug fix: compatible granularities for date_part Nov 21, 2023
@courtneyholcomb courtneyholcomb marked this pull request as ready for review November 21, 2023 00:36
@@ -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()]
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.

@@ -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.

tlento
tlento previously requested changes Nov 21, 2023
Copy link
Collaborator

@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.

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"
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 🙂

@tlento tlento dismissed their stale review November 21, 2023 03:19

Apparently I can't read

@tlento tlento self-requested a review November 21, 2023 03:21
Copy link
Collaborator

@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.

Sorry I didn't read the target branch name, this is totally fine (although maybe .dev for the version number is better).

@courtneyholcomb courtneyholcomb merged commit 0f90d5e into 0.4.latest Nov 21, 2023
8 checks passed
@courtneyholcomb courtneyholcomb deleted the backport-date-part-fix-to-0.4.latest branch November 21, 2023 05:51
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.

3 participants