lidavidm commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1206879398
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -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;
Review Comment:
I added the check there as well.
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -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;
}
+ if (!raw_driver) {
+ SetError(error, "Must provide non-NULL raw_driver");
+ return ADBC_STATUS_INVALID_ARGUMENT;
+ }
Review Comment:
I replicated the check there.
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -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;
Review Comment:
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.
##########
adbc.h:
##########
@@ -667,8 +786,80 @@ struct ADBC_EXPORT AdbcDriver {
struct AdbcError*);
AdbcStatusCode (*StatementSetSubstraitPlan)(struct AdbcStatement*, const
uint8_t*,
size_t, struct AdbcError*);
+
+ /// \defgroup adbc-1.1.0 ADBC API Revision 1.1.0
+ ///
+ /// Functions added in ADBC 1.1.0. For backwards compatibility,
+ /// these members must not be accessed unless the version passed to
+ /// the AdbcDriverInitFunc is greater than or equal to
+ /// ADBC_VERSION_1_1_0.
+ ///
+ /// For a 1.0.0 driver being loaded by a 1.1.0 driver manager: the
+ /// 1.1.0 manager will allocate the new, expanded AdbcDriver struct
+ /// and attempt to have the driver initialize it with
+ /// ADBC_VERSION_1_1_0. This must return an error, after which the
+ /// driver will try again with ADBC_VERSION_1_0_0. The driver must
+ /// not access the new fields.
Review Comment:
In this scenario, the new fields do not exist because it is an old driver
manager working with a new driver.
##########
adbc.h:
##########
@@ -667,8 +786,80 @@ struct ADBC_EXPORT AdbcDriver {
struct AdbcError*);
AdbcStatusCode (*StatementSetSubstraitPlan)(struct AdbcStatement*, const
uint8_t*,
size_t, struct AdbcError*);
+
+ /// \defgroup adbc-1.1.0 ADBC API Revision 1.1.0
+ ///
+ /// Functions added in ADBC 1.1.0. For backwards compatibility,
+ /// these members must not be accessed unless the version passed to
+ /// the AdbcDriverInitFunc is greater than or equal to
+ /// ADBC_VERSION_1_1_0.
+ ///
+ /// For a 1.0.0 driver being loaded by a 1.1.0 driver manager: the
+ /// 1.1.0 manager will allocate the new, expanded AdbcDriver struct
Review Comment:
If you use the `AdbcDatabaseInit` path (where the driver manager pretends to
just be an ADBC driver, and you pass the driver name as one of the init
options) then it will allocate for you, yes.
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -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);
Review Comment:
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.)
##########
adbc.h:
##########
@@ -315,6 +354,16 @@ struct ADBC_EXPORT AdbcError {
///
/// \see AdbcConnectionGetInfo
#define ADBC_INFO_DRIVER_ARROW_VERSION 102
+/// \brief The driver ADBC API version (type: int64).
+///
+/// The value should be one of the ADBC_VERSION constants.
Review Comment:
"Yes", in that if/when there's another revision, then that would be a valid
value, but earlier clients/drivers of course wouldn't know about that.
##########
adbc.h:
##########
@@ -667,8 +786,80 @@ struct ADBC_EXPORT AdbcDriver {
struct AdbcError*);
AdbcStatusCode (*StatementSetSubstraitPlan)(struct AdbcStatement*, const
uint8_t*,
size_t, struct AdbcError*);
+
+ /// \defgroup adbc-1.1.0 ADBC API Revision 1.1.0
+ ///
+ /// Functions added in ADBC 1.1.0. For backwards compatibility,
+ /// these members must not be accessed unless the version passed to
+ /// the AdbcDriverInitFunc is greater than or equal to
+ /// ADBC_VERSION_1_1_0.
+ ///
+ /// For a 1.0.0 driver being loaded by a 1.1.0 driver manager: the
+ /// 1.1.0 manager will allocate the new, expanded AdbcDriver struct
+ /// and attempt to have the driver initialize it with
+ /// ADBC_VERSION_1_1_0. This must return an error, after which the
+ /// driver will try again with ADBC_VERSION_1_0_0. The driver must
+ /// not access the new fields.
+ ///
+ /// For a 1.1.0 driver being loaded by a 1.0.0 driver manager: the
+ /// 1.0.0 manager will allocate the old AdbcDriver struct and
+ /// attempt to have the driver initialize it with
+ /// ADBC_VERSION_1_0_0. The driver must not access the new fields,
+ /// and should initialize the old fields.
+ ///
+ /// @{
+
+ AdbcStatusCode (*DatabaseGetOption)(struct AdbcDatabase*, const char*, const
char**,
+ struct AdbcError*);
+ AdbcStatusCode (*DatabaseGetOptionInt)(struct AdbcDatabase*, const char*,
int64_t*,
+ struct AdbcError*);
+ AdbcStatusCode (*DatabaseGetOptionDouble)(struct AdbcDatabase*, const char*,
double*,
+ struct AdbcError*);
+ AdbcStatusCode (*DatabaseSetOptionInt)(struct AdbcDatabase*, const char*,
int64_t,
+ struct AdbcError*);
+ AdbcStatusCode (*DatabaseSetOptionDouble)(struct AdbcDatabase*, const char*,
double,
+ struct AdbcError*);
+
+ AdbcStatusCode (*ConnectionGetOption)(struct AdbcConnection*, const char*,
const char**,
+ struct AdbcError*);
+ AdbcStatusCode (*ConnectionGetOptionInt)(struct AdbcConnection*, const
char*, int64_t*,
+ struct AdbcError*);
+ AdbcStatusCode (*ConnectionGetOptionDouble)(struct AdbcConnection*, const
char*,
+ double*, struct AdbcError*);
+ AdbcStatusCode (*ConnectionSetOptionInt)(struct AdbcConnection*, const
char*, int64_t,
+ struct AdbcError*);
+ AdbcStatusCode (*ConnectionSetOptionDouble)(struct AdbcConnection*, const
char*, double,
+ struct AdbcError*);
+
+ AdbcStatusCode (*StatementCancel)(struct AdbcStatement*, struct AdbcError*);
+ AdbcStatusCode (*StatementExecuteSchema)(struct AdbcStatement*, struct
ArrowSchema*,
+ struct AdbcError*);
+ AdbcStatusCode (*StatementGetOption)(struct AdbcStatement*, const char*,
const char**,
+ struct AdbcError*);
+ AdbcStatusCode (*StatementGetOptionInt)(struct AdbcStatement*, const char*,
int64_t*,
+ struct AdbcError*);
+ AdbcStatusCode (*StatementGetOptionDouble)(struct AdbcStatement*, const
char*, double*,
+ struct AdbcError*);
+ AdbcStatusCode (*StatementSetOptionInt)(struct AdbcStatement*, const char*,
int64_t,
+ struct AdbcError*);
+ AdbcStatusCode (*StatementSetOptionDouble)(struct AdbcStatement*, const
char*, double,
+ struct AdbcError*);
+
+ /// Pad the struct to have 96 pointers. Space reserved for future growth.
+ void* reserved[50];
+
+ /// @}
};
+/// \brief The size of the AdbcDriver structure in ADBC 1.0.0.
+/// Drivers written for ADBC 1.1.0 and later should never touch more
+/// than this portion of an AdbcDriver struct when given
+/// ADBC_VERSION_1_0_0.
+///
+/// \since ADBC API revision 1.1.0
+/// \addtogroup adbc-1.1.0
+#define ADBC_DRIVER_1_0_0_SIZE (offsetof(struct AdbcDriver, DatabaseGetOption))
Review Comment:
Added.
##########
adbc.h:
##########
@@ -667,8 +786,80 @@ struct ADBC_EXPORT AdbcDriver {
struct AdbcError*);
AdbcStatusCode (*StatementSetSubstraitPlan)(struct AdbcStatement*, const
uint8_t*,
size_t, struct AdbcError*);
+
+ /// \defgroup adbc-1.1.0 ADBC API Revision 1.1.0
+ ///
+ /// Functions added in ADBC 1.1.0. For backwards compatibility,
+ /// these members must not be accessed unless the version passed to
+ /// the AdbcDriverInitFunc is greater than or equal to
+ /// ADBC_VERSION_1_1_0.
+ ///
+ /// For a 1.0.0 driver being loaded by a 1.1.0 driver manager: the
+ /// 1.1.0 manager will allocate the new, expanded AdbcDriver struct
+ /// and attempt to have the driver initialize it with
+ /// ADBC_VERSION_1_1_0. This must return an error, after which the
+ /// driver will try again with ADBC_VERSION_1_0_0. The driver must
+ /// not access the new fields.
+ ///
+ /// For a 1.1.0 driver being loaded by a 1.0.0 driver manager: the
+ /// 1.0.0 manager will allocate the old AdbcDriver struct and
+ /// attempt to have the driver initialize it with
+ /// ADBC_VERSION_1_0_0. The driver must not access the new fields,
+ /// and should initialize the old fields.
+ ///
+ /// @{
+
+ AdbcStatusCode (*DatabaseGetOption)(struct AdbcDatabase*, const char*, const
char**,
+ struct AdbcError*);
+ AdbcStatusCode (*DatabaseGetOptionInt)(struct AdbcDatabase*, const char*,
int64_t*,
+ struct AdbcError*);
+ AdbcStatusCode (*DatabaseGetOptionDouble)(struct AdbcDatabase*, const char*,
double*,
+ struct AdbcError*);
+ AdbcStatusCode (*DatabaseSetOptionInt)(struct AdbcDatabase*, const char*,
int64_t,
+ struct AdbcError*);
+ AdbcStatusCode (*DatabaseSetOptionDouble)(struct AdbcDatabase*, const char*,
double,
+ struct AdbcError*);
+
+ AdbcStatusCode (*ConnectionGetOption)(struct AdbcConnection*, const char*,
const char**,
+ struct AdbcError*);
+ AdbcStatusCode (*ConnectionGetOptionInt)(struct AdbcConnection*, const
char*, int64_t*,
+ struct AdbcError*);
+ AdbcStatusCode (*ConnectionGetOptionDouble)(struct AdbcConnection*, const
char*,
+ double*, struct AdbcError*);
+ AdbcStatusCode (*ConnectionSetOptionInt)(struct AdbcConnection*, const
char*, int64_t,
+ struct AdbcError*);
+ AdbcStatusCode (*ConnectionSetOptionDouble)(struct AdbcConnection*, const
char*, double,
+ struct AdbcError*);
+
+ AdbcStatusCode (*StatementCancel)(struct AdbcStatement*, struct AdbcError*);
+ AdbcStatusCode (*StatementExecuteSchema)(struct AdbcStatement*, struct
ArrowSchema*,
+ struct AdbcError*);
+ AdbcStatusCode (*StatementGetOption)(struct AdbcStatement*, const char*,
const char**,
+ struct AdbcError*);
+ AdbcStatusCode (*StatementGetOptionInt)(struct AdbcStatement*, const char*,
int64_t*,
+ struct AdbcError*);
+ AdbcStatusCode (*StatementGetOptionDouble)(struct AdbcStatement*, const
char*, double*,
+ struct AdbcError*);
+ AdbcStatusCode (*StatementSetOptionInt)(struct AdbcStatement*, const char*,
int64_t,
+ struct AdbcError*);
+ AdbcStatusCode (*StatementSetOptionDouble)(struct AdbcStatement*, const
char*, double,
+ struct AdbcError*);
+
+ /// Pad the struct to have 96 pointers. Space reserved for future growth.
Review Comment:
It's not strictly necessary but would let us avoid technically breaking ABI
in the future without having to be as careful about what's going on.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
