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 generics and implementations for custom Date and POSIXt escaping #391

Merged
merged 10 commits into from Apr 16, 2020

Conversation

OssiLehtinen
Copy link
Contributor

@OssiLehtinen OssiLehtinen commented Dec 16, 2019

These modifications address the issue described in #384

@OssiLehtinen
Copy link
Contributor Author

OssiLehtinen commented Dec 16, 2019

Travis CI was not too happy about my pull request. I'm not familiar with Travis CI so not sure how to proceed.

@OssiLehtinen
Copy link
Contributor Author

OssiLehtinen commented Dec 18, 2019

I added export of sql_escape_string.

This would enable optionally setting, e.g.,

sql_escape_string.AthenaConnection <- function(con, x) {
  if(try(as.Date(x), silent=T)  == x) {
    paste0('date ', DBI::dbQuoteString(con, x))
  } else {
    DBI::dbQuoteString(con, x)
  }
}

meaning a call, such as tbl(con, "calendar") %>% filter(date_col == "2019-01-01"), would work, as the string gets identified as an isodate and the 'date' prefix gets added.
This is obviously very much subjective, but in pretty much all of our ad-hoc analysis use cases we need to do filtering of data by date, so any streamlining of this would go a long way.

However, I'm uncertain whether automatic detection of isodates should be the default behaviour. This change would enable doing this if wanted.

@OssiLehtinen
Copy link
Contributor Author

OssiLehtinen commented Dec 20, 2019

A bit of a mess, but I think something like this would actually be needed to have date vectors work out and have only iso-dates cast to dates. Automatically casting something like "1" to a date is not a good thing.

sql_escape_string.AthenaConnection <- function(con, x) {
  all_dates <- all(try(as.Date(x, tryFormats = "%Y-%m-%d"), silent=T) == x)
  if(all_dates & !is.na(all_dates)) {
    paste0('date ', DBI::dbQuoteString(con, x))
  } else {
    DBI::dbQuoteString(con, x)
  }
}

In any case, I don't think this should be the default behaviour of dbplyr, but exporting the generic would allow a user to shoot him/herself in the foot, if that is what he/she wishes to do.

@OssiLehtinen
Copy link
Contributor Author

OssiLehtinen commented Jan 3, 2020

One more note:

if one would define sql_escape_string.AthenaConnection as described above, the escape.Date definition would not be necessary, as currently the dates get cast to strings first and the sql_escape_string function is called on those, which would then detect them as dates.

However, still I don't think the sql_escape_string should be defined like that as default, so having the Athena-specific escape.Date (and escape.POSIXt) would be a good thing imho.

p.s. If I'm not mixing up things (still learning about s3-classes etc), exporting sql_escape_string is actually not necessary to be able to define the Athena specific version, right?

Copy link
Member

@hadley hadley left a comment

Looks good thanks — don't worry about the travis failures; I'll fix those shortly. Can you please add a bullet to the top of NEWS.md? It should briefly describe the change and end with (@yourname, #issuenumber).

Yeah, we definitely don't want to do that magic date wrapping by default. I'm not sure how can we make that more elegant in the future. Maybe we could add a ymd() helper than dbplyr knew to execute locally?

sql_escape_string() is complicated because it's exported by dplyr. At some point I have to centralise all the database methods in this package, but it's a bit fiddly.

R/escape.R Outdated
@@ -69,14 +69,13 @@ escape.factor <- function(x, parens = NA, collapse = ", ", con = NULL) {

#' @export
escape.Date <- function(x, parens = NA, collapse = ", ", con = NULL) {
x <- as.character(x)
escape.character(x, parens = parens, collapse = collapse, con = con)
x = as.character(x)
Copy link
Member

@hadley hadley Apr 15, 2020

Choose a reason for hiding this comment

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

I think you should delete this line; sql_escape_date() should be in charge of converting from a date to a character

Copy link
Contributor Author

@OssiLehtinen OssiLehtinen Apr 16, 2020

Choose a reason for hiding this comment

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

Moved to coverting bit to sql_escape_date.

R/escape.R Outdated
@@ -229,13 +228,37 @@ sql_escape_logical <- function(con, x) {
UseMethod("sql_escape_logical")
}

#' @keywords internal
#' @export
sql_escape_Date <- function(con, x) {
Copy link
Member

@hadley hadley Apr 15, 2020

Choose a reason for hiding this comment

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

I'd prefer to call these sql_escape_date() and sql_escape_datetime().

Copy link
Contributor Author

@OssiLehtinen OssiLehtinen Apr 16, 2020

Choose a reason for hiding this comment

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

Renamed the functions.

@hadley
Copy link
Member

hadley commented Apr 15, 2020

(Also since it's a been a while since you submitted this PR, please let me know if you're not interested, and I'll finish it)

R/escape.R Outdated
# DBIConnection methods --------------------------------------------------------

#' @export
sql_escape_string.DBIConnection <- function(con, x) {
dbQuoteString(con, x)
}

#' @export
sql_escape_Date.DBIConnection <- function(con, x) {
dbQuoteString(con, x)
Copy link
Member

@krlmlr krlmlr Apr 16, 2020

Choose a reason for hiding this comment

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

DBI has this, DBI backends can override.

Suggested change
dbQuoteString(con, x)
dbQuoteLiteral(con, x)

Copy link
Contributor Author

@OssiLehtinen OssiLehtinen Apr 16, 2020

Choose a reason for hiding this comment

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

I'm sorry, could you explain a bit what would be the benefit of this change? dbQuoteLiteral seems to format a date as something like '2020-04-15 UTC' which does not work at least in Athena (don't have other databases up right now to test against). So if I would use dbQuoteLiteral, I would still need to first cast the date as character.

Surely I'm just missing the point here, so please help me out :)

R/escape.R Outdated
x <- strftime(x, "%Y-%m-%dT%H:%M:%OSZ", tz = "UTC")
dbQuoteString(con, x)
Copy link
Member

@krlmlr krlmlr Apr 16, 2020

Choose a reason for hiding this comment

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

Should this be %OS6? DBI needs to be fixed here -- I think passing the time zone might be correct here.

Suggested change
x <- strftime(x, "%Y-%m-%dT%H:%M:%OSZ", tz = "UTC")
dbQuoteString(con, x)
dbQuoteLiteral(con, x)

Copy link
Contributor Author

@OssiLehtinen OssiLehtinen Apr 16, 2020

Choose a reason for hiding this comment

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

This specific formatting is what is in place in dbplyr at the moment for POSIXt, so I wouldn't want to touch that.

Ossi Lehtinen added 2 commits Apr 16, 2020
…Date and sql_escape_datetime and move the string casting to sql_escape_date() from escape.Date()
@OssiLehtinen
Copy link
Contributor Author

OssiLehtinen commented Apr 16, 2020

I made changes to the code according to the recommendations. I hope I got it right. Could you @hadley take a look at the news bullet, as I'm a bit uncertain about the terminology? Thanks!

@hadley
Copy link
Member

hadley commented Apr 16, 2020

Thanks @OssiLehtinen — I finished it by updating your branch with the latest changes on master, tweaking the news bullet, and making a couple of small documentation and test fixes. Thanks for all your work on this!

@OssiLehtinen
Copy link
Contributor Author

OssiLehtinen commented Apr 16, 2020

Thanks @OssiLehtinen — I finished it by updating your branch with the latest changes on master, tweaking the news bullet, and making a couple of small documentation and test fixes. Thanks for all your work on this!

I'm very happy to be able to contribute even a little bit to this great stack of tools!

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.

None yet

3 participants