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

Add more translations for Snowflake #860

Merged
merged 11 commits into from
Aug 12, 2022

Conversation

fh-afrachioni
Copy link
Contributor

@fh-afrachioni fh-afrachioni commented May 5, 2022

This request adds a number of additional translations for Snowflake backends, with the intent to translate all functions currently available for Postgres. Specifically, support is added for:

base

  • log10
  • grepl
  • round
  • paste and paste0

stringr

  • str_c
  • str_locate
  • str_detect
  • str_replace and str_replace_all
  • str_remove and str_remove_all
  • str_trim and str_squish

lubridate

  • day, mday, wday, yday, week, isoweek, month, quarter, isoyear
  • seconds, minutes, hours, days, weeks, months, years
  • floor_date

This fixes issue #935

@fh-afrachioni
Copy link
Contributor Author

Reopening after adding backend testing. Any feedback would be appreciated.

@fh-afrachioni fh-afrachioni reopened this Jul 6, 2022
@fh-afrachioni
Copy link
Contributor Author

@mgirlich @hadley The above failing checks seem unrelated to the changes, but I could be mistaken. Happy to address any other feedback!

Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! A bunch of small feedback follows.

R/backend-snowflake.R Outdated Show resolved Hide resolved
R/backend-snowflake.R Show resolved Hide resolved
R/backend-snowflake.R Outdated Show resolved Hide resolved
R/backend-snowflake.R Show resolved Hide resolved
R/backend-snowflake.R Outdated Show resolved Hide resolved
R/backend-snowflake.R Outdated Show resolved Hide resolved
R/backend-snowflake.R Outdated Show resolved Hide resolved
@hadley
Copy link
Member

hadley commented Jul 15, 2022

@mgirlich could you please take a quick look too? (just to make sure I haven't forgotten anything major since I haven't worked on dbplyr in a while)

build_sql("INTERVAL '", x, " year'")
},
# https://docs.snowflake.com/en/sql-reference/functions/date_trunc.html
floor_date = function(x, unit = "seconds") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The default is plural, below is always singular. I see this is also the case for the postgres backend...
Either you simply fix the default to "seconds" or if you want to be more user friendly you could also allow the plural version. Up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, maybe I'll follow up with an update to the postgres backend. Since the singular and plural versions are all supported on both Snowflake (here) and lubridate, I'm expanding the arg list to include both.

R/backend-snowflake.R Outdated Show resolved Hide resolved
R/backend-snowflake.R Outdated Show resolved Hide resolved
@mgirlich
Copy link
Collaborator

@fh-afrachioni Looks great overall. Thanks!

@mgirlich mgirlich added this to the 2.3.0 milestone Aug 2, 2022
@fh-afrachioni
Copy link
Contributor Author

Thanks for the review, @mgirlich; I just updated with the requested changes. Happy to address anything else!

@mgirlich mgirlich merged commit 10f65e7 into tidyverse:main Aug 12, 2022
@fh-afrachioni
Copy link
Contributor Author

Thanks for the review and approval, @hadley and @mgirlich! Do you know when the next release is planned? I have a team that would love to use these features within the next few weeks.

@hadley
Copy link
Member

hadley commented Aug 12, 2022

We've accumulated quite a few new features lately thanks to @mgirlich's hard work. So it would be nice to get a release out in the not too distant future. But no concrete plans yet.

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.

3 participants