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

Jump to bottom

Engine-specific rendering for sub-daily granularity options #1258

Merged
merged 6 commits into from
Jun 11, 2024
Merged

Engine-specific rendering for sub-daily granularity options #1258

merged 6 commits into from
Jun 11, 2024

Conversation

courtneyholcomb
Copy link
Contributor

Support for rendering DATE_TRUNC statements with sub-daily granularity options.
Includes validation for engines that don't support certain granularities.
Note about validation: warehouse validations in YAML would be ideal, but we don't have those currently. Query parsing validations would also be ideal, but the query parse has no awareness of the engine being queried. This seemed like the next best option, and it's such an rare edge case that I'm not too concerned about optimizing this error.

@courtneyholcomb courtneyholcomb requested review from tlento and plypaul June 7, 2024 23:12
@cla-bot cla-bot bot added the cla:yes label Jun 7, 2024
@dbt-labs dbt-labs deleted a comment from github-actions bot Jun 7, 2024
@courtneyholcomb courtneyholcomb marked this pull request as ready for review June 7, 2024 23:13
tlento
tlento reviewed Jun 8, 2024
View reviewed changes
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.

The BigQuery update will not work as expected for sub-daily granularities. To be honest I don't know what'll happen when you put a DATETIME in there, probably a type mismatch error.

metricflow/protocols/sql_client.py Outdated Show resolved Hide resolved
metricflow/sql/render/expr_renderer.py Show resolved Hide resolved
metricflow/sql/render/big_query.py Outdated Show resolved Hide resolved
metricflow/sql/render/big_query.py Outdated Show resolved Hide resolved
@courtneyholcomb
Copy link
Contributor Author

@tlento updated!

@courtneyholcomb courtneyholcomb requested a review from tlento June 10, 2024 17:18
@tlento tlento added the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Jun 10, 2024
@tlento tlento had a problem deploying to DW_INTEGRATION_TESTS June 10, 2024 18:28 — with GitHub Actions Failure
@tlento tlento temporarily deployed to DW_INTEGRATION_TESTS June 10, 2024 18:28 — with GitHub Actions Inactive
@tlento tlento temporarily deployed to DW_INTEGRATION_TESTS June 10, 2024 18:28 — with GitHub Actions Inactive
@tlento tlento had a problem deploying to DW_INTEGRATION_TESTS June 10, 2024 18:28 — with GitHub Actions Failure
tlento
tlento reviewed Jun 10, 2024
View reviewed changes
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.

Engine failures abound. Looks like a snapshot update and some way of installing databricks to make them happy (editable install in the databricks env, I think) should do it.

@github-actions github-actions bot removed the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Jun 10, 2024
@courtneyholcomb courtneyholcomb added the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Jun 10, 2024
@courtneyholcomb courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS June 10, 2024 19:23 — with GitHub Actions Inactive
@courtneyholcomb courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS June 10, 2024 19:23 — with GitHub Actions Inactive
@courtneyholcomb courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS June 10, 2024 19:23 — with GitHub Actions Inactive
@courtneyholcomb courtneyholcomb temporarily deployed to DW_INTEGRATION_TESTS June 10, 2024 19:23 — with GitHub Actions Inactive
@github-actions github-actions bot removed the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Jun 10, 2024
@courtneyholcomb
Copy link
Contributor Author

@tlento SORRY! Should be good now!

@courtneyholcomb courtneyholcomb requested a review from tlento June 10, 2024 19:38
tlento
tlento approved these changes Jun 11, 2024
View reviewed changes
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.

Let's goooooo! 🚀

Base automatically changed from court/upgrade to main June 11, 2024 23:15
@courtneyholcomb courtneyholcomb enabled auto-merge (squash) June 11, 2024 23:17
@courtneyholcomb courtneyholcomb merged commit 06a1467 into main Jun 11, 2024
15 checks passed
@courtneyholcomb courtneyholcomb deleted the court/subd-query branch June 11, 2024 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tlento tlento tlento approved these changes

@plypaul plypaul Awaiting requested review from plypaul

Assignees
No one assigned
Labels
cla:yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@courtneyholcomb @tlento