Skip to content

Commit

Permalink
refactor(r): Improve testing for ADBC 1.1 features in R bindings (#1214)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
paleolimbot authored Oct 26, 2023
1 parent 1789870 commit ef72e6f
Show file tree
Hide file tree
Showing 15 changed files with 1,089 additions and 501 deletions.
6 changes: 6 additions & 0 deletions c/driver_manager/adbc_driver_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,11 @@ AdbcStatusCode StatementBind(struct AdbcStatement*, struct ArrowArray*,
return ADBC_STATUS_NOT_IMPLEMENTED;
}

AdbcStatusCode StatementBindStream(struct AdbcStatement*, struct ArrowArrayStream*,
struct AdbcError* error) {
return ADBC_STATUS_NOT_IMPLEMENTED;
}

AdbcStatusCode StatementCancel(struct AdbcStatement* statement, struct AdbcError* error) {
return ADBC_STATUS_NOT_IMPLEMENTED;
}
Expand Down Expand Up @@ -1666,6 +1671,7 @@ AdbcStatusCode AdbcLoadDriverFromInitFunc(AdbcDriverInitFunc init_func, int vers
CHECK_REQUIRED(driver, StatementNew);
CHECK_REQUIRED(driver, StatementRelease);
FILL_DEFAULT(driver, StatementBind);
FILL_DEFAULT(driver, StatementBindStream);
FILL_DEFAULT(driver, StatementGetParameterSchema);
FILL_DEFAULT(driver, StatementPrepare);
FILL_DEFAULT(driver, StatementSetOption);
Expand Down
6 changes: 6 additions & 0 deletions go/adbc/drivermgr/adbc_driver_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,11 @@ AdbcStatusCode StatementBind(struct AdbcStatement*, struct ArrowArray*,
return ADBC_STATUS_NOT_IMPLEMENTED;
}

AdbcStatusCode StatementBindStream(struct AdbcStatement*, struct ArrowArrayStream*,
struct AdbcError* error) {
return ADBC_STATUS_NOT_IMPLEMENTED;
}

AdbcStatusCode StatementCancel(struct AdbcStatement* statement, struct AdbcError* error) {
return ADBC_STATUS_NOT_IMPLEMENTED;
}
Expand Down Expand Up @@ -1666,6 +1671,7 @@ AdbcStatusCode AdbcLoadDriverFromInitFunc(AdbcDriverInitFunc init_func, int vers
CHECK_REQUIRED(driver, StatementNew);
CHECK_REQUIRED(driver, StatementRelease);
FILL_DEFAULT(driver, StatementBind);
FILL_DEFAULT(driver, StatementBindStream);
FILL_DEFAULT(driver, StatementGetParameterSchema);
FILL_DEFAULT(driver, StatementPrepare);
FILL_DEFAULT(driver, StatementSetOption);
Expand Down
8 changes: 6 additions & 2 deletions r/adbcdrivermanager/R/error.R
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,12 @@ adbc_error_from_array_stream <- function(stream) {
.Call(RAdbcErrorFromArrayStream, stream)
}

adbc_allocate_error <- function(shelter = NULL) {
.Call(RAdbcAllocateError, shelter)
adbc_allocate_error <- function(shelter = NULL, use_legacy_error = NULL) {
if (is.null(use_legacy_error)) {
use_legacy_error <- getOption("adbcdrivermanager.use_legacy_error", FALSE)
}

.Call(RAdbcAllocateError, shelter, use_legacy_error)
}

stop_for_error <- function(status, error) {
Expand Down
74 changes: 71 additions & 3 deletions r/adbcdrivermanager/R/options.R
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ adbc_database_set_options <- function(database, options) {
error <- adbc_allocate_error()
for (i in seq_along(options)) {
key <- names(options)[i]
value <- options[i]
value <- options[[i]]
status <- .Call(
RAdbcDatabaseSetOption,
database,
Expand All @@ -42,7 +42,7 @@ adbc_connection_set_options <- function(connection, options) {
error <- adbc_allocate_error()
for (i in seq_along(options)) {
key <- names(options)[i]
value <- options[i]
value <- options[[i]]
status <- .Call(
RAdbcConnectionSetOption,
connection,
Expand All @@ -62,7 +62,7 @@ adbc_statement_set_options <- function(statement, options) {
error <- adbc_allocate_error()
for (i in seq_along(options)) {
key <- names(options)[i]
value <- options[i]
value <- options[[i]]
status <- .Call(
RAdbcStatementSetOption,
statement,
Expand Down Expand Up @@ -160,3 +160,71 @@ adbc_statement_get_option_double <- function(statement, option) {
error <- adbc_allocate_error()
.Call(RAdbcStatementGetOptionDouble, statement, option, error)
}

# Ensures that options are a list of bare character, raw, integer, or double
key_value_options <- function(options) {
options <- as.list(options)

if (length(options) == 0) {
names(options) <- character()
} else if (is.null(names(options))) {
# OK to have no names, because options could contain a series of
# adbc_options() objects that will be concatenated
names(options) <- rep("", length(options))
}

out <- vector("list", 10L)
out_names <- character(10L)
n_out <- 0L

for (i in seq_along(options)) {
key <- names(options)[[i]]
item <- options[[i]]

# Skip NULL item
if (is.null(item)) {
next
}

# Append all items of an existing adbc_options
if (inherits(item, "adbc_options")) {
out <- c(out[seq_len(n_out)], item)
out_names <- c(out_names[seq_len(n_out)], names(item))
n_out <- n_out + length(item)
next
}

# Otherwise, append a single value (coercing to character if item
# is an S3 object)
if (is.object(item)) {
item <- as.character(item)
}

if (identical(key, "") || identical(key, NA_character_)) {
stop("key/value options must be named")
}

n_out <- n_out + 1L
out_names[n_out] <- key
if (is.character(item)) {
out[[n_out]] <- item
} else if (is.integer(item)) {
out[[n_out]] <- as.integer(item)
} else if (is.double(item)) {
out[[n_out]] <- as.double(item)
} else if (is.raw(item)) {
out[[n_out]] <- item
} else {
stop(
sprintf(
"Option of type '%s' (key: '%s') not supported",
typeof(item),
key
)
)
}
}

names(out) <- out_names
structure(out[seq_len(n_out)], class = "adbc_options")
}
17 changes: 0 additions & 17 deletions r/adbcdrivermanager/R/utils.R
Original file line number Diff line number Diff line change
Expand Up @@ -15,23 +15,6 @@
# specific language governing permissions and limitations
# under the License.

key_value_options <- function(options) {
if (!is.character(options)) {
options <- as.list(options)
options <- options[!vapply(options, is.null, logical(1))]
options <- vapply(options, as.character, character(1))
}

keys <- names(options)
if (length(options) == 0) {
names(options) <- character()
} else if (is.null(keys) || all(keys == "")) {
stop("key/value options must be named")
}

options
}

new_env <- function() {
new.env(parent = emptyenv())
}
Expand Down
Loading

0 comments on commit ef72e6f

Please sign in to comment.