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.

Sign up for GitHub

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

Jump to bottom

dbAppendTable: do not check for existence twice #691

Merged
merged 12 commits into from
Jan 20, 2024
Merged

dbAppendTable: do not check for existence twice #691

merged 12 commits into from
Jan 20, 2024

Conversation

detule
Copy link
Collaborator

@detule detule commented Dec 19, 2023

Hi -

This is a performance enhancement when using dbAppendTable.

We have seen recently that calls to dbExistsTable can be costly. Avoid calling it twice when using the dbAppendTable API.

@detule
Copy link
Collaborator Author

detule commented Dec 19, 2023

Not in love with the argument name or style.

Feedback welcome!

@hadley
Copy link
Member

hadley commented Dec 19, 2023

What if we just remove stopifnot(dbExistsTable(conn, name)) from dbAppendTable()? If the user calls it incorrectly on a table that doesn't exist, they'll now get an error from the database rather than R, but I think that's fine. We might also be able to remove the explicit dbExistsTable() check in odbc_write_table() in a similar way, but I think that would require a bit more analysis. (And maybe a little wrapping with withCallingHandlers() to ensure the user is adequately informed about what's happening).

We might instead (or also?) consider moving the append part of odbc_write_table() into dbAppendTable() so that dbWriteTable() becomes mostly a wrapper around dbExists()/dbRemoveTable() + dbCreateTable() + dbAppendTable().

@simonpcouch
Copy link
Collaborator

Possibly related to #480. Will wait to review pending discussion of ^^^!

@hadley
Copy link
Member

hadley commented Dec 19, 2023

@simonpcouch ah yes, good catch!

@detule
Copy link
Collaborator Author

detule commented Dec 20, 2023

Hey Hadley, Simon:

I like the idea of letting the DB report the error - made the change, thanks.
I vote to tackle the dbWrite->dbAppend, rather than dbAppend->dbWrite task / refactor in a separate ticket - though happy to also tackle here.

hadley
hadley approved these changes Dec 20, 2023
View reviewed changes
Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

I think it's fine to do the bigger change in separate PR, but you might want to note that this one fixes #480, and if you don't start work on the next change right away, make an issue so we don't forget about it.

simonpcouch
simonpcouch approved these changes Dec 20, 2023
View reviewed changes
Copy link
Collaborator

@simonpcouch simonpcouch left a comment

Choose a reason for hiding this comment

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

I'm on board!🙂

@hadley
Copy link
Member

hadley commented Dec 20, 2023

I'll merge this just so it's in the next release.

@hadley
Copy link
Member

hadley commented Dec 20, 2023

Looks like this breaks some tests so I'll hold off on until after this release.

@detule
Copy link
Collaborator Author

detule commented Dec 22, 2023

Sorry about that - figured out why I went with the new argument to odbc_write_table to begin with - it's because dbWriteTable, per the DBI spec, should create the table on-the-fly if it does not exist ( unlike dbAppendTable where the expectation is that it fails ).

Given that the release is out the door, i might spend a bit of time to try and fit this within the dbAppendTable/dbWriteTable refactor we discussed.

@hadley
Copy link
Member

hadley commented Dec 22, 2023

No problems; hopefully the fix will fall in a fairly straightforward way once dbWriteTable() is implemented in terms of dbCreateTable() and dbAppendTable().

@detule
dbi-table: refactor dbAppend/dbWriteTable
6672d73 
* dbWriteTable calls dbAppendTable
* dbAppendTable does not check for table existence / back-end can report
error

Thanks @krlmlr
@detule
documentation: catch-up
cbefc97
@detule
Copy link
Collaborator Author

detule commented Dec 26, 2023

Thanks for the patience / gave it a shot. Let me know if anything looks suspect.
Will re-run pipeline once upstream issues have been resolved.

hadley
hadley approved these changes Jan 9, 2024
View reviewed changes
R/dbi-table.R Outdated Show resolved Hide resolved
R/dbi-table.R Outdated Show resolved Hide resolved
@hadley hadley added this to the v1.4.2 milestone Jan 18, 2024
hadley
hadley reviewed Jan 18, 2024
View reviewed changes
R/dbi-table.R
@@ -36,10 +36,11 @@ odbc_write_table <- function(conn,
overwrite = FALSE,
append = FALSE,
temporary = FALSE,
row.names = NA,
row.names = NULL,
Copy link
Member

Choose a reason for hiding this comment

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

I think this will fix #737

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just pulled and confirmed it does. :)

@simonpcouch
Copy link
Collaborator

Closes #737.

@detule detule merged commit e2b2eaa into r-dbi:main Jan 20, 2024
15 of 16 checks passed
hadley
hadley reviewed Jan 22, 2024
View reviewed changes
NEWS.md
@@ -35,9 +37,6 @@
* SQL Server: correctly enumerate schemas across databases in connections pane
(@detule, #527).

* SQL Server: now uses column type `"BIGINT"` integer64 objects
Copy link
Member

Choose a reason for hiding this comment

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

@detule I think you deleted this accidentally?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ugh - sorry about that! Will fix tonight, unless someone beats me to it.

Copy link
Member

Choose a reason for hiding this comment

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

Let me do it since I'm polishing news for release.

@hadley hadley mentioned this pull request Jan 22, 2024
Closed
hadley added a commit that referenced this pull request Jan 22, 2024
@hadley
Polish news
8d2f038 
Add news bullet accidentally dropped in #691
@simonpcouch simonpcouch mentioned this pull request Jun 6, 2024
Closed
@simonpcouch simonpcouch mentioned this pull request Aug 27, 2024
Closed
krlmlr
krlmlr reviewed Oct 19, 2024
View reviewed changes
Copy link
Member

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks, great! Do we still need the custom dbWriteTable() implementation?

R/dbi-table.R
Comment on lines +156 to +158
if (batch_rows == 0) {
batch_rows <- 1
}
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how this condition can ever be TRUE, with the check in line 137 in this PR.

R/dbi-table.R
return(NULL)
})

if (nrow(value) > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps worth rephrasing as

out <- invisible(NA_real_)

if (nrow(value) == 0) {
  return(out)
}

to avoid the indent?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonpcouch simonpcouch simonpcouch approved these changes

@krlmlr krlmlr krlmlr left review comments

@hadley hadley hadley approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.4.2
Development

Successfully merging this pull request may close these issues.
4 participants
@detule @hadley @simonpcouch @krlmlr