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 dbSendManip() generic #132

Merged
merged 12 commits into from Aug 10, 2016
Merged

Add dbSendManip() generic #132

merged 12 commits into from Aug 10, 2016

Conversation

krlmlr
Copy link
Member

@krlmlr krlmlr commented Aug 8, 2016

  • Expects a DBIresult as return value.
  • Fix and link documentation of dbSendQuery(), dbGetQuery() and dbExecute().
  • Now called by dbExecute().
  • Implement default dbSendUpdate() that forwards to dbSendQuery().
  • Includes updated revdep check results.

@hannesmuehleisen @imanuelcostigan @s-u: Your packages already define dbSendUpdate(), which is fine, but the new interface expects a DBIresult which can be queried for the number of rows affected, and closed. It will be also used for prepared statements.

I understand that this is a breach of the contract which has been defined and established earlier by your packages. MonetDBLite checks fail because of that, too. Perhaps it would be safest if DBI used a different method name, I'd love to hear your opinion here.

Fixes #20.

CC @bborgesr.

@hannes
Copy link
Contributor

hannes commented Aug 8, 2016

happy to change my implementation to match the new behaviour.

@krlmlr
Copy link
Member Author

krlmlr commented Aug 8, 2016

Just saw that dbSendUpdate() in RJDBC is more than 8 years old. At this point I'd argue that it's not safe anymore to change the contract. I'll rename the function.

What should the new name be, dbSendManip()?

@codecov-io
Copy link

codecov-io commented Aug 8, 2016

Current coverage is 43.70% (diff: 0.00%)

Merging #132 into master will decrease coverage by 0.10%

@@             master       #132   diff @@
==========================================
  Files            16         16          
  Lines           436        437     +1   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits            191        191          
- Misses          245        246     +1   
  Partials          0          0          

Powered by Codecov. Last update 0bff38e...6727036

@krlmlr krlmlr changed the title Add dbSendUpdate() generic Add dbSendManip() generic Aug 8, 2016
@imanuelcostigan
Copy link

@krlmlr I'll take a look over next week and revert with comments if that's ok?

@krlmlr
Copy link
Member Author

krlmlr commented Aug 8, 2016

@imanuelcostigan: Thanks. I think I'm not going to define dbSendUpdate() but a differently named function, current candidate dbSendManip(). The difference is the return value, dbSendManip() needs a DBIresult. I'm happy to take naming suggestions.

@imanuelcostigan
Copy link

Concern I have is that you are expecting dbSendManip() to return a DBIResult (not DBIresult I assume). I don't think non-query statements return this for my backend - but I need to take a closer look.

ON naming - not convinced with naming of dbSendManip()! I'll see if I can figure out anything better.

@krlmlr
Copy link
Member Author

krlmlr commented Aug 8, 2016

Yes, returning a DBIResult would be a change you'd need to make to your backend to be "fully DBI compatible". RJDBC currently returns NULL in dbSendUpdate(), that's why I'm renaming the function in the first place. Perhaps this even should be changed in RJDBC itself.

@bborgesr
Copy link
Contributor

bborgesr commented Aug 8, 2016

@krlmlr, I'm also a little confused as to why there is a need for this (#20 (comment)) -- dbExecute statements tend to be more atomic than query statements by nature, so I think there's less of a case for the symmetry argument... Is it so bad that dbExecute calls dbSendQuery instead of having a dedicated function for execute statements? It feels a little unnecessary to me, but obviously I was not part of this discussion from the beginning, so I may be completely off base here... I do worry about this being too many too closely related things for users to deal with in one release -- the potential for confusion seems high... Again, apologies if I'm just being nit-picky here...

@krlmlr
Copy link
Member Author

krlmlr commented Aug 8, 2016

@bborgesr: The motivation for dbSendManip() is #20 (comment), especially the first bullet point. Let me rephrase:

For prepared queries, you'd:

  1. call dbSendQuery() with a query that contains placeholders
  2. call dbBind() to assign values to the placeholders
  3. call dbFetch() to retrieve query results
  4. repeat 2 and 3 as needed
  5. call dbClearResult().

I'd like to use the same pattern for prepared data manipulation statements such as INSERT INTO t VALUES (?, ?).

I agree that a monolith dbExecute() may be simpler to implement for some backends. However, for RSQLite, RMySQL and RPostgres, dbSendQuery() and dbSendManip() are currently the same functions -- they work out of the box. All there's to do for MonetDB and RJDBC/RSQLServer (who originally had dbSendUpdate()) is to wrap the number of rows affected in a DBIResult object. I think this isn't too much asked for.

Still open for naming suggestions.

@bborgesr
Copy link
Contributor

bborgesr commented Aug 9, 2016

Ok, I get it a bit better now. You probably thought about this already, but dbSendStatement? or dbSendExec (which would stand for "send executable statement" -- mirroring dbExecute)? This would have the advantage that it is a bit more general than dbSendManip, since data manipulation statements aren't the only thing that's supported here, right? You could also use it to set connection variables and stuff like that, which is not actually manipulating any data...

@krlmlr
Copy link
Member Author

krlmlr commented Aug 9, 2016

I like dbSendStatement(). Yes, it's for all kinds of non-SELECT statements.

@krlmlr krlmlr merged commit 0fefe7c into master Aug 10, 2016
@krlmlr krlmlr deleted the f-#20-db-send-update branch August 10, 2016 22:11
krlmlr added a commit that referenced this pull request 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 pull request 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).
@github-actions github-actions bot locked and limited conversation to collaborators Oct 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add dbSendUpdate or equivalent to DBI
5 participants