You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "lidavidm (via GitHub)" <gi...@apache.org> on 2023/05/18 13:26:20 UTC
[GitHub] [arrow-adbc] lidavidm opened a new pull request, #692: feat(format): introduce ADBC_VERSION_1_1_0
lidavidm opened a new pull request, #692:
URL: https://github.com/apache/arrow-adbc/pull/692
Fixes #317.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] pitrou commented on a diff in pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1205987167
##########
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:
I'm trying to understand the logic. Does it mean that, if the user passes a future `ADBC_VERSION_1_2_0`, we try calling `init_func` with `ADBC_VERSION_1_1_0` and then with `ADBC_VERSION_1_0_0`?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] zeroshade commented on a diff in pull request #692: feat(format): introduce ADBC_VERSION_1_1_0
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1197961857
##########
adbc.h:
##########
@@ -667,6 +675,34 @@ 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.
+ ///
+ /// @{
+
+ /// Pad the struct to have 64 pointers. Space reserved for future
+ /// growth.
+ void* reserved[35];
Review Comment:
are we going to wait for another revision after this to introduce the ExecuteSchema function you were thinking of? Or are you just doing two separate changes, one to introduce these reserved pointers and then one to add the new callback in place of one of the reserved pointers?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#issuecomment-1559310780
I will have to come back and add support for [ArrowDeviceArray](https://lists.apache.org/thread/o2hsw7o1gm3qgw5z51rmz6zqxh0p7bvk) once that vote passes. And probably, some standard options to hint to drivers to try to use a particular device if possible. (I think this is basically only possible for Flight-UCX, or maybe for a hypothetical database that uses an accelerator and is embeddable, like the GPU analog to SQLite.)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] zeroshade commented on a diff in pull request #692: feat(format): introduce ADBC_VERSION_1_1_0
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1198267421
##########
adbc.h:
##########
@@ -1138,6 +1293,43 @@ AdbcStatusCode AdbcStatementBindStream(struct AdbcStatement* statement,
struct ArrowArrayStream* stream,
struct AdbcError* error);
+/// \brief Cancel execution of an in-progress query.
+///
+/// This can be called during AdbcStatementExecuteQuery (or similar),
+/// or while consuming an ArrowArrayStream returned from such.
+/// Calling this function should make the other functions return
+/// ADBC_STATUS_CANCELLED or ECANCELED (for ArrowArrayStream).
+///
+/// This must always be thread-safe (other operations are not).
+///
+/// \since ADBC API revision 1.1.0
+/// \addtogroup adbc-1.1.0
+///
+/// \param[in] statement The statement to cancel.
+/// \param[out] error An optional location to return an error
+/// message if necessary.
+///
+/// \return ADBC_STATUS_INVALID_STATE if there is no query to cancel.
+/// \return ADBC_STATUS_UNKNOWN if the query could not be cancelled.
+ADBC_EXPORT
+AdbcStatusCode AdbcStatementCancel(struct AdbcStatement* statement,
+ struct AdbcError* error);
+
+/// \brief Get a double property of the statement.
Review Comment:
what would this be used for? is it just to avoid consumers having to call `strtof`/`strtod` themselves?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #692: feat(format): introduce ADBC_VERSION_1_1_0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1198275979
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -816,6 +846,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);
Review Comment:
Agh, let me retarget this PR against the branch, not main.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm merged pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #692: feat(format): introduce ADBC_VERSION_1_1_0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#issuecomment-1557347379
Added Java and Go, though I think I'd like to go back and align things a bit.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #692: feat(format): introduce ADBC_VERSION_1_1_0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#issuecomment-1555133210
Some other thoughts: maybe we shouldn't overload getInfo for things like current catalog/schema, and instead add a get/set option pair? That would be consistent with how Java does this
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] pitrou commented on a diff in pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1211817732
##########
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:
Ok, so perhaps clarify things a bit? "The driver must not access the new fields, which will carry undefined values" perhaps?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #692: feat(format): introduce ADBC_VERSION_1_1_0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1198276618
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -816,6 +846,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);
Review Comment:
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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#issuecomment-1557876076
Ah, I think I know what happened.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] pitrou commented on a diff in pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1205988693
##########
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:
Why are being stricter than `AdbcLoadDriverFromInitFunc` here?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] pitrou commented on a diff in pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1205990950
##########
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:
Perhaps you should explain the intent in a comment.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#issuecomment-1578935462
https://github.com/apache/arrow-adbc/pull/731
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] pitrou commented on a diff in pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1205998257
##########
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:
Should we also have `ADBC_DRIVER_1_1_0_SIZE`? That way, a 1.1.0 driver could perhaps be source-compatible with later versions of `adbc.h`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #692: feat(format): introduce ADBC_VERSION_1_1_0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1197963780
##########
adbc.h:
##########
@@ -667,6 +675,34 @@ 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.
+ ///
+ /// @{
+
+ /// Pad the struct to have 64 pointers. Space reserved for future
+ /// growth.
+ void* reserved[35];
Review Comment:
I'm going to add everything in this revision. I think what I'll actually do now is update this PR with all the spec changes for C/C++ but without implementations, then follow up with one PR per spec change for implementation (ditto for Java, Go). And we can tweak the spec changes if we find something during implementation that doesn't mesh.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #692: feat(format): introduce ADBC_VERSION_1_1_0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#issuecomment-1557614057
There's a macOS error in CI that I'll try to track down, but otherwise I think this is ready (for C, Java, Go)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] zeroshade commented on a diff in pull request #692: feat(format): introduce ADBC_VERSION_1_1_0
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1198271058
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -816,6 +846,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);
Review Comment:
shouldn't this also have the same for the other new functions? `AdbcStatementCancel` and `AdbcStatementGetDouble`?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] zeroshade commented on a diff in pull request #692: feat(format): introduce ADBC_VERSION_1_1_0
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1197973526
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -782,11 +782,14 @@ AdbcStatusCode AdbcLoadDriverFromInitFunc(AdbcDriverInitFunc init_func, int vers
}
auto result = init_func(version, raw_driver, error);
+ if (version == ADBC_VERSION_1_1_0 && result == ADBC_STATUS_NOT_IMPLEMENTED) {
+ result = init_func(ADBC_VERSION_1_0_0, raw_driver, error);
+ }
Review Comment:
maybe create a static const list of versions and just go through them in order until we get something other than `ADBC_STATUS_NOT_IMPLEMENTED` calling `init_func`? That way future updates are just adding the version to the list?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] zeroshade commented on a diff in pull request #692: feat(format): introduce ADBC_VERSION_1_1_0
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1197963786
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -640,8 +640,8 @@ 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");
+ if (version != ADBC_VERSION_1_0_0 && version != ADBC_VERSION_1_1_0) {
+ SetError(error, "Only ADBC 1.0.0 and 1.1.0 are supported");
return ADBC_STATUS_NOT_IMPLEMENTED;
}
Review Comment:
to avoid this becoming a huge thing in the future, should we make this a switch with a default? that way future updates would just be to add a new case statement to it rather than this if condition just growing ever larger
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] pitrou commented on a diff in pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1205995339
##########
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:
The new fields will be null-initialized?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1202301674
##########
adbc.h:
##########
@@ -1138,6 +1549,101 @@ AdbcStatusCode AdbcStatementBindStream(struct AdbcStatement* statement,
struct ArrowArrayStream* stream,
struct AdbcError* error);
+/// \brief Cancel execution of an in-progress query.
+///
+/// This can be called during AdbcStatementExecuteQuery (or similar),
+/// or while consuming an ArrowArrayStream returned from such.
+/// Calling this function should make the other functions return
+/// ADBC_STATUS_CANCELLED or ECANCELED (for ArrowArrayStream).
Review Comment:
I clarified this a bit.
##########
adbc.h:
##########
@@ -694,6 +961,24 @@ ADBC_EXPORT
AdbcStatusCode AdbcDatabaseSetOption(struct AdbcDatabase* database, const char* key,
const char* value, struct AdbcError* error);
+/// \brief Set an integer option on a database.
+///
+/// \since ADBC API revision 1.1.0
+/// \addtogroup adbc-1.1.0
+/// \return ADBC_STATUS_NOT_IMPLEMENTED if the option is not recognized
Review Comment:
Done!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] zeroshade commented on a diff in pull request #692: feat(format): introduce ADBC_VERSION_1_1_0
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1197963786
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -640,8 +640,8 @@ 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");
+ if (version != ADBC_VERSION_1_0_0 && version != ADBC_VERSION_1_1_0) {
+ SetError(error, "Only ADBC 1.0.0 and 1.1.0 are supported");
return ADBC_STATUS_NOT_IMPLEMENTED;
}
Review Comment:
to avoid this becoming a huge thing in the future, should we make this a switch with a default? that way future updates would just be to add a new case statement to it rather than this if condition just growing ever larger
ie:
```c++
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;
}
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#issuecomment-1557816222
I never really learned amd64 assembly but I think the CI failure I'm seeing is possibly a miscompilation or something odd? I see basically an unconditional call to UBSAN's abort here:
```
0x103770820 <+64>: movq -0x10(%rbp), %rax
0x103770824 <+68>: movq %rax, -0x20(%rbp)
0x103770828 <+72>: movq -0x20(%rbp), %rax
0x10377082c <+76>: movq %rax, -0x28(%rbp)
0x103770830 <+80>: xorl %eax, %eax
0x103770832 <+82>: testb $0x1, %al
0x103770834 <+84>: jne 0x10377084a ; <+106> at postgresql.cc
0x10377083a <+90>: leaq 0x109f3f(%rip), %rdi ; _dyld_private + 158976
0x103770841 <+97>: xorl %eax, %eax
0x103770843 <+99>: movl %eax, %esi
0x103770845 <+101>: callq 0x1037f75ae ; symbol stub for: __ubsan_handle_type_mismatch_v1_abort
```
aka (if I'm reading it right):
- -0x10(%rbp) is `raw_driver`,
- 4 instructions of shuffling registers/stack,
- a comparison that always fails (since we zero out EAX, then bitwise-AND it with 1 before doing a conditional jump),
- an unconditional call to `__ubsan_handle_type_mismatch_v1_abort`
Ghidra seems to agree:
```c
if (param_1 == 1000000) {
if (param_2 == 0) {
local_9 = 5;
}
else {
___ubsan_handle_type_mismatch_v1_abort(&DAT_00194780,0);
```
I'm not sure why this is happening, or if there's UB in the code and this is a consequence of it...
In the debugger, I can confirm `raw_driver` is not NULL despite UBSAN reporting a null-pointer-dereference.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#issuecomment-1559138586
Alright, CI passes here: https://github.com/lidavidm/arrow-adbc/tree/spec-1.1.0-newstyle
I'll go ahead and start working on implementation in a new branch while leaving this for review.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] kou commented on a diff in pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1202252450
##########
adbc.h:
##########
@@ -667,8 +786,84 @@ 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*, 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*, 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*, 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 64 pointers. Space reserved for future
+ /// growth.
+ void* reserved[18];
+
+ /// @}
};
+/// \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(DRIVER) \
+ (((size_t)(((uintptr_t)(&((DRIVER)->StatementSetSubstraitPlan))) - \
+ ((uintptr_t)(DRIVER)))) + \
+ sizeof(void*))
Review Comment:
How about using `offsetof()`?
```suggestion
#define ADBC_DRIVER_1_0_0_SIZE (offsetof(struct AdbcDriver, DatabaseGetOption))
```
##########
adbc.h:
##########
@@ -694,6 +961,24 @@ ADBC_EXPORT
AdbcStatusCode AdbcDatabaseSetOption(struct AdbcDatabase* database, const char* key,
const char* value, struct AdbcError* error);
+/// \brief Set an integer option on a database.
+///
+/// \since ADBC API revision 1.1.0
+/// \addtogroup adbc-1.1.0
+/// \return ADBC_STATUS_NOT_IMPLEMENTED if the option is not recognized
Review Comment:
Should we add `\param`s here and other `SetOption` family?
##########
adbc.h:
##########
@@ -684,6 +879,78 @@ struct ADBC_EXPORT AdbcDriver {
ADBC_EXPORT
AdbcStatusCode AdbcDatabaseNew(struct AdbcDatabase* database, struct AdbcError* error);
+/// \brief Get a string option of the database.
+///
+/// This must always be thread-safe (other operations are not).
+///
+/// The returned option value is only valid until the next call to
+/// GetOption or Release.
+///
+/// For standard options, drivers must always support getting the
+/// option value (if they support getting option values at all) via
+/// the type specified in the option. (For example, an option set via
+/// SetOptionDouble must be retrievable via GetOptionDouble.) Drivers
+/// may also support getting a converted option value via other
+/// getters if needed. (For example, getting the string
+/// representation of a double option.)
+///
+/// \since ADBC API revision 1.1.0
+/// \addtogroup adbc-1.1.0
+/// \param[in] database The database.
+/// \param[in] key The option to get.
+/// \param[out] value The option value.
+/// \param[out] error An optional location to return an error
+/// message if necessary.
+/// \return ADBC_STATUS_NOT_IMPLEMENTED if the option is not recognized.
+AdbcStatusCode AdbcDatabaseGetOption(struct AdbcDatabase* database, const char* key,
+ char** value, struct AdbcError* error);
+
+/// \brief Get an integer option of the database.
+///
+/// This must always be thread-safe (other operations are not).
+///
+/// For standard options, drivers must always support getting the
+/// option value (if they support getting option values at all) via
+/// the type specified in the option. (For example, an option set via
+/// SetOptionDouble must be retrievable via GetOptionDouble.) Drivers
+/// may also support getting a converted option value via other
+/// getters if needed. (For example, getting the string
Review Comment:
```suggestion
/// getters if needed. (For example, getting the integer
```
##########
adbc.h:
##########
@@ -684,6 +879,78 @@ struct ADBC_EXPORT AdbcDriver {
ADBC_EXPORT
AdbcStatusCode AdbcDatabaseNew(struct AdbcDatabase* database, struct AdbcError* error);
+/// \brief Get a string option of the database.
+///
+/// This must always be thread-safe (other operations are not).
+///
+/// The returned option value is only valid until the next call to
+/// GetOption or Release.
+///
+/// For standard options, drivers must always support getting the
+/// option value (if they support getting option values at all) via
+/// the type specified in the option. (For example, an option set via
+/// SetOptionDouble must be retrievable via GetOptionDouble.) Drivers
+/// may also support getting a converted option value via other
+/// getters if needed. (For example, getting the string
+/// representation of a double option.)
+///
+/// \since ADBC API revision 1.1.0
+/// \addtogroup adbc-1.1.0
+/// \param[in] database The database.
+/// \param[in] key The option to get.
+/// \param[out] value The option value.
+/// \param[out] error An optional location to return an error
+/// message if necessary.
+/// \return ADBC_STATUS_NOT_IMPLEMENTED if the option is not recognized.
+AdbcStatusCode AdbcDatabaseGetOption(struct AdbcDatabase* database, const char* key,
+ char** value, struct AdbcError* error);
+
+/// \brief Get an integer option of the database.
+///
+/// This must always be thread-safe (other operations are not).
+///
+/// For standard options, drivers must always support getting the
+/// option value (if they support getting option values at all) via
+/// the type specified in the option. (For example, an option set via
+/// SetOptionDouble must be retrievable via GetOptionDouble.) Drivers
+/// may also support getting a converted option value via other
+/// getters if needed. (For example, getting the string
+/// representation of a double option.)
+///
+/// \since ADBC API revision 1.1.0
+/// \addtogroup adbc-1.1.0
+/// \param[in] database The database.
+/// \param[in] key The option to get.
+/// \param[out] value The option value.
+/// \param[out] error An optional location to return an error
+/// message if necessary.
+/// \return ADBC_STATUS_NOT_IMPLEMENTED if the option is not recognized.
+AdbcStatusCode AdbcDatabaseGetOptionInt(struct AdbcDatabase* database, const char* key,
+ int64_t* value, struct AdbcError* error);
+
+/// \brief Get a double option of the database.
+///
+/// This must always be thread-safe (other operations are not).
+///
+/// For standard options, drivers must always support getting the
+/// option value (if they support getting option values at all) via
+/// the type specified in the option. (For example, an option set via
+/// SetOptionDouble must be retrievable via GetOptionDouble.) Drivers
+/// may also support getting a converted option value via other
+/// getters if needed. (For example, getting the string
+/// representation of a double option.)
Review Comment:
```suggestion
/// getters if needed. (For example, getting the double
/// representation of an integer option.)
```
##########
adbc.h:
##########
@@ -1138,6 +1549,101 @@ AdbcStatusCode AdbcStatementBindStream(struct AdbcStatement* statement,
struct ArrowArrayStream* stream,
struct AdbcError* error);
+/// \brief Cancel execution of an in-progress query.
+///
+/// This can be called during AdbcStatementExecuteQuery (or similar),
+/// or while consuming an ArrowArrayStream returned from such.
+/// Calling this function should make the other functions return
+/// ADBC_STATUS_CANCELLED or ECANCELED (for ArrowArrayStream).
Review Comment:
How is `ECANCELED` returned? `errno`?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] pitrou commented on a diff in pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1205999219
##########
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:
Should the pointer-to-function be checked for null? Or is it the caller's job?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] pitrou commented on a diff in pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1205989546
##########
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:
Should this check be moved in `AdbcLoadDriverFromInitFunc`?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] zeroshade commented on a diff in pull request #692: feat(format): introduce ADBC_VERSION_1_1_0
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1199158993
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -816,6 +846,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);
Review Comment:
Gotcha, okay then in that case I'm in favor of this approach.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #692: feat(format): introduce ADBC_VERSION_1_1_0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#issuecomment-1555062983
I'm quite tempted to just remove cpplint, it seems to mostly just give noise and it's basically unconfigurable/unmaintained
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #692: feat(format): introduce ADBC_VERSION_1_1_0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1198274168
##########
adbc.h:
##########
@@ -1138,6 +1293,43 @@ AdbcStatusCode AdbcStatementBindStream(struct AdbcStatement* statement,
struct ArrowArrayStream* stream,
struct AdbcError* error);
+/// \brief Cancel execution of an in-progress query.
+///
+/// This can be called during AdbcStatementExecuteQuery (or similar),
+/// or while consuming an ArrowArrayStream returned from such.
+/// Calling this function should make the other functions return
+/// ADBC_STATUS_CANCELLED or ECANCELED (for ArrowArrayStream).
+///
+/// This must always be thread-safe (other operations are not).
+///
+/// \since ADBC API revision 1.1.0
+/// \addtogroup adbc-1.1.0
+///
+/// \param[in] statement The statement to cancel.
+/// \param[out] error An optional location to return an error
+/// message if necessary.
+///
+/// \return ADBC_STATUS_INVALID_STATE if there is no query to cancel.
+/// \return ADBC_STATUS_UNKNOWN if the query could not be cancelled.
+ADBC_EXPORT
+AdbcStatusCode AdbcStatementCancel(struct AdbcStatement* statement,
+ struct AdbcError* error);
+
+/// \brief Get a double property of the statement.
Review Comment:
There's a constant above motivating it. Also, 1) there's no Get* function at all right now and 2) returning strings is really annoying due to allocations.
I could change this to return a string, the caveats would be either: 1) we have to add some release-callback thing (C Data Interface-ish) or 2) we return a const char*, _but_ calling this again or doing other things with the statement will invalidate the pointer (SQLite-ish) or 3) we expect a caller-allocated and/or fixed-size buffer (ODBC-ish)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #692: feat(format): introduce ADBC_VERSION_1_1_0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#issuecomment-1555042468
Interestingly the CI failure exposes what does look like a compatibility issue: the new fields in AdbcDriver make the Python test crash (because it mixes a driver using the new definition and a wheel using the old definition)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] kou commented on a diff in pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1202307483
##########
adbc.h:
##########
@@ -684,16 +875,121 @@ struct ADBC_EXPORT AdbcDriver {
ADBC_EXPORT
AdbcStatusCode AdbcDatabaseNew(struct AdbcDatabase* database, struct AdbcError* error);
+/// \brief Get a string option of the database.
+///
+/// This must always be thread-safe (other operations are not).
+///
+/// The returned option value is only valid until the next call to
+/// GetOption or Release.
+///
+/// For standard options, drivers must always support getting the
+/// option value (if they support getting option values at all) via
+/// the type specified in the option. (For example, an option set via
+/// SetOptionDouble must be retrievable via GetOptionDouble.) Drivers
+/// may also support getting a converted option value via other
+/// getters if needed. (For example, getting the string
+/// representation of a double option.)
+///
+/// \since ADBC API revision 1.1.0
+/// \addtogroup adbc-1.1.0
+/// \param[in] database The database.
+/// \param[in] key The option to get.
+/// \param[out] value The option value.
+/// \param[out] error An optional location to return an error
+/// message if necessary.
+/// \return ADBC_STATUS_NOT_IMPLEMENTED if the option is not recognized.
+AdbcStatusCode AdbcDatabaseGetOption(struct AdbcDatabase* database, const char* key,
+ char** value, struct AdbcError* error);
+
+/// \brief Get an integer option of the database.
+///
+/// This must always be thread-safe (other operations are not).
+///
+/// For standard options, drivers must always support getting the
+/// option value (if they support getting option values at all) via
+/// the type specified in the option. (For example, an option set via
+/// SetOptionDouble must be retrievable via GetOptionDouble.) Drivers
+/// may also support getting a converted option value via other
+/// getters if needed. (For example, getting the integer
+/// representation of a double option.)
+///
+/// \since ADBC API revision 1.1.0
+/// \addtogroup adbc-1.1.0
+/// \param[in] database The database.
+/// \param[in] key The option to get.
+/// \param[out] value The option value.
+/// \param[out] error An optional location to return an error
+/// message if necessary.
+/// \return ADBC_STATUS_NOT_IMPLEMENTED if the option is not recognized.
+AdbcStatusCode AdbcDatabaseGetOptionInt(struct AdbcDatabase* database, const char* key,
+ int64_t* value, struct AdbcError* error);
+
+/// \brief Get a double option of the database.
+///
+/// This must always be thread-safe (other operations are not).
+///
+/// For standard options, drivers must always support getting the
+/// option value (if they support getting option values at all) via
+/// the type specified in the option. (For example, an option set via
+/// SetOptionDouble must be retrievable via GetOptionDouble.) Drivers
+/// may also support getting a converted option value via other
+/// getters if needed. (For example, getting the double
+/// representation of an integer option.)
+///
+/// \since ADBC API revision 1.1.0
+/// \addtogroup adbc-1.1.0
+/// \param[in] database The database.
+/// \param[in] key The option to get.
+/// \param[out] value The option value.
+/// \param[out] error An optional location to return an error
+/// message if necessary.
+/// \return ADBC_STATUS_NOT_IMPLEMENTED if the option is not recognized.
+AdbcStatusCode AdbcDatabaseGetOptionDouble(struct AdbcDatabase* database, const char* key,
+ double* value, struct AdbcError* error);
+
/// \brief Set a char* option.
///
/// Options may be set before AdbcDatabaseInit. Some drivers may
/// support setting options after initialization as well.
///
+/// \param[in] database The database.
+/// \param[in] key The option to get.
Review Comment:
```suggestion
/// \param[in] key The option to set.
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] pitrou commented on a diff in pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1205994926
##########
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:
Does the driver manager allocate anything? In `AdbcLoadDriver` it's the caller that passes the `AdbcDriver` pointer.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] pitrou commented on a diff in pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1211818846
##########
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:
I don't know, to me it seems a bit weird to have two different ABI-stability mechanisms. It adds cognitive overload.
##########
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:
I don't know, to me it seems a bit weird to have two different ABI-stability mechanisms. It adds cognitive load.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#issuecomment-1557618916
TODOs
- [ ] Reserve more space for AdbcDriver?
- [ ] Figure out macOS CI error
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] pitrou commented on a diff in pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1205996408
##########
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:
Is it useful, given that we are careful with the struct size anyway?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#issuecomment-1561701114
I added language clarifying that returned ArrowArrayStreams must outlive the object that created them, since that seemed to be a complaint in #702
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #692: feat(format): introduce ADBC_VERSION_1_1_0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#issuecomment-1555044000
Ah, because the drivers do this
```
auto* driver = reinterpret_cast<struct AdbcDriver*>(raw_driver);
std::memset(driver, 0, sizeof(*driver));
```
and now that the struct has changed, they need to actually check the size of the struct they're about to manipulate.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #692: feat(format): introduce ADBC_VERSION_1_1_0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#issuecomment-1553587898
I've addressed feedback + added APIs for most of the new proposals. If this is OK I'll follow up with Go (with a set of interfaces, one per method or so) and Java (which is very easy).
I think we will have to punt on #320 and #621. I don't think #630 is worth doing. #685 is too speculative at this point.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] kou commented on a diff in pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1202303029
##########
adbc.h:
##########
@@ -1138,6 +1549,101 @@ AdbcStatusCode AdbcStatementBindStream(struct AdbcStatement* statement,
struct ArrowArrayStream* stream,
struct AdbcError* error);
+/// \brief Cancel execution of an in-progress query.
+///
+/// This can be called during AdbcStatementExecuteQuery (or similar),
+/// or while consuming an ArrowArrayStream returned from such.
+/// Calling this function should make the other functions return
+/// ADBC_STATUS_CANCELLED or ECANCELED (for ArrowArrayStream).
Review Comment:
Ah, I see.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1202283011
##########
adbc.h:
##########
@@ -1138,6 +1549,101 @@ AdbcStatusCode AdbcStatementBindStream(struct AdbcStatement* statement,
struct ArrowArrayStream* stream,
struct AdbcError* error);
+/// \brief Cancel execution of an in-progress query.
+///
+/// This can be called during AdbcStatementExecuteQuery (or similar),
+/// or while consuming an ArrowArrayStream returned from such.
+/// Calling this function should make the other functions return
+/// ADBC_STATUS_CANCELLED or ECANCELED (for ArrowArrayStream).
Review Comment:
ArrowArrayStream's methods return an `int` that is specified to be `errno`-compatible, but not actually errno:
https://github.com/apache/arrow/blob/fbe5f641d327ee81db00ce5f056940a69f4d8603/cpp/src/arrow/c/abi.h#L75
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#issuecomment-1559312051
Ah, and right now we can't even support Flight-UCX since we aren't using the C++ Flight implementation anyways.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] pitrou commented on a diff in pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1205992297
##########
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:
Is it allowed to be a later version?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #692: feat(format): introduce ADBC_VERSION_1_1_0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#discussion_r1198272680
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -816,6 +846,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);
Review Comment:
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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#issuecomment-1557828808
For now let's see if writing it differently helps...
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#issuecomment-1577415895
I'm going to merge this soon (into the branch) and then update/continue the follow-up PRs.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #692: feat(format): introduce ADBC API revision 1.1.0
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #692:
URL: https://github.com/apache/arrow-adbc/pull/692#issuecomment-1578933349
Ah...let me file a follow up PR for some of Antoine's last comments
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org