r-dbi / RSQLite

R interface for SQLite
https://rsqlite.r-dbi.org
GNU Lesser General Public License v2.1
328 stars 78 forks source link

dbWriteTable Inconsistently accepts DBI:ID #457

Open kmishra9 opened 1 year ago

kmishra9 commented 1 year ago

While doing some bug hunting for RPostgres, realized there's also probably a bug in RSQLite when writing from a CSV:

# ******************************************************************************
# 1. Set Up ####
# ******************************************************************************

suppressPackageStartupMessages(library(dplyr))
suppressPackageStartupMessages(library(dbplyr))

db1 <- DBI::dbConnect(RSQLite::SQLite(), dbname = ":memory:")
db2 <- DBI::dbConnect(RSQLite::SQLite(), dbname = ":memory:")

test_df_full <- nycflights13::flights
test_df_lite <- nycflights13::flights %>% slice(1:100)

temp_path_full <- tempfile()
temp_path_lite <- tempfile()

db_path_full_sq <- DBI::Id(schema = NULL, table = 'test_df_full')
db_path_lite_sq <- DBI::Id(schema = NULL, table = 'test_df_lite')

readr::write_csv(x = test_df_full, file = temp_path_full)
readr::write_csv(x = test_df_lite, file = temp_path_lite)

# ******************************************************************************
# 2. Testing DBI:Id with Local DF ####
# ******************************************************************************

# Both work in SQLite and Microsoft SQL though
DBI::dbWriteTable(conn = db1, name = db_path_full_sq, value = test_df_full, overwrite = T)
DBI::dbWriteTable(conn = db1, name = db_path_lite_sq, value = test_df_lite, overwrite = T)

db1 %>% tbl(db_path_full_sq) %>% tally() # Expected: n = 336,776; Actual: 336,776
#> # Source:   SQL [1 x 1]
#> # Database: sqlite 3.40.1 [:memory:]
#>        n
#>    <int>
#> 1 336776
db1 %>% tbl(db_path_lite_sq) %>% tally() # Expected: n = 100; Actual: n = 100
#> # Source:   SQL [1 x 1]
#> # Database: sqlite 3.40.1 [:memory:]
#>       n
#>   <int>
#> 1   100

# ******************************************************************************
# 3. Testing DBI:Id with CSV ####
# ******************************************************************************

# Fails (Shouldn't)
DBI::dbWriteTable(conn = db2, name = db_path_full_sq, value = temp_path_full, overwrite = T)
#> Error in connection_import_file(conn@ptr, name, value, sep, eol, skip): RS_sqlite_import: no such table: `test_df_full`
DBI::dbWriteTable(conn = db2, name = db_path_lite_sq , value = temp_path_lite, overwrite = T)
#> Error in connection_import_file(conn@ptr, name, value, sep, eol, skip): RS_sqlite_import: no such table: `test_df_lite`

db2 %>% tbl(db_path_full_sq) %>% tally() # Expected: n = 336,776; Actual: Error
#> Error in `db_query_fields.DBIConnection()`:
#> ! Can't query fields.
#> Caused by error:
#> ! no such table: test_df_full
#> Backtrace:
#>      ▆
#>   1. ├─db2 %>% tbl(db_path_full_sq) %>% tally()
#>   2. ├─dplyr::tally(.)
#>   3. ├─dplyr::tbl(., db_path_full_sq)
#>   4. └─dplyr:::tbl.DBIConnection(., db_path_full_sq)
#>   5.   ├─dplyr::tbl(...)
#>   6.   └─dbplyr:::tbl.src_dbi(...)
#>   7.     └─dbplyr::tbl_sql(c(subclass, "dbi"), src = src, from = from, ...)
#>   8.       ├─vars %||% dbplyr_query_fields(src$con, from_sql)
#>   9.       └─dbplyr:::dbplyr_query_fields(src$con, from_sql)
#>  10.         └─dbplyr:::dbplyr_fallback(con, "db_query_fields", ...)
#>  11.           ├─rlang::eval_bare(expr((!!fun)(con, ...)))
#>  12.           └─dbplyr:::db_query_fields.DBIConnection(con, ...)
#>  13.             └─base::tryCatch(...)
#>  14.               └─base (local) tryCatchList(expr, classes, parentenv, handlers)
#>  15.                 └─base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
#>  16.                   └─value[[3L]](cond)
#>  17.                     └─cli::cli_abort("Can't query fields.", parent = cnd)
#>  18.                       └─rlang::abort(...)
db2 %>% tbl(db_path_lite_sq) %>% tally() # Expected: n = 100; Actual: Error
#> Error in `db_query_fields.DBIConnection()`:
#> ! Can't query fields.
#> Caused by error:
#> ! no such table: test_df_lite
#> Backtrace:
#>      ▆
#>   1. ├─db2 %>% tbl(db_path_lite_sq) %>% tally()
#>   2. ├─dplyr::tally(.)
#>   3. ├─dplyr::tbl(., db_path_lite_sq)
#>   4. └─dplyr:::tbl.DBIConnection(., db_path_lite_sq)
#>   5.   ├─dplyr::tbl(...)
#>   6.   └─dbplyr:::tbl.src_dbi(...)
#>   7.     └─dbplyr::tbl_sql(c(subclass, "dbi"), src = src, from = from, ...)
#>   8.       ├─vars %||% dbplyr_query_fields(src$con, from_sql)
#>   9.       └─dbplyr:::dbplyr_query_fields(src$con, from_sql)
#>  10.         └─dbplyr:::dbplyr_fallback(con, "db_query_fields", ...)
#>  11.           ├─rlang::eval_bare(expr((!!fun)(con, ...)))
#>  12.           └─dbplyr:::db_query_fields.DBIConnection(con, ...)
#>  13.             └─base::tryCatch(...)
#>  14.               └─base (local) tryCatchList(expr, classes, parentenv, handlers)
#>  15.                 └─base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
#>  16.                   └─value[[3L]](cond)
#>  17.                     └─cli::cli_abort("Can't query fields.", parent = cnd)
#>  18.                       └─rlang::abort(...)

# Works
DBI::dbWriteTable(conn = db2, name = 'test_df_full', value = temp_path_full, overwrite = T)
DBI::dbWriteTable(conn = db2, name = 'test_df_lite', value = temp_path_lite, overwrite = T)

db2 %>% tbl(db_path_full_sq) %>% tally() # Expected: n = 336,776; Actual: 336,776
#> # Source:   SQL [1 x 1]
#> # Database: sqlite 3.40.1 [:memory:]
#>        n
#>    <int>
#> 1 336776
db2 %>% tbl(db_path_lite_sq) %>% tally() # Expected: n = 100; Actual: n = 100
#> # Source:   SQL [1 x 1]
#> # Database: sqlite 3.40.1 [:memory:]
#>       n
#>   <int>
#> 1   100

Created on 2023-03-30 with reprex v2.0.2

krlmlr commented 1 year ago

Thanks, good catch.

It seems that the RS_sqlite_import() C function uses the %q format specifier to construct SQL with the table name quoted. We could change that to use %s in that function and call dbQuoteIdentifier() before entering RS_sqlite_import() .