-
-
Notifications
You must be signed in to change notification settings - Fork 880
added options immedate & replace to eng_SQL #2128
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
base: master
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -560,13 +560,14 @@ is_sql_update_query = function(query) { | |||||||||||||||||||
| grepl('^\\s*(INSERT|UPDATE|DELETE|CREATE|DROP).*', query, ignore.case = TRUE) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| # sql engine | ||||||||||||||||||||
| eng_sql = function(options) { | ||||||||||||||||||||
| # return chunk before interpolation eagerly to avoid connection option check | ||||||||||||||||||||
| if (isFALSE(options$eval) && !isTRUE(options$sql.show_interpolated)) { | ||||||||||||||||||||
| return(engine_output(options, options$code, '')) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| # Return char vector of sql interpolation param names | ||||||||||||||||||||
| varnames_from_sql = function(conn, sql) { | ||||||||||||||||||||
| varPos = DBI::sqlParseVariables(conn, sql) | ||||||||||||||||||||
|
|
@@ -575,27 +576,27 @@ eng_sql = function(options) { | |||||||||||||||||||
| sub('^\\?', '', varNames) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| # Vectorized version of exists | ||||||||||||||||||||
| mexists = function(x, env = knit_global(), inherits = TRUE) { | ||||||||||||||||||||
| vapply(x, exists, logical(1), where = env, inherits = inherits) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| # Interpolate a sql query based on the variables in an environment | ||||||||||||||||||||
| interpolate_from_env = function(conn, sql, env = knit_global(), inherits = TRUE) { | ||||||||||||||||||||
| names = unique(varnames_from_sql(conn, sql)) | ||||||||||||||||||||
| names_missing = names[!mexists(names, env, inherits)] | ||||||||||||||||||||
| if (length(names_missing) > 0) { | ||||||||||||||||||||
| stop("Object(s) not found: ", paste('"', names_missing, '"', collapse = ", ")) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| args = if (length(names) > 0) setNames( | ||||||||||||||||||||
| mget(names, envir = env, inherits = inherits), names | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| do.call(DBI::sqlInterpolate, c(list(conn, sql), args)) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| # extract options | ||||||||||||||||||||
| conn = options$connection | ||||||||||||||||||||
| if (is.character(conn)) conn = get(conn, envir = knit_global()) | ||||||||||||||||||||
|
|
@@ -608,13 +609,26 @@ eng_sql = function(options) { | |||||||||||||||||||
| max.print = -1 | ||||||||||||||||||||
| sql = one_string(options$code) | ||||||||||||||||||||
| params = options$params | ||||||||||||||||||||
|
|
||||||||||||||||||||
| immediate= F | ||||||||||||||||||||
|
||||||||||||||||||||
| if(isTRUE(options$immediate)) { | ||||||||||||||||||||
| immediate= T | ||||||||||||||||||||
| replace= F | ||||||||||||||||||||
| if(isTRUE(options$replace)) { | ||||||||||||||||||||
| replace= T | ||||||||||||||||||||
|
||||||||||||||||||||
| immediate= F | |
| if(isTRUE(options$immediate)) { | |
| immediate= T | |
| replace= F | |
| if(isTRUE(options$replace)) { | |
| replace= T | |
| immediate = isTRUE(options$immediate) | |
| replace = isTRUE(options$replace) | |
| if (immediate && replace) { |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would use sql. prefix on those new options, to go with existing sql.print and sql.show_interpolated.
@yihui I don't know if we can end up with a rule on under which condition which should use engine.opts, bare chunk option name or prefixed option name. I think we should try being consistent. sql.show_interpolated is not really documented so we could also support show_interpolated to be consistent.
Probably than the chunk engine is enough to scope the variable (advanced usage of chunk options hooks would require checking the engine, and then checking the option in case of same option name used by two different engine with different meaning - may never happen though).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let's use the sql. prefix for the new options here.
I don't know if we can end up with a rule on under which condition which should use
engine.opts, bare chunk option name or prefixed option name. I think we should try being consistent.
I agree. It has been a mess, which I don't like, but I feel it's tough to decide which is better:
```{sql, engine.opts = list(immediate = TRUE, replace = TRUE)}```{sql, sql.immediate = TRUE, sql.replace = TRUE}chunk engine is enough to scope the variable (advanced usage of chunk options hooks would require checking the engine, and then checking the option in case of same option name used by two different engine with different meaning - may never happen though).
That's also what I think.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would use sql. prefix on those new options, to go with existing sql.print and sql.show_interpolated.
@yihui I don't know if we can end up with a rule on under which condition which should use engine.opts, bare chunk option name or prefixed option name. I think we should try being consistent. sql.show_interpolated is not really documented so we could also support show_interpolated to be consistent.
Probably than the chunk engine is enough to scope the variable (advanced usage of chunk options hooks would require checking the engine, and then checking the option in case of same option name used by two different engine with different meaning - may never happen though).
cderv marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to take precaution here.
DBI::dbExecute() is a generic and immediate argument, is indeed among the specifications. However, immediate is not part of the generic argument to improve compatibility with the different backends.
https://dbi.r-dbi.org/reference/dbexecute#additional-arguments-1
The following arguments are not part of the dbExecute() generic (to improve compatibility across backends) but are part of the DBI specification:
- params (default: NULL)
- immediate (default: NULL)
They must be provided as named arguments. See the "Specification" sections for details on their usage
This is valid for other calls too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to take precaution here.
DBI::dbExecute() is a generic and immediate argument, is indeed among the specifications. However, immediate is not part of the generic argument to improve compatibility with the different backends.
https://dbi.r-dbi.org/reference/dbexecute#additional-arguments-1
The following arguments are not part of the dbExecute() generic (to improve compatibility across backends) but are part of the DBI specification:
- params (default: NULL)
- immediate (default: NULL)
They must be provided as named arguments. See the "Specification" sections for details on their usage
This is valid for other calls too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DBI seems to default to NULL for
dbExecuteanddbGetQuery, which meanshttps://dbi.r-dbi.org/reference/dbexecute#specification-for-the-immediate-argument-1
So setting FALSE, will change behavior for all the DB that backends that will use TRUE by default.
Maybe we should stick to that, and pass
immediate = TRUEorFALSEwhen the chunk option is explicitly set ?