You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kou (via GitHub)" <gi...@apache.org> on 2023/05/23 12:52:24 UTC

[GitHub] [arrow-adbc] kou commented on a diff in pull request #692: feat(format): introduce ADBC API revision 1.1.0

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