From 717810988dce4f40eba881a5ea3281f33ca3fc7f Mon Sep 17 00:00:00 2001 From: simonpcouch Date: Thu, 31 Oct 2024 13:11:03 -0500 Subject: [PATCH 1/4] use type checkers in `backend-snowflake.R` --- R/backend-snowflake.R | 29 +++++++++++----------- tests/testthat/_snaps/backend-snowflake.md | 16 ------------ tests/testthat/test-backend-snowflake.R | 22 ++++++++++------ 3 files changed, 29 insertions(+), 38 deletions(-) diff --git a/R/backend-snowflake.R b/R/backend-snowflake.R index 8219da421..710f4bd83 100644 --- a/R/backend-snowflake.R +++ b/R/backend-snowflake.R @@ -27,6 +27,7 @@ sql_translation.Snowflake <- function(con) { paste = snowflake_paste(" "), paste0 = snowflake_paste(""), str_c = function(..., sep = "", collapse = NULL) { + check_string(sep) if (!is.null(collapse)) { cli_abort(c( "{.arg collapse} not supported in DB translation of {.fn str_c}.", @@ -40,6 +41,7 @@ sql_translation.Snowflake <- function(con) { }, str_detect = function(string, pattern, negate = FALSE) { con <- sql_current_con() + check_bool(negate) # Snowflake needs backslashes escaped, so we must increase the level of escaping pattern <- gsub("\\", "\\\\", pattern, fixed = TRUE) @@ -51,6 +53,7 @@ sql_translation.Snowflake <- function(con) { }, str_starts = function(string, pattern, negate = FALSE) { con <- sql_current_con() + check_bool(negate) # Snowflake needs backslashes escaped, so we must increase the level of escaping pattern <- gsub("\\", "\\\\", pattern, fixed = TRUE) @@ -62,6 +65,7 @@ sql_translation.Snowflake <- function(con) { }, str_ends = function(string, pattern, negate = FALSE) { con <- sql_current_con() + check_bool(negate) # Snowflake needs backslashes escaped, so we must increase the level of escaping pattern <- gsub("\\", "\\\\", pattern, fixed = TRUE) @@ -111,6 +115,8 @@ sql_translation.Snowflake <- function(con) { sql_expr(EXTRACT(DAY %FROM% !!x)) }, wday = function(x, label = FALSE, abbr = TRUE, week_start = NULL) { + check_bool(label) + check_bool(abbr) if (!label) { week_start <- week_start %||% getOption("lubridate.week.start", 7) offset <- as.integer(7 - week_start) @@ -140,6 +146,8 @@ sql_translation.Snowflake <- function(con) { }, isoweek = function(x) sql_expr(EXTRACT("weekiso", !!x)), month = function(x, label = FALSE, abbr = TRUE) { + check_bool(label) + check_bool(abbr) if (!label) { sql_expr(EXTRACT("month", !!x)) } else { @@ -167,6 +175,7 @@ sql_translation.Snowflake <- function(con) { } }, quarter = function(x, with_year = FALSE, fiscal_start = 1) { + check_bool(with_year) check_unsupported_arg(fiscal_start, 1) if (with_year) { @@ -220,6 +229,8 @@ sql_translation.Snowflake <- function(con) { sql_expr(DATEADD(YEAR, !!n, !!x)) }, date_build = function(year, month = 1L, day = 1L, ..., invalid = NULL) { + check_dots_empty() + check_unsupported_arg(invalid, allowed = NULL) # https://docs.snowflake.com/en/sql-reference/functions/date_from_parts sql_expr(DATE_FROM_PARTS(!!year, !!month, !!day)) }, @@ -235,25 +246,15 @@ sql_translation.Snowflake <- function(con) { date_count_between = function(start, end, precision, ..., n = 1L){ check_dots_empty() - if (precision != "day") { - cli_abort("{.arg precision} must be {.val day} on SQL backends.") - } - if (n != 1) { - cli_abort("{.arg n} must be {.val 1} on SQL backends.") - } + check_unsupported_arg(precision, allowed = "day") + check_unsupported_arg(n, allowed = 1L) sql_expr(DATEDIFF(DAY, !!start, !!end)) }, difftime = function(time1, time2, tz, units = "days") { - - if (!missing(tz)) { - cli::cli_abort("The {.arg tz} argument is not supported for SQL backends.") - } - - if (units[1] != "days") { - cli::cli_abort('The only supported value for {.arg units} on SQL backends is "days"') - } + check_unsupported_arg(tz) + check_unsupported_arg(units, allowed = "days") sql_expr(DATEDIFF(DAY, !!time2, !!time1)) }, diff --git a/tests/testthat/_snaps/backend-snowflake.md b/tests/testthat/_snaps/backend-snowflake.md index 1c3ed964a..ccb978075 100644 --- a/tests/testthat/_snaps/backend-snowflake.md +++ b/tests/testthat/_snaps/backend-snowflake.md @@ -7,22 +7,6 @@ ! `collapse` not supported in DB translation of `paste()`. i Please use `str_flatten()` instead. -# difftime is translated correctly - - Code - test_translate_sql(difftime(start_date, end_date, units = "auto")) - Condition - Error in `difftime()`: - ! The only supported value for `units` on SQL backends is "days" - ---- - - Code - test_translate_sql(difftime(start_date, end_date, tz = "UTC", units = "days")) - Condition - Error in `difftime()`: - ! The `tz` argument is not supported for SQL backends. - # pmin() and pmax() respect na.rm Code diff --git a/tests/testthat/test-backend-snowflake.R b/tests/testthat/test-backend-snowflake.R index a865587cf..467c1cc12 100644 --- a/tests/testthat/test-backend-snowflake.R +++ b/tests/testthat/test-backend-snowflake.R @@ -120,8 +120,14 @@ test_that("custom clock functions translated correctly", { expect_equal(test_translate_sql(get_day(date_column)), sql("DATE_PART(DAY, `date_column`)")) expect_equal(test_translate_sql(date_count_between(date_column_1, date_column_2, "day")), sql("DATEDIFF(DAY, `date_column_1`, `date_column_2`)")) - expect_error(test_translate_sql(date_count_between(date_column_1, date_column_2, "year"))) - expect_error(test_translate_sql(date_count_between(date_column_1, date_column_2, "day", n = 5))) + expect_error( + test_translate_sql(date_count_between(date_column_1, date_column_2, "year")), + class = "dbplyr_error_unsupported_arg" + ) + expect_error( + test_translate_sql(date_count_between(date_column_1, date_column_2, "day", n = 5)), + class = "dbplyr_error_unsupported_arg" + ) }) test_that("difftime is translated correctly", { @@ -129,13 +135,13 @@ test_that("difftime is translated correctly", { expect_equal(test_translate_sql(difftime(start_date, end_date, units = "days")), sql("DATEDIFF(DAY, `end_date`, `start_date`)")) expect_equal(test_translate_sql(difftime(start_date, end_date)), sql("DATEDIFF(DAY, `end_date`, `start_date`)")) - expect_snapshot( - error = TRUE, - test_translate_sql(difftime(start_date, end_date, units = "auto")) + expect_error( + test_translate_sql(difftime(start_date, end_date, units = "auto")), + class = "dbplyr_error_unsupported_arg" ) - expect_snapshot( - error = TRUE, - test_translate_sql(difftime(start_date, end_date, tz = "UTC", units = "days")) + expect_error( + test_translate_sql(difftime(start_date, end_date, tz = "UTC", units = "days")), + class = "dbplyr_error_unsupported_arg" ) }) From 6f912fc67203bc81c0a12ee4e62231e52c006d3c Mon Sep 17 00:00:00 2001 From: simonpcouch Date: Thu, 31 Oct 2024 13:15:41 -0500 Subject: [PATCH 2/4] add NEWS bullet --- NEWS.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/NEWS.md b/NEWS.md index e08ad2d69..722b5f4aa 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,10 @@ # dbplyr (development version) +* Tightened argument checks for Snowflake SQL translations. These changes should + result in more informative errors in cases where code already failed; if you + see errors with code that used to work correctly, please report them to + the package authors (@simonpcouch, #1554). + * `clock::add_years()` translates to correct SQL on Spark (@ablack3, #1510). * Translations for `as.double()` and `as.character()` with Teradata previously From 9069c572eda4154356def08844c0e81b0ede83ab Mon Sep 17 00:00:00 2001 From: simonpcouch Date: Thu, 31 Oct 2024 13:31:54 -0500 Subject: [PATCH 3/4] clarify phrasing --- NEWS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index 722b5f4aa..00d832e34 100644 --- a/NEWS.md +++ b/NEWS.md @@ -2,7 +2,7 @@ * Tightened argument checks for Snowflake SQL translations. These changes should result in more informative errors in cases where code already failed; if you - see errors with code that used to work correctly, please report them to + see errors with code that used to run without issue, please report them to the package authors (@simonpcouch, #1554). * `clock::add_years()` translates to correct SQL on Spark (@ablack3, #1510). From fe2f3a790517ea68f68efb56b07ba6e29f21aca8 Mon Sep 17 00:00:00 2001 From: simonpcouch Date: Thu, 31 Oct 2024 16:36:54 -0500 Subject: [PATCH 4/4] check `wday(week_start)` --- R/backend-snowflake.R | 1 + 1 file changed, 1 insertion(+) diff --git a/R/backend-snowflake.R b/R/backend-snowflake.R index 710f4bd83..af132d6af 100644 --- a/R/backend-snowflake.R +++ b/R/backend-snowflake.R @@ -117,6 +117,7 @@ sql_translation.Snowflake <- function(con) { wday = function(x, label = FALSE, abbr = TRUE, week_start = NULL) { check_bool(label) check_bool(abbr) + check_number_whole(week_start, allow_null = TRUE) if (!label) { week_start <- week_start %||% getOption("lubridate.week.start", 7) offset <- as.integer(7 - week_start)