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

Simplify escaping strategy #1396

Merged
merged 55 commits into from
Feb 20, 2024
Merged
Show file tree
Hide file tree
Changes from 45 commits
Commits
Show all changes
55 commits
Select commit Hold shift + click to select a range
8105d7b
First stab
hadley Nov 7, 2023
fb1e38a
Merge commit '388a6eef0e634efa693d809061ceac91d3b69a77'
hadley Nov 8, 2023
d03a206
Fixing alias escaping
hadley Nov 8, 2023
054ee2b
Fix argument name
hadley Nov 8, 2023
892b23e
Correct escaping in sql_star()
hadley Nov 8, 2023
f7a65bc
Extract table name from id string
hadley Nov 8, 2023
afa58e9
More getting stuff to work
hadley Nov 8, 2023
f370e12
Restore missing con argument
hadley Nov 28, 2023
6ca54e3
Making progress on joins
hadley Nov 28, 2023
3365fe8
Add c method
hadley Nov 28, 2023
c520833
x_as and y_as are now already escaped
hadley Nov 28, 2023
9c5ac01
Escape LHS & RHS names
hadley Nov 28, 2023
ffd27e8
Generate aliases based on table names
hadley Nov 28, 2023
381a7ed
Misc fixes
hadley Nov 28, 2023
d8b66f3
Ensure unique_query_name() generates esacped name
hadley Nov 28, 2023
2b2c3d2
Escaping now happens earlier
hadley Nov 28, 2023
8ec7670
Update postgres setup information
hadley Nov 28, 2023
68c799c
Remove old ident code
hadley Nov 28, 2023
6889d45
Start testing table_name functions
hadley Nov 28, 2023
73d9628
Merged origin/main into simplify-escaping
hadley Nov 28, 2023
90bc9a1
Rename constructor
hadley Nov 28, 2023
fa83a18
Reorganise table-name file
hadley Nov 29, 2023
8ae7399
Fix R CMD check issues
hadley Nov 29, 2023
0048df6
Move as and is functions closer & test together
hadley Nov 29, 2023
2ef6fba
More explaining & re-organising
hadley Nov 29, 2023
07aee8e
Make as_table_name() stricter
hadley Nov 29, 2023
90ee786
Test more methods; fix test
hadley Nov 29, 2023
0baaf3c
Update old function
hadley Nov 30, 2023
fbc23aa
Tweak argument checking to hopefully fix problems on older R
hadley Nov 30, 2023
a749f3c
Simplify code; keep question
hadley Nov 30, 2023
2252c79
WS
hadley Nov 30, 2023
0ebc70f
Update temporary table prefix
hadley Dec 1, 2023
5024363
Add table name check
hadley Dec 1, 2023
12f682f
Implement check_table_name()
hadley Dec 1, 2023
bff0130
unique_subquery_name() no longer needs con
hadley Dec 1, 2023
e7bef90
Coerce rather than checking
hadley Dec 18, 2023
ac051f7
RPostgreSQL needs unquoted table name
hadley Dec 18, 2023
b8e9b62
Disable windows R3.6 checking for RMariaDB
hadley Dec 18, 2023
7ddea50
Use correct function name
hadley Dec 18, 2023
86aeea5
Refine table_name_table() interface
hadley Dec 18, 2023
af52e5e
Teach S4 that dbplyr_table_name inherits from character
hadley Dec 18, 2023
fd5eaec
Move coercion back to methods
hadley Dec 18, 2023
177af83
Merge commit '781d630b3ae059e7e8f2176c7261a48ba17a53d3'
mgirlich Jan 22, 2024
dd2d0c3
Use dev odbc for testing MS SQL
mgirlich Jan 22, 2024
2e83a36
Remove remote odbc and require >= 1.4.2 instead
mgirlich Jan 31, 2024
6c3c2a6
Update R/lazy-join-query.R
hadley Feb 14, 2024
091b04f
sql_table_name_prefix() improvements
hadley Feb 14, 2024
b85ca5d
Make `c()` safer
hadley Feb 14, 2024
a434eb0
Handle unquoting within in_schema/in_catalog
hadley Feb 15, 2024
09e78e7
Uncomment accidentally commented tests
hadley Feb 15, 2024
2cc65f3
Qualify is
hadley Feb 15, 2024
c28dd74
Add bullet about index name
hadley Feb 15, 2024
c88c355
Rename table_name to table_path
hadley Feb 15, 2024
27a789f
Add news bullets
hadley Feb 15, 2024
261b3d6
Merged origin/main into simplify-escaping
hadley Feb 15, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions DESCRIPTION
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ Suggests:
knitr,
Lahman,
nycflights13,
odbc,
odbc (>= 1.4.2),
RMariaDB (>= 1.2.2),
rmarkdown,
RPostgres (>= 1.4.5),
Expand Down Expand Up @@ -127,7 +127,7 @@ Collate:
'sql-expr.R'
'src-sql.R'
'src_dbi.R'
'table-ident.R'
'table-name.R'
'tbl-lazy.R'
'tbl-sql.R'
'test-frame.R'
Expand Down
15 changes: 5 additions & 10 deletions NAMESPACE
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# Generated by roxygen2: do not edit by hand

S3method("$",tbl_lazy)
S3method("[",dbplyr_table_name)
S3method("[[",dbplyr_table_name)
S3method(add_count,tbl_lazy)
S3method(anti_join,tbl_lazy)
S3method(arrange,tbl_lazy)
Expand All @@ -12,15 +14,8 @@ S3method(as.sql,dbplyr_catalog)
S3method(as.sql,dbplyr_schema)
S3method(as.sql,ident)
S3method(as.sql,sql)
S3method(as_table_ident,Id)
S3method(as_table_ident,character)
S3method(as_table_ident,dbplyr_catalog)
S3method(as_table_ident,dbplyr_schema)
S3method(as_table_ident,dbplyr_table_ident)
S3method(as_table_ident,ident)
S3method(as_table_ident,ident_q)
S3method(as_table_ident,sql)
S3method(auto_copy,tbl_sql)
S3method(c,dbplyr_table_name)
S3method(c,ident)
S3method(c,sql)
S3method(collapse,tbl_sql)
Expand Down Expand Up @@ -122,7 +117,7 @@ S3method(escape,character)
S3method(escape,data.frame)
S3method(escape,dbplyr_catalog)
S3method(escape,dbplyr_schema)
S3method(escape,dbplyr_table_ident)
S3method(escape,dbplyr_table_name)
S3method(escape,double)
S3method(escape,factor)
S3method(escape,ident)
Expand All @@ -144,7 +139,6 @@ S3method(flatten_query,union_query)
S3method(flatten_query,values_query)
S3method(format,dbplyr_catalog)
S3method(format,dbplyr_schema)
S3method(format,dbplyr_table_ident)
S3method(format,ident)
S3method(format,sql)
S3method(format,src_sql)
Expand Down Expand Up @@ -181,6 +175,7 @@ S3method(print,base_query)
S3method(print,dbplyr_catalog)
S3method(print,dbplyr_schema)
S3method(print,dbplyr_sql_options)
S3method(print,dbplyr_table_name)
S3method(print,ident)
S3method(print,join_query)
S3method(print,lazy_base_local_query)
Expand Down
6 changes: 1 addition & 5 deletions R/backend-hana.R
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,8 @@ sql_translation.HDB <- function(con) {
# nocov start
#' @export
db_table_temporary.HDB <- function(con, table, temporary, ...) {
if (temporary && substr(table, 1, 1) != "#") {
table <- hash_temp(table)
}

list(
table = table,
table = add_temporary_prefix(con, table, temporary = temporary),
temporary = FALSE
)
}
Expand Down
11 changes: 2 additions & 9 deletions R/backend-mssql.R
Original file line number Diff line number Diff line change
Expand Up @@ -510,20 +510,13 @@ mssql_version <- function(con) {
# <https://docs.microsoft.com/en-us/previous-versions/sql/sql-server-2008-r2/ms177399%28v%3dsql.105%29#temporary-tables>
#' @export
`db_table_temporary.Microsoft SQL Server` <- function(con, table, temporary, ...) {
table <- as_table_ident(table)
table_name <- vctrs::field(table, "table")

if (temporary && substr(table_name, 1, 1) != "#") {
table_name <- hash_temp(table_name)
vctrs::field(table, "table") <- table_name
}

list(
table = table,
table = add_temporary_prefix(con, table, temporary = temporary),
temporary = FALSE
)
}


#' @export
`sql_query_save.Microsoft SQL Server` <- function(con,
sql,
Expand Down
2 changes: 1 addition & 1 deletion R/backend-mysql.R
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ db_connection_describe.MySQLConnection <- db_connection_describe.MariaDBConnecti

#' @export
db_col_types.MariaDBConnection <- function(con, table, call) {
table <- as_table_ident(table, error_call = call)
table <- as_table_name(table, con, error_call = call)
col_info_df <- DBI::dbGetQuery(con, glue_sql2(con, "SHOW COLUMNS FROM {.tbl table};"))
set_names(col_info_df[["Type"]], col_info_df[["Field"]])
}
Expand Down
3 changes: 1 addition & 2 deletions R/backend-postgres-old.R
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ db_write_table.PostgreSQLConnection <- function(con,
values,
temporary = TRUE,
...) {
table <- as_table_ident(table)
if (!isFALSE(temporary)) {
cli_abort(c(
"RPostgreSQL backend does not support creation of temporary tables",
Expand All @@ -27,7 +26,7 @@ db_write_table.PostgreSQLConnection <- function(con,
# the bare table name
dbWriteTable(
con,
name = vctrs::field(table, "table"),
name = table_name_table(table, con),
value = values,
field.types = types,
...,
Expand Down
2 changes: 1 addition & 1 deletion R/backend-postgres.R
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,7 @@ db_supports_table_alias_with_as.PostgreSQL <- function(con) {

#' @export
db_col_types.PqConnection <- function(con, table, call) {
table <- as_table_ident(table, error_call = call)
table <- as_table_name(table, con, error_call = call)
res <- DBI::dbSendQuery(con, glue_sql2(con, "SELECT * FROM {.tbl table} LIMIT 0"))
on.exit(DBI::dbClearResult(res))
DBI::dbFetch(res, n = 0)
Expand Down
1 change: 0 additions & 1 deletion R/backend-spark-sql.R
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,6 @@ simulate_spark_sql <- function() simulate_dbi("Spark SQL")
cli::cli_abort("Spark SQL only support temporary tables")
}

table <- as_table_ident(table)
sql <- glue_sql2(
con,
"CREATE ", if (overwrite) "OR REPLACE ",
Expand Down
4 changes: 2 additions & 2 deletions R/build-sql.R
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,9 @@ sql_quote_transformer <- function(connection) {
glue_check_collapse(type, should_collapse)

if (type == "tbl") {
value <- as_table_ident(value)
value <- as_table_name(value, connection)
} else if (type == "from") {
value <- as_from(value)
value <- as_table_source(value, connection)
} else if (type == "col") {
if (is_bare_character(value)) {
value <- ident(value)
Expand Down
13 changes: 7 additions & 6 deletions R/db-io.R
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ db_copy_to <- function(con,
indexes = NULL,
analyze = TRUE,
in_transaction = TRUE) {
as_table_ident(table)
check_table_id(table)
check_bool(overwrite)
check_character(types, allow_null = TRUE)
check_named(types)
Expand All @@ -65,10 +65,11 @@ db_copy_to.DBIConnection <- function(con,
indexes = NULL,
analyze = TRUE,
in_transaction = TRUE) {
table <- as_table_ident(table)
table <- as_table_name(table, con)
new <- db_table_temporary(con, table, temporary)
table <- new$table
temporary <- new$temporary
call <- current_env()

with_transaction(
con,
Expand Down Expand Up @@ -103,7 +104,7 @@ db_compute <- function(con,
indexes = list(),
analyze = TRUE,
in_transaction = TRUE) {
as_table_ident(table)
check_table_id(table)
check_scalar_sql(sql)
check_bool(overwrite)
check_bool(temporary)
Expand All @@ -123,7 +124,7 @@ db_compute.DBIConnection <- function(con,
indexes = list(),
analyze = TRUE,
in_transaction = FALSE) {
table <- as_table_ident(table)
table <- as_table_name(table, con)
new <- db_table_temporary(con, table, temporary)
table <- new$table
temporary <- new$temporary
Expand Down Expand Up @@ -179,7 +180,7 @@ db_write_table.DBIConnection <- function(con,
temporary = TRUE,
...,
overwrite = FALSE) {
table <- as_table_ident(table)
check_table_name(table)
check_character(types, allow_null = TRUE)
check_named(types)
check_bool(temporary)
Expand All @@ -188,7 +189,7 @@ db_write_table.DBIConnection <- function(con,
withCallingHandlers(
dbWriteTable(
con,
name = table_ident_to_id(table),
name = SQL(table),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think table should be converted to a table name first. Otherwise, e.g. db_write_table(con, in_schema("schema1", "table1")) won't work.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't remember what I was thinking when I wrote this code, but coming back to it now, it looks like db_write_table() expects table to already be a table name.

value = values,
field.types = types,
temporary = temporary,
Expand Down
Loading
Loading