-
Notifications
You must be signed in to change notification settings - Fork 99
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
feat(format): introduce ADBC API revision 1.1.0 #692
Changes from 1 commit
572984e
894e19f
6da6a80
b8bced0
23c66da
30b282e
c839ea9
245f80a
ca5083a
2124d88
0f4dee1
ff919a1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ | |
#include <adbc.h> | ||
|
||
#include <algorithm> | ||
#include <array> | ||
#include <cstring> | ||
#include <string> | ||
#include <unordered_map> | ||
|
@@ -191,6 +192,12 @@ AdbcStatusCode StatementExecutePartitions(struct AdbcStatement* statement, | |
return ADBC_STATUS_NOT_IMPLEMENTED; | ||
} | ||
|
||
AdbcStatusCode StatementExecuteSchema(struct AdbcStatement* statement, | ||
struct ArrowSchema* schema, | ||
struct AdbcError* error) { | ||
return ADBC_STATUS_NOT_IMPLEMENTED; | ||
} | ||
|
||
AdbcStatusCode StatementGetParameterSchema(struct AdbcStatement* statement, | ||
struct ArrowSchema* schema, | ||
struct AdbcError* error) { | ||
|
@@ -540,6 +547,15 @@ AdbcStatusCode AdbcStatementExecuteQuery(struct AdbcStatement* statement, | |
error); | ||
} | ||
|
||
AdbcStatusCode AdbcStatementExecuteSchema(struct AdbcStatement* statement, | ||
struct ArrowSchema* schema, | ||
struct AdbcError* error) { | ||
if (!statement->private_driver) { | ||
return ADBC_STATUS_INVALID_STATE; | ||
} | ||
return statement->private_driver->StatementExecuteSchema(statement, schema, error); | ||
} | ||
|
||
AdbcStatusCode AdbcStatementGetParameterSchema(struct AdbcStatement* statement, | ||
struct ArrowSchema* schema, | ||
struct AdbcError* error) { | ||
|
@@ -640,11 +656,19 @@ AdbcStatusCode AdbcLoadDriver(const char* driver_name, const char* entrypoint, | |
AdbcDriverInitFunc init_func; | ||
std::string error_message; | ||
|
||
if (version != ADBC_VERSION_1_0_0) { | ||
SetError(error, "Only ADBC 1.0.0 is supported"); | ||
return ADBC_STATUS_NOT_IMPLEMENTED; | ||
switch (version) { | ||
case ADBC_VERSION_1_0_0: | ||
case ADBC_VERSION_1_1_0: | ||
break; | ||
default: | ||
SetError(error, "Only ADBC 1.0.0 and 1.1.0 are supported"); | ||
return ADBC_STATUS_NOT_IMPLEMENTED; | ||
Comment on lines
+663
to
+665
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are being stricter than There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added the check there as well. |
||
} | ||
|
||
if (!raw_driver) { | ||
SetError(error, "Must provide non-NULL raw_driver"); | ||
return ADBC_STATUS_INVALID_ARGUMENT; | ||
} | ||
Comment on lines
+668
to
+671
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this check be moved in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I replicated the check there. |
||
auto* driver = reinterpret_cast<struct AdbcDriver*>(raw_driver); | ||
|
||
if (!entrypoint) { | ||
|
@@ -771,6 +795,11 @@ AdbcStatusCode AdbcLoadDriver(const char* driver_name, const char* entrypoint, | |
|
||
AdbcStatusCode AdbcLoadDriverFromInitFunc(AdbcDriverInitFunc init_func, int version, | ||
void* raw_driver, struct AdbcError* error) { | ||
constexpr std::array<int, 2> kSupportedVersions = { | ||
ADBC_VERSION_1_1_0, | ||
ADBC_VERSION_1_0_0, | ||
}; | ||
|
||
#define FILL_DEFAULT(DRIVER, STUB) \ | ||
if (!DRIVER->STUB) { \ | ||
DRIVER->STUB = &STUB; \ | ||
|
@@ -781,12 +810,17 @@ AdbcStatusCode AdbcLoadDriverFromInitFunc(AdbcDriverInitFunc init_func, int vers | |
return ADBC_STATUS_INTERNAL; \ | ||
} | ||
|
||
auto result = init_func(version, raw_driver, error); | ||
AdbcStatusCode result = ADBC_STATUS_NOT_IMPLEMENTED; | ||
for (const int try_version : kSupportedVersions) { | ||
if (try_version > version) continue; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm trying to understand the logic. Does it mean that, if the user passes a future There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps you should explain the intent in a comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, so as mentioned above this should check for a supported version first. I've clarified in the source: the intent is that if they pass 1_1_0, we'll try to pass that to the underlying driver, then try 1_0_0 with the driver. If they pass 1_0_0 we'll only try 1_0_0. |
||
result = init_func(try_version, raw_driver, error); | ||
if (result != ADBC_STATUS_NOT_IMPLEMENTED) break; | ||
} | ||
if (result != ADBC_STATUS_OK) { | ||
return result; | ||
} | ||
|
||
if (version == ADBC_VERSION_1_0_0) { | ||
if (version >= ADBC_VERSION_1_0_0) { | ||
auto* driver = reinterpret_cast<struct AdbcDriver*>(raw_driver); | ||
CHECK_REQUIRED(driver, DatabaseNew); | ||
CHECK_REQUIRED(driver, DatabaseInit); | ||
|
@@ -816,6 +850,13 @@ AdbcStatusCode AdbcLoadDriverFromInitFunc(AdbcDriverInitFunc init_func, int vers | |
FILL_DEFAULT(driver, StatementSetSqlQuery); | ||
FILL_DEFAULT(driver, StatementSetSubstraitPlan); | ||
} | ||
if (version >= ADBC_VERSION_1_1_0) { | ||
auto* driver = reinterpret_cast<struct AdbcDriver*>(raw_driver); | ||
FILL_DEFAULT(driver, StatementExecuteSchema); | ||
Comment on lines
+870
to
+872
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't this also have the same for the other new functions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to defer implementation, just wanted to demonstrate the basic idea worked. I just want to get the API sort of in the right shape. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agh, let me retarget this PR against the branch, not main. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There - the intent is to build this out on a branch then manually ff-merge it at the end. If the approach looks reasonable I'm also going to build out more serious compatibility tests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha, okay then in that case I'm in favor of this approach. |
||
|
||
// Zero out the padding | ||
std::memset(driver->reserved, 0, sizeof(driver->reserved)); | ||
} | ||
|
||
return ADBC_STATUS_OK; | ||
|
||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the pointer-to-function be checked for null? Or is it the caller's job?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's assumed that if you're using these functions, you're using the driver manager, and that the driver manager has initialized all the functions. (Obviously, not true right now, since I decided to split implementation out.)