correct sign of difftime for various DBMS' (#1532)

edward-burn · simonpcouch · web-flow · commit 1ff6363437c3 · 2024-09-09T12:20:11.000-04:00
---------

Co-authored-by: Simon P. Couch &lt;simonpatrickcouch@gmail.com&gt;
diff --git a/NEWS.md b/NEWS.md
@@ -1,5 +1,8 @@
 # dbplyr (development version)
 
+* Translations of `difftime()` for Postgres, SQL server, Redshift, and Snowflake
+  previously returned the wrong sign and are now correct (@edward-burn, #1532). 
+  
 * `across(everything())` doesn't select grouping columns created via `.by` in
   `summarise()` (@mgirlich, #1493).
   
diff --git a/R/backend-mssql.R b/R/backend-mssql.R
@@ -383,7 +383,7 @@ simulate_mssql <- function(version = "15.0") {
           cli::cli_abort('The only supported value for {.arg units} on SQL backends is "days"')
         }
 
-        sql_expr(DATEDIFF(DAY, !!time1, !!time2))
+        sql_expr(DATEDIFF(DAY, !!time2, !!time1))
       }
     )
 
diff --git a/R/backend-postgres.R b/R/backend-postgres.R
@@ -268,7 +268,7 @@ sql_translation.PqConnection <- function(con) {
           cli::cli_abort('The only supported value for {.arg units} on SQL backends is "days"')
         }
 
-        sql_expr((CAST(!!time2 %AS% DATE) - CAST(!!time1 %AS% DATE)))
+        sql_expr((CAST(!!time1 %AS% DATE) - CAST(!!time2 %AS% DATE)))
       },
     ),
     sql_translator(.parent = base_agg,
diff --git a/R/backend-redshift.R b/R/backend-redshift.R
@@ -94,7 +94,7 @@ sql_translation.RedshiftConnection <- function(con) {
           cli::cli_abort('The only supported value for {.arg units} on SQL backends is "days"')
         }
 
-        sql_expr(DATEDIFF(DAY, !!time1, !!time2))
+        sql_expr(DATEDIFF(DAY, !!time2, !!time1))
       }
     ),
     sql_translator(.parent = postgres$aggregate,
diff --git a/R/backend-snowflake.R b/R/backend-snowflake.R
@@ -243,7 +243,7 @@ sql_translation.Snowflake <- function(con) {
           cli::cli_abort('The only supported value for {.arg units} on SQL backends is "days"')
         }
 
-        sql_expr(DATEDIFF(DAY, !!time1, !!time2))
+        sql_expr(DATEDIFF(DAY, !!time2, !!time1))
       },
       # LEAST / GREATEST on Snowflake will not respect na.rm = TRUE by default (similar to Oracle/Access)
       # https://docs.snowflake.com/en/sql-reference/functions/least
diff --git a/tests/testthat/test-backend-mssql.R b/tests/testthat/test-backend-mssql.R
@@ -143,8 +143,8 @@ test_that("custom clock functions translated correctly", {
 
 test_that("difftime is translated correctly", {
   local_con(simulate_mssql())
-  expect_equal(test_translate_sql(difftime(start_date, end_date, units = "days")), sql("DATEDIFF(DAY, `start_date`, `end_date`)"))
-  expect_equal(test_translate_sql(difftime(start_date, end_date)), sql("DATEDIFF(DAY, `start_date`, `end_date`)"))
+  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_error(test_translate_sql(difftime(start_date, end_date, units = "auto")))
   expect_error(test_translate_sql(difftime(start_date, end_date, tz = "UTC", units = "days")))
diff --git a/tests/testthat/test-backend-postgres.R b/tests/testthat/test-backend-postgres.R
@@ -102,8 +102,8 @@ test_that("custom clock functions translated correctly", {
 
 test_that("difftime is translated correctly", {
   local_con(simulate_postgres())
-  expect_equal(test_translate_sql(difftime(start_date, end_date, units = "days")), sql("(CAST(`end_date` AS DATE) - CAST(`start_date` AS DATE))"))
-  expect_equal(test_translate_sql(difftime(start_date, end_date)), sql("(CAST(`end_date` AS DATE) - CAST(`start_date` AS DATE))"))
+  expect_equal(test_translate_sql(difftime(start_date, end_date, units = "days")), sql("(CAST(`start_date` AS DATE) - CAST(`end_date` AS DATE))"))
+  expect_equal(test_translate_sql(difftime(start_date, end_date)), sql("(CAST(`start_date` AS DATE) - CAST(`end_date` AS DATE))"))
 
   expect_error(test_translate_sql(difftime(start_date, end_date, units = "auto")))
   expect_error(test_translate_sql(difftime(start_date, end_date, tz = "UTC", units = "days")))
diff --git a/tests/testthat/test-backend-redshift.R b/tests/testthat/test-backend-redshift.R
@@ -72,8 +72,8 @@ test_that("custom clock functions translated correctly", {
 
 test_that("difftime is translated correctly", {
   local_con(simulate_redshift())
-  expect_equal(test_translate_sql(difftime(start_date, end_date, units = "days")), sql("DATEDIFF(DAY, `start_date`, `end_date`)"))
-  expect_equal(test_translate_sql(difftime(start_date, end_date)), sql("DATEDIFF(DAY, `start_date`, `end_date`)"))
+  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_error(test_translate_sql(difftime(start_date, end_date, units = "auto")))
   expect_error(test_translate_sql(difftime(start_date, end_date, tz = "UTC", units = "days")))
diff --git a/tests/testthat/test-backend-snowflake.R b/tests/testthat/test-backend-snowflake.R
@@ -116,8 +116,8 @@ test_that("custom clock functions translated correctly", {
 
 test_that("difftime is translated correctly", {
   local_con(simulate_snowflake())
-  expect_equal(test_translate_sql(difftime(start_date, end_date, units = "days")), sql("DATEDIFF(DAY, `start_date`, `end_date`)"))
-  expect_equal(test_translate_sql(difftime(start_date, end_date)), sql("DATEDIFF(DAY, `start_date`, `end_date`)"))
+  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_error(test_translate_sql(difftime(start_date, end_date, units = "auto")))
   expect_error(test_translate_sql(difftime(start_date, end_date, tz = "UTC", units = "days")))

Original file line number	Diff line number	Diff line change
`@@ -383,7 +383,7 @@ simulate_mssql <- function(version = "15.0") {`
`383`	`383`	`cli::cli_abort('The only supported value for {.arg units} on SQL backends is "days"')`
`384`	`384`	`}`
`385`	`385`
`386`		`- sql_expr(DATEDIFF(DAY, !!time1, !!time2))`
	`386`	`+ sql_expr(DATEDIFF(DAY, !!time2, !!time1))`
`387`	`387`	`}`
`388`	`388`	`)`
`389`	`389`
Original file line number	Diff line number	Diff line change
`@@ -268,7 +268,7 @@ sql_translation.PqConnection <- function(con) {`
`268`	`268`	`cli::cli_abort('The only supported value for {.arg units} on SQL backends is "days"')`
`269`	`269`	`}`
`270`	`270`
`271`		`- sql_expr((CAST(!!time2 %AS% DATE) - CAST(!!time1 %AS% DATE)))`
	`271`	`+ sql_expr((CAST(!!time1 %AS% DATE) - CAST(!!time2 %AS% DATE)))`
`272`	`272`	`},`
`273`	`273`	`),`
`274`	`274`	`sql_translator(.parent = base_agg,`
Original file line number	Diff line number	Diff line change
`@@ -94,7 +94,7 @@ sql_translation.RedshiftConnection <- function(con) {`
`94`	`94`	`cli::cli_abort('The only supported value for {.arg units} on SQL backends is "days"')`
`95`	`95`	`}`
`96`	`96`
`97`		`- sql_expr(DATEDIFF(DAY, !!time1, !!time2))`
	`97`	`+ sql_expr(DATEDIFF(DAY, !!time2, !!time1))`
`98`	`98`	`}`
`99`	`99`	`),`
`100`	`100`	`sql_translator(.parent = postgres$aggregate,`
Original file line number	Diff line number	Diff line change
`@@ -243,7 +243,7 @@ sql_translation.Snowflake <- function(con) {`
`243`	`243`	`cli::cli_abort('The only supported value for {.arg units} on SQL backends is "days"')`
`244`	`244`	`}`
`245`	`245`
`246`		`- sql_expr(DATEDIFF(DAY, !!time1, !!time2))`
	`246`	`+ sql_expr(DATEDIFF(DAY, !!time2, !!time1))`
`247`	`247`	`},`
`248`	`248`	`# LEAST / GREATEST on Snowflake will not respect na.rm = TRUE by default (similar to Oracle/Access)`
`249`	`249`	`# https://docs.snowflake.com/en/sql-reference/functions/least`