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

r/adbcdrivermanager: Improve options handling #1126

Closed
paleolimbot opened this issue Sep 28, 2023 · 1 comment · Fixed by #1214
Closed

r/adbcdrivermanager: Improve options handling #1126

paleolimbot opened this issue Sep 28, 2023 · 1 comment · Fixed by #1214

Comments

@paleolimbot
Copy link
Member

  • TRUE/FALSE should map to "true"/"false"
  • It would be nice to be able to inject a previously prepared set of options to "splat" into the options list
@nbenn
Copy link
Collaborator

nbenn commented Sep 28, 2023

@paleolimbot the reason I brought this up was that currently it is not super straight-forward how these options should be set (with what keys/values). Better documentation would be nice of course, but maybe it is too early for duplicating things that are in flux. Another thing that could be helpful here is "get-option" functionality (#1129). If that tells you list(..., adbc.connection.autocommit = "true") it is obvious how to disable auto-commit on a connection.

paleolimbot added a commit that referenced this issue Oct 26, 2023
This PR implements option setting/getting in the "void" driver and
implements tests for the full grid of set/get by
string/bytes/integer/double by database/connection/statement. The error
detail information was also not implemented in the dummy driver and so
couldn't be tested (so there is an implementation of that here).

Implementing a driver that actually did this was sufficient work that I
did some rather heavy abstraction to make it easier to write. That
abstraction is not unlike a "driver framework" except it is (1) not
complete, since the void driver only needs options methods and (2)
doesn't provide any result helpers (building streams, etc.). It might be
worth porting the driver base to be more general but for now I'd like to
keep it constrained to what I need to test in R.

Closes #1126.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants