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

Fix week_start on Snowflake #54

Merged
merged 1 commit into from
Jan 13, 2022
Merged

Fix week_start on Snowflake #54

merged 1 commit into from
Jan 13, 2022

Conversation

clausherther
Copy link
Contributor

week_start had a buggy and incomplete implementation on Snowflake. This PR intends to simplify and fix this.

Closes #53

@clausherther
Copy link
Contributor Author

clausherther commented Jan 12, 2022

@jpmmcneill let me know if this makes sense to you. I think we had an incomplete macro there...

The other issue was that n_days_ago expects only integers, not expressions, so it was setting the offset to 0. :/

@davesgonechina
Copy link
Contributor

Coincidentally I am also on Snowflake and noticed the week_start_date, week_end_date, and their prior counterparts were all null in the table I was generating with get_date_dimension, which calls week_start.

This PR works but only if you also follow Snowflake's recommendation to change from the legacy week_start==0 to, in my case, week_start==1.

@clausherther
Copy link
Contributor Author

@davesgonechina my Snowflake setup is using week_start: 0 and this PR seems to work fine there?

image

@davesgonechina
Copy link
Contributor

@clausherther and when you use get_date_dimension, are the week_start_date, week_end_date, and their prior_ counterparts populated? Because mine are not.

@clausherther
Copy link
Contributor Author

@davesgonechina using this PR branch, here's what I see on my Snowflake instance after building all tables and running all tests:
(dates is built using get_date_dimension and dim_date comes from dates.
image

@davesgonechina
Copy link
Contributor

@clausherther I take it back, you're right that it works with either parameter value, and solves the null column issue for get_date_dimension. Cheers!

@clausherther
Copy link
Contributor Author

Going to go ahead and merge this and cut a new patch release. If there's something else, feel free to reopen the original issue (#53). Thanks!

@clausherther clausherther merged commit 2f06e3e into main Jan 13, 2022
@clausherther clausherther deleted the fix/snowflake-week-start branch January 13, 2022 20:49
@jpmmcneill
Copy link
Contributor

jpmmcneill commented Jan 14, 2022

Thanks @clausherther! Apols that I didn't contribute the PR, missed these notifications. I'll test it out today on the new patch and LYK how it's looking!

@jpmmcneill
Copy link
Contributor

@clausherther verified working for me on 0.5.1. Thanks for the quick fix, i'd love to contribute to this package in future! I'll keep an eye on the issues that come in :)

@clausherther
Copy link
Contributor Author

Thanks for checking @jpmmcneill! Sorry, didn't mean to jump in front on that one. I'm sure there'll be plenty of other bugs to squash in the future!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Week start macro returning all nulls
3 participants