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

Rename SavedQuery.group_bys to SavedQuery.group_by #186

Merged
merged 1 commit into from
Oct 26, 2023

Conversation

plypaul
Copy link
Contributor

@plypaul plypaul commented Oct 24, 2023

Resolves #192

Description

Please see associated issue for more details.

Checklist

@cla-bot cla-bot bot added the cla:yes label Oct 24, 2023
@github-actions
Copy link

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@plypaul plypaul force-pushed the plypaul--52--saved-queries-group-by branch from 9d011fe to d2cb710 Compare October 24, 2023 07:06
@plypaul plypaul marked this pull request as ready for review October 24, 2023 17:47
@plypaul
Copy link
Contributor Author

plypaul commented Oct 24, 2023

@QMalcolm - How do we go about handling this kind of rename on the dbt-core side?

@QMalcolm
Copy link
Collaborator

@QMalcolm - How do we go about handling this kind of rename on the dbt-core side?

We can't, at least not for dbt-core 1.7, and that is a for a few reasons

  • dbt-core 1.7 will depend on DSI 0.3.x, and we can't backport this to 0.3.latest because it's breaking
  • dbt-core handles this via detecting manifest schema versions. Only 1 schema exists per dbt-core minor version
    • group_bys was introduced in 1.7.0rc1, thus there is no schema migration path for this property in core 1.7.latest

The only way to do this currently for dbt-core 1.7.0 and DSI 0.3.0 would be to support both, which also seems undesireable

@QMalcolm
Copy link
Collaborator

Also I have a few separate questions:

  1. Is there context on why we want to rename this? The listed metricflow issue has group_bys not group_by
  2. For this kind of work going forward can we get a DSI issue created? I had no idea this was coming down the pipe and I'm missing a lot of context of what we actually want to get it into. Additionally with automatically creating release changelog via changie we need to link to issues in this project.

@QMalcolm
Copy link
Collaborator

We'll put this into 0.4.0 and then jam that into the final cut of dbt-core 1.7.0

Copy link
Collaborator

@QMalcolm QMalcolm left a comment

Choose a reason for hiding this comment

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

Looks good to me!

.changes/unreleased/Fixes-20231023-235936.yaml Outdated Show resolved Hide resolved
@QMalcolm QMalcolm mentioned this pull request Oct 25, 2023
@plypaul plypaul changed the title Rename group_bys field in SavedQueries to group_by Rename SavedQuery.group_bys to SavedQuery.group_by Oct 26, 2023
@plypaul plypaul force-pushed the plypaul--52--saved-queries-group-by branch from d2cb710 to 2678fea Compare October 26, 2023 00:38
@plypaul
Copy link
Contributor Author

plypaul commented Oct 26, 2023

@QMalcolm - Good point, created an issue in DSI and updated links.

@courtneyholcomb courtneyholcomb merged commit 637cbb5 into main Oct 26, 2023
9 checks passed
@courtneyholcomb courtneyholcomb deleted the plypaul--52--saved-queries-group-by branch October 26, 2023 18:00
QMalcolm added a commit to dbt-labs/dbt-core that referenced this pull request Oct 26, 2023
In dbt-labs/dbt-semantic-interfaces#186 we
renamed `group_bys` to `group_by` in DSI (which went out in DSI 0.4.0).
The original `group_bys` naming was a typo. This fixes that. This is a
non-breaking change from the core perspective because saved queries
are being introduced in 1.7.0, which has not been cut yet.
QMalcolm added a commit to dbt-labs/dbt-core that referenced this pull request Nov 1, 2023
In dbt-labs/dbt-semantic-interfaces#186 we
renamed `group_bys` to `group_by` in DSI (which went out in DSI 0.4.0).
The original `group_bys` naming was a typo. This fixes that. This is a
non-breaking change from the core perspective because saved queries
are being introduced in 1.7.0, which has not been cut yet.
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.

[Bug] Rename SavedQuery.group_bys to SavedQuery.group_by
3 participants