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 Redshift lag and lead functions without the default parameter #549

Merged
merged 5 commits into from
Jan 22, 2021

Conversation

hdplsa
Copy link
Contributor

@hdplsa hdplsa commented Nov 26, 2020

Fixes #548

This pull request removes the default argument from the LAG and LEAD SQL functions. Redshift assumes that the default parameter is NULL, therefore, we added an error in case that the user gives an alternative value for default.

Here is a reprex for the same code in #548 but with the fix applied:

library(dbplyr)
library(DBI)
library(reprex)

con <- dbConnect(RPostgres::Redshift(),
                 host = "",
                 dbname = "",
                 port = ,
                 user = "",
                 password = )

table.sql <- sql("SELECT 1 AS YR, 1 AS MON, 123456 AS SALES
                  UNION ALL
                  SELECT 1 AS YR, 2 AS MON, 234567 AS SALES
                  UNION ALL
                  SELECT 1 AS YR, 3 AS MON, 345678 AS SALES
                  UNION ALL
                  SELECT 1 AS YR, 4 AS MON, NULL AS SALES
                  UNION ALL
                  SELECT 2 AS YR, 1 AS MON, 123456 AS SALES
                  UNION ALL
                  SELECT 2 AS YR, 2 AS MON, 234567 AS SALES
                  UNION ALL
                  SELECT 2 AS YR, 3 AS MON, 345678 AS SALES
                  UNION ALL
                  SELECT 3 AS YR, 1 AS MON, 123456 AS SALES
                  ")

new.table <- con %>% dplyr::tbl(table.sql)

new.table %>%
  dbplyr::window_order(yr, mon) %>%
  dplyr::mutate(LG = lag(sales), LD = lead(sales)) %>%
  dplyr::arrange(yr, mon)
#> # Source:     lazy query [?? x 5]
#> # Database:   postgres
#> #   []
#> # Ordered by: yr, mon, yr, mon
#>      yr   mon  sales     lg     ld
#>   <int> <int>  <int>  <int>  <int>
#> 1     1     1 123456     NA 234567
#> 2     1     2 234567 123456 345678
#> 3     1     3 345678 234567     NA
#> 4     1     4     NA 345678 123456
#> 5     2     1 123456     NA 234567
#> 6     2     2 234567 123456 345678
#> 7     2     3 345678 234567 123456
#> 8     3     1 123456 345678     NA

Created on 2020-11-26 by the reprex package (v0.3.0)

And if the user gives a default value other than NA:

new.table %>%
  dbplyr::window_order(yr, mon) %>%
  dplyr::mutate(LG = lag(sales, default = 2), LD = lead(sales)) %>%
  dplyr::arrange(yr, mon)
#> Error in lag(sales, default = 2): Redshift does not support `default` values in LAG. By design `default` is always NULL.

Created on 2020-11-26 by the reprex package (v0.3.0)

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.

Can you please also add a bullet to the top of NEWS.md? It should briefly describe the change and end with (@yourname, #issuenumber).

sql_translator(.parent = postgres$window)
sql_translator(.parent = postgres$window,
# https://docs.aws.amazon.com/redshift/latest/dg/r_WF_LAG.html
lag = function(x, n = 1L, default = NA, order_by = NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this would be simpler if you just omitted the default argument all together — then the user will just get the regular unused argument (default = 1) error

expect_equal(translate_sql(lead(x)), sql("LEAD(`x`, 1) OVER ()"))
expect_equal(translate_sql(lag(x)), sql("LAG(`x`, 1) OVER ()"))

expect_error(translate_sql(lead(x, default = y)), "Redshift does not support")
Copy link
Member

Choose a reason for hiding this comment

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

This will also need to be updated

@hdplsa
Copy link
Contributor Author

hdplsa commented Jan 22, 2021

Hi @hadley, just did the changes you suggested.

@hadley hadley merged commit a4872e2 into tidyverse:master Jan 22, 2021
@hadley
Copy link
Member

hadley commented Jan 22, 2021

Thank you!

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.

LAG and LEAD do not have default argument in Redshift
2 participants