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

dbWriteTable not escaping column names in 0.3.1 release #2622

Closed
2 tasks done
lentinj opened this issue Nov 17, 2021 · 4 comments
Closed
2 tasks done

dbWriteTable not escaping column names in 0.3.1 release #2622

lentinj opened this issue Nov 17, 2021 · 4 comments

Comments

@lentinj
Copy link
Contributor

lentinj commented Nov 17, 2021

What happens?

Version 0.3.1, using dbWriteTable() with a name column in field.types specified results in invalid SQL being generated.

This has been picked up by CRAN package checks on the downstream MFDB package: https://cran.r-project.org/web/checks/check_results_mfdb.html

To Reproduce

> db <- DBI::dbConnect(duckdb::duckdb())
> DBI::dbWriteTable(db, basename(tempfile("temp_")),
+     data.frame(id = 1:3, name = c("cuthbert", "dibble", "grubb")),
+     field.types = c(id = "INTEGER", name = "VARCHAR"))
Error in .local(conn, statement, ...) :
  duckdb_prepare_R: Failed to prepare query CREATE  TABLE "temp_226be6a3dc5d1" AS SELECT #1::INTEGER id,#2::VARCHAR name FROM _duckdb_write_view_vcxzjonvgn
Error: Parser Error: syntax error at or near "name"
LINE 1: ..." AS SELECT #1::INTEGER id,#2::VARCHAR name FROM _duckdb_write_view_vcxzjonvgn
                                                  ^
Environment:
  1: DBI::dbWriteTable(db, basename(tempfile("temp_")), data.frame(id = 1:3, name = c("cuthbert", "dibble", "grubb")), field.types = c(id = "INTEGER",
  2: DBI::dbWriteTable(db, basename(tempfile("temp_")), data.frame(id = 1:3, name = c("cuthbert", "dibble", "grubb")), field.types = c(id = "INTEGER",
  3: .local(conn, name, value, ...)
  4: dbExecute(conn, SQL(sprintf("CREATE %s TABLE %s AS SELECT %s FROM %s", temp_str, table_name, paste(cols, collapse = ","), view_name)))
  5: dbExecute(conn, SQL(sprintf("CREATE %s TABLE %s AS SELECT %s FROM %s", temp_str, table_name, paste(cols, collapse = ","), view_name)))
  6: dbSendStatement(conn, statement, ...)
  7: dbSendStatement(conn, statement, ...)
  8: dbSendQuery(conn, statement, ...)
  9: dbSendQuery(conn, statement, ...)
  10: .local(conn, statement, ...)

The command-line thinks this is invalid SQL too (and is fair enough IMO, name is probably a reserved word):

v0.3.1 88aa81c6b
Enter ".help" for usage hints.
Connected to a transient in-memory database.
Use ".open FILENAME" to reopen on a persistent database.
D CREATE TABLE moo (moo_id INTEGER, name VARCHAR);
D SELECT #1::INTEGER, #2::VARCHAR name FROM moo;
Error: Parser Error: syntax error at or near "name"
LINE 1: SELECT #1::INTEGER, #2::VARCHAR name FROM moo;

If you ask me the bug is here, the SQL generation isn't escaping the column name:

cols <- c(cols, sprintf("#%d::%s %s", col_idx, field.types[name], name))

this was added in this commit, part of 0.3.1 8aed295. I can make a pull request to add some escaping here if that's useful?

Cheers!

Environment (please complete the following information):

  • OS: Debian GNU/Linux 10 (buster)
  • DuckDB Version: 0.3.2
  • DuckDB Client: R version 4.1.1 (2021-08-10)

Before Submitting

  • Have you tried this on the latest master branch?
  • R: install.packages("https://github.com/duckdb/duckdb/releases/download/master-builds/duckdb_r_src.tar.gz", repos = NULL)
  • Have you tried the steps to reproduce? Do they include all relevant data and configuration? Does the issue you report still appear there?
@Mytherin
Copy link
Collaborator

Thanks for the report!

Indeed it seems like the column name needs to be escaped.

I can make a pull request to add some escaping here if that's useful?

That would be fantastic!

@l1t1

This comment was marked as abuse.

@mskyttner
Copy link

This may be where the reserved column name keywords are listed: https://github.com/duckdb/duckdb/tree/master/third_party/libpg_query/grammar/keywords (more details in #989)

@hannes
Copy link
Member

hannes commented Nov 18, 2021

Sorry for that, we're missing a call todbQuoteIdentifier here. Doing this based on whether something is reserved or not is not a good idea.

hannes added a commit to hannes/duckdb that referenced this issue Nov 18, 2021
@hannes hannes linked a pull request Nov 18, 2021 that will close this issue
@lentinj lentinj changed the title dbWriteTable broken in 0.3.1 release dbWriteTable not escaping column names in 0.3.1 release Nov 18, 2021
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 a pull request may close this issue.

5 participants