Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: Add dbplyr translations for clock::add_days(), clock::add_years(), clock::get_day(), clock::get_month(), and clock::get_year() #153

Merged
merged 13 commits into from
May 7, 2024

Conversation

edward-burn
Copy link
Contributor

For isssue #116

  • add translations for clock::add_days() and clock::add_years()
  • add associated tests

@edward-burn
Copy link
Contributor Author

@krlmlr I have open this for issue #116. Hopefully it is ready for review.

Copy link
Collaborator

@krlmlr krlmlr 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 looking into it!

sql <- function(...) dbplyr::sql(...)

expect_equal(translate(add_days(x, 1L)), sql(r"{date_add(x, INTERVAL '1 day')}"))
expect_equal(translate(add_years(x, 2L)), sql(r"{date_add(x, INTERVAL '2 day')}"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
expect_equal(translate(add_years(x, 2L)), sql(r"{date_add(x, INTERVAL '2 day')}"))
expect_equal(translate(add_days(x, 2L)), sql(r"{date_add(x, INTERVAL '2 day')}"))

What happens with negative days? It's also interesting to see an integration test -- running an actual query with negative months, let's say.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@krlmlr sure, here's a reprex (comparing clock's behaviour for a dataframe with how this duckdb implementation works) and it seems to work as expected for both adding days and years as well as substracting them

options(conflicts.policy = list(warn = FALSE))
library(DBI)
#> Warning: package 'DBI' was built under R version 4.2.3
library(RPostgres)
#> Warning: package 'RPostgres' was built under R version 4.2.3
library(dplyr)
#> Warning: package 'dplyr' was built under R version 4.2.3
library(dbplyr)
#> Warning: package 'dbplyr' was built under R version 4.2.3
library(clock)
#> Warning: package 'clock' was built under R version 4.2.3
library(duckdb)

con <- DBI::dbConnect(duckdb(), path = ":memory:")
test_data <- data.frame(
  person = 1L,
  date_1 = as.Date("2000-01-01")
)
test_data |> 
  mutate(date_plus_two_days  = clock::add_days(date_1, 2), 
         date_plus_two_years = clock::add_years(date_1, 2),
         date_minus_two_days  = clock::add_days(date_1, -2), 
         date_minus_two_years = clock::add_years(date_1, -2)) |> 
  dplyr::glimpse()
#> Rows: 1
#> Columns: 6
#> $ person               <int> 1
#> $ date_1               <date> 2000-01-01
#> $ date_plus_two_days   <date> 2000-01-03
#> $ date_plus_two_years  <date> 2002-01-01
#> $ date_minus_two_days  <date> 1999-12-30
#> $ date_minus_two_years <date> 1998-01-01

db_test_data <- copy_to(con, test_data, overwrite = TRUE)
db_test_data |> 
  mutate(date_plus_two_days  = clock::add_days(date_1, 2), 
         date_plus_two_years = clock::add_years(date_1, 2),
         date_minus_two_days  = clock::add_days(date_1, -2), 
         date_minus_two_years = clock::add_years(date_1, -2)) |> 
  dplyr::glimpse()
#> Rows: ??
#> Columns: 6
#> Database: DuckDB v0.10.3-dev163 [eburn@Windows 10 x64:R 4.2.1/:memory:]
#> $ person               <int> 1
#> $ date_1               <date> 2000-01-01
#> $ date_plus_two_days   <dttm> 2000-01-03
#> $ date_plus_two_years  <dttm> 2002-01-01
#> $ date_minus_two_days  <dttm> 1999-12-30
#> $ date_minus_two_years <dttm> 1998-01-01

Created on 2024-05-03 with reprex v2.0.2

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the example. Having a code like this in a test would be useful too.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.34%. Comparing base (8fa2d7b) to head (4934d40).
Report is 26 commits behind head on main.

❗ Current head 4934d40 differs from pull request most recent head ffbe5e4. Consider uploading reports for the commit ffbe5e4 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #153      +/-   ##
==========================================
- Coverage   70.19%   63.34%   -6.85%     
==========================================
  Files         118      120       +2     
  Lines        4472     5118     +646     
==========================================
+ Hits         3139     3242     +103     
- Misses       1333     1876     +543     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@edward-burn
Copy link
Contributor Author

@krlmlr have added those examples as tests. I have also added the related clock functions get_day, get_month, get_year to this pr (hope that is not expanding it too much, can remove if it is)

@krlmlr krlmlr changed the title Add support for clock::add_days() and clock::add_years() feat: Add dbplyr translations for clock::add_days(), clock::add_years(), clock::get_day(), clock::get_month(), and clock::get_year() May 6, 2024
@edward-burn
Copy link
Contributor Author

Hi @krlmlr, hopefully this is getting there. I saw a note Undefined global functions or variables: DATE_PART, so I switched to using build_sql throughout which hopefully should fix that .

@krlmlr krlmlr merged commit f333cde into duckdb:main May 7, 2024
24 checks passed
@krlmlr
Copy link
Collaborator

krlmlr commented May 7, 2024

Thank you so much for your patience!

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

Successfully merging this pull request may close these issues.

3 participants