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

Add dbSendUpdate or equivalent to DBI #20

Closed
macqdh opened this issue Sep 4, 2014 · 12 comments · Fixed by #132
Closed

Add dbSendUpdate or equivalent to DBI #20

macqdh opened this issue Sep 4, 2014 · 12 comments · Fixed by #132
Assignees
Milestone

Comments

@macqdh
Copy link

macqdh commented Sep 4, 2014

Please add a function like RJDBC::dbSendUpdate to DBI so that it will be available for other db access packages such as ROracle. See the R-sig-DB thread starting with https://stat.ethz.ch/pipermail/r-sig-db/2014q3/001443.html for more information and discussion, including alternate name and some implementation details.

@hadley
Copy link
Member

hadley commented Sep 5, 2014

Regardless of the name, I think the implementation will be something like:

dbExecute <- function(con, sql) {
  rs <- dbSendQuery(con, sql)
  on.exit(dbClearResult(rs))

  dbHasCompleted(res)
}

@krlmlr
Copy link
Member

krlmlr commented Nov 11, 2015

The number of affected rows might be interesting here as well.

@hadley
Copy link
Member

hadley commented Nov 11, 2015

Might be better to return dbGetRowsAffected().

@imanuelcostigan
Copy link

More general question: what do you define as a "query"? Here's the SQL wikipedia entry for query:

The most common operation in SQL, the query, makes use of the declarative SELECT statement.

If this is correct, then using dbSendQuery to denote sending any command to the DB is probably misleading. Something like dbExecute or dbSendUpdate might better distinguish the two.

@hadley that implementation will be tricky for JDBC based backends. RJDBC's dbSendQuery method wraps the executeQuery method and expects a ResultSet return value. CREATE or UPDATE commands do not return such a result and therefore must be executed using dbSendUpdate which does not return a ResultSet. It is possible to avoid this approach by overriding RJDBC's dbSendQuery method so that it wraps JDBC's execute rather than executeQuery method, but I'm sure @s-u has a good reason for not doing so.

@krlmlr note also that DBItest assumes you can create, update tables using dbSendQuery in the command_query subtest of the test_result function

@hadley
Copy link
Member

hadley commented May 19, 2016

If it wasn't clear, I meant the code sample to be a default method that could be overridden by specific back ends that need to do something differently.

imanuelcostigan added a commit to imanuelcostigan/RSQLServer that referenced this issue May 21, 2016
@krlmlr
Copy link
Member

krlmlr commented May 24, 2016

Makes sense to me:

  • Define a generic dbExecute() (or dbSendUpdate(), or ...)
  • Default implementation in DBI forwards to dbSendQuery()
  • Backends are free to override the DBI implementation (seems helpful at least for JDBC)
  • Need to use the new function in DBItest, too

Still unclear:

  • Asynchronous execution (#69)?
  • Return value?

@hadley
Copy link
Member

hadley commented May 24, 2016

I think it should block for now, and then when we figure out a good approach for async get/send we can implement it for execute too.

I think that since dbExecute() is called primarily for its side effect, it should return invisible(con)

@krlmlr
Copy link
Member

krlmlr commented Jul 6, 2016

We still need a dbSendUpdate() which is called by dbExecute().

@krlmlr krlmlr modified the milestone: 0.5 Jul 6, 2016
@hadley
Copy link
Member

hadley commented Jul 6, 2016

@krlmlr why?

@krlmlr
Copy link
Member

krlmlr commented Jul 6, 2016

  • Prepared statements like INSERT INTO t VALUES (?, ?) needs an object against which dbBind() can be called
  • dbExecute() contains boilerplate which I'd rather not see duplicated in backends
  • There's a nice symmetry with dbSendQuery/dbGetQuery and dbSendUpdate/dbExecute

I'd even like to define a new class DBIStatus, objects of this class would be returned by dbSendUpdate_Async_(), to make DBIResult a bit lighter weight.

@krlmlr krlmlr modified the milestones: 0.5, 0.6 Aug 6, 2016
@krlmlr krlmlr self-assigned this Aug 7, 2016
krlmlr added a commit that referenced this issue Aug 10, 2016
- Add `dbSendStatement()` generic, which forwards to `dbSendQuery()` by default. `dbExecute()` now calls `dbSendStatement()` (#20, #132).
krlmlr added a commit that referenced this issue Aug 10, 2016
- New `dbBreak()` function which allows aborting from within `dbWithTransaction()` (#115, #133).
- Add `dbSendStatement()` generic, which forwards to `dbSendQuery()` by default. `dbExecute()` now calls `dbSendStatement()` (#20, #132).
krlmlr added a commit that referenced this issue Aug 11, 2016
- Interface changes
    - `dbDataType()` maps `character` values to `"TEXT"` by default (#102).
    - The default implementation of `dbQuoteString()` doesn't call `encodeString()` anymore: Neither SQLite nor Postgres understand e.g. `\n` in a string literal, and all of SQLite, Postgres, and MySQL accept an embedded newline (#121).

- Interface enhancements
    - New `dbSendStatement()` generic, forwards to `dbSendQuery()` by default (#20, #132).
    - New `dbExecute()`, calls `dbSendStatement()` by default (#109, @bborgesr).
    - New `dbWithTransaction()` that calls `dbBegin()` and `dbCommit()`, and `dbRollback()` on failure (#110, @bborgesr).
    - New `dbBreak()` function which allows aborting from within `dbWithTransaction()` (#115, #133).
    - Export `dbFetch()` and `dbQuoteString()` methods.

- Documentation improvements:
    - One example per function (except functions scheduled for deprecation) (#67).
    - Consistent layout and identifier naming.
    - Better documentation of generics by adding links to the class and related generics in the "See also" section under "Other DBI... generics" (#130). S4 documentation is directed to a hidden page to unclutter documentation index (#59).
    - Fix two minor vignette typos (#124, @mdsumner).
    - Add package documentation.
    - Remove misleading parts in `dbConnect()` documentation (#118).
    - Remove misleading link in `dbDataType()` documentation.
    - Remove full stop from documentation titles.
    - New help topic "DBIspec" that contains the full DBI specification (currently work in progress) (#129).
    - HTML documentation generated by `staticdocs` is now uploaded to http://rstats-db.github.io/DBI for each build of the "production" branch (#131).
    - Further minor changes and fixes.

- Internal
    - Use `contains` argument instead of `representation()` to denote base classes (#93).
    - Remove redundant declaration of transaction methods (#110, @bborgesr).
@krlmlr
Copy link
Member

krlmlr commented Nov 26, 2018

@woblak: Can you please take this to the maintainer of RJDBC?

@github-actions
Copy link

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue and link to this old issue if necessary.

@github-actions github-actions bot locked and limited conversation to collaborators Sep 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants