Use dbWriteTable Copy in dbAppendTable to Eliminate Slowdown #286
Conversation
…TRUE and copy = TRUE
R/tables.R
Outdated
| connection_copy_data(conn@ptr, sql, value) | ||
| } |
There was a problem hiding this comment.
Need to return a value here. Test with DBItest::test_some("append_table_return"). Also, factors should give a warning: DBItest::test_some("append_roundtrip_factor")
There was a problem hiding this comment.
@krlmlr I have it now returning nrow(values) and a warning if factors are supplied. Let me know if there is anything else I need to do!
Co-authored-by: Kirill Müller <krlmlr@users.noreply.github.com>
|
@krlmlr Approved suggestion and committed to branch! |
Co-authored-by: Kirill Müller <krlmlr@users.noreply.github.com>
|
Commit is done! |
|
Thanks! I think I should have noticed earlier. Would you like to include this in this PR? If not, no worries, I can take this on. Also, |
|
@krlmlr Could you review the changes I made in the most recent commit and let me know if that is the way you want those things addressed? The only thing I think I didn't add was the addition of using |
|
Thanks. Could you please run |
|
@krlmlr All checks passed. Had to add a param for warn. I will add the |
|
@krlmlr wait, I just checked and it already looks like |
|
@krlmlr Hey just wanted to make sure you had seen the above message asking for clarification! |
| @@ -188,20 +174,33 @@ format_keep_na <- function(x, ...) { | |||
| #' @rdname postgres-tables | |||
| #' @export | |||
| setMethod("dbAppendTable", c("PqConnection"), | |||
| function(conn, name, value, ..., row.names = NULL) { | |||
| function(conn, name, value, copy = TRUE, warn = TRUE, ..., row.names = NULL) { | |||
There was a problem hiding this comment.
We don't need a warn argument here, dbAppendTable() is a new generic.
| function(conn, name, value, copy = TRUE, warn = TRUE, ..., row.names = NULL) { | |
| function(conn, name, value, ..., copy = TRUE, row.names = NULL) { |
| ) | ||
| row.names <- FALSE | ||
|
|
||
| value = factor_to_string(value, warn = warn) |
There was a problem hiding this comment.
| value = factor_to_string(value, warn = warn) | |
| value = factor_to_string(value, warn = TRUE) |
| if (!copy) { | ||
| value <- sqlData(conn, value, row.names = FALSE) | ||
|
|
||
| sql <- sqlAppendTable(conn, name, value, row.names = FALSE) |
There was a problem hiding this comment.
I wonder if the previous approach via sqlAppendTableTemplate() works with Redshift? Can we implement a dbAppendTable() method for a RedshiftConnection that uses sqlAppendTable() and keep using sqlAppendTableTemplate() in the PqConnection method?
|
|
||
| value <- sqlRownamesToColumn(value, row.names) | ||
|
|
||
| if (!copy) { |
There was a problem hiding this comment.
What happens here if the input has zero rows? Do we have a test for this in DBItest?
|
@JSchoenbachler: We are in the process of relicensing RPostgres under MIT, #312. Are you ok with maintaining your contribution under that license? |
|
@krlmlr yes, that is fine! Sorry I haven't looked at this in a long time. |
|
No worries! I can pick this up in the next few days, feel free to add more commits in the meantime. |
This is a pull request to close #241
The only change is to replace the dbAppendTable function with the relevant pieces of dbWriteTable when append & copy = TRUE.