Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion c/driver_manager/adbc_version_100_compatibility_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,11 @@ class AdbcVersion : public ::testing::Test {
TEST_F(AdbcVersion, StructSize) {
ASSERT_EQ(sizeof(AdbcErrorVersion100), ADBC_ERROR_1_0_0_SIZE);
ASSERT_EQ(sizeof(AdbcError), ADBC_ERROR_1_1_0_SIZE);
ASSERT_EQ(sizeof(AdbcError), ADBC_ERROR_1_2_0_SIZE);

ASSERT_EQ(sizeof(AdbcDriverVersion100), ADBC_DRIVER_1_0_0_SIZE);
ASSERT_EQ(sizeof(AdbcDriver), ADBC_DRIVER_1_1_0_SIZE);
ASSERT_EQ(offsetof(struct AdbcDriver, StatementNextResultSet), ADBC_DRIVER_1_1_0_SIZE);
ASSERT_EQ(sizeof(AdbcDriver), ADBC_DRIVER_1_2_0_SIZE);
}

// Initialize a version 1.0.0 driver with the version 1.1.0 driver struct.
Expand Down
130 changes: 119 additions & 11 deletions c/include/arrow-adbc/adbc.h
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,15 @@ struct ADBC_EXPORT AdbcError {
/// \since ADBC API revision 1.1.0
#define ADBC_ERROR_1_1_0_SIZE (sizeof(struct AdbcError))

/// \brief The size of the AdbcError structure in ADBC 1.2.0.
///
/// Drivers written for ADBC 1.2.0 and later should never touch more than this
/// portion of an AdbcDriver struct when vendor_code is
/// ADBC_ERROR_VENDOR_CODE_PRIVATE_DATA.
///
/// \since ADBC API revision 1.2.0
#define ADBC_ERROR_1_2_0_SIZE (sizeof(struct AdbcError))

/// \brief Extra key-value metadata for an error.
///
/// The fields here are owned by the driver and should not be freed. The
Expand Down Expand Up @@ -423,6 +432,14 @@ const struct AdbcError* AdbcErrorFromArrayStream(struct ArrowArrayStream* stream
/// \since ADBC API revision 1.1.0
#define ADBC_VERSION_1_1_0 1001000

/// \brief ADBC revision 1.2.0
///
/// When passed to an AdbcDriverInitFunc(), the driver parameter must
/// point to an AdbcDriver.
///
/// \since ADBC API revision 1.2.0
#define ADBC_VERSION_1_2_0 1002000

/// \brief Canonical option value for enabling an option.
///
/// For use as the value in SetOption calls.
Expand Down Expand Up @@ -525,6 +542,7 @@ const struct AdbcError* AdbcErrorFromArrayStream(struct ArrowArrayStream* stream
/// \see AdbcConnectionGetInfo
/// \see ADBC_VERSION_1_0_0
/// \see ADBC_VERSION_1_1_0
/// \see ADBC_VERSION_1_2_0
#define ADBC_INFO_DRIVER_ADBC_VERSION 103

/// \brief Return metadata on catalogs, schemas, tables, and columns.
Expand Down Expand Up @@ -1059,17 +1077,18 @@ struct ADBC_EXPORT AdbcDriver {
/// 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, which will carry undefined values.
/// When a driver implementing an older spec is loaded by a newer
/// driver manager, the newer manager will allocate the new, expanded
/// AdbcDriver struct and attempt to have the driver initialize it with
/// the newer version. This must return an error, after which the driver
/// will try again with successively older versions all the way back to
/// ADBC_VERSION_1_0_0. The driver must not access the new fields,
/// which will carry undefined values.
Comment thread
zeroshade marked this conversation as resolved.
Outdated
///
/// 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,
/// When a driver implementing a newer spec is loaded by an older
/// driver manager, the older manager will allocate the old AdbcDriver
/// struct and attempt to have the driver initialize it with the
/// older version. The driver must not access the new fields,
/// and should initialize the old fields.
///
/// @{
Expand Down Expand Up @@ -1135,6 +1154,36 @@ struct ADBC_EXPORT AdbcDriver {
struct AdbcError*);

/// @}

/// \defgroup adbc-1.2.0 ADBC API Revision 1.2.0
///
/// Functions added in ADBC 1.2.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_2_0.
///
/// When a driver implementing an older spec is loaded by a newer
/// driver manager, the newer manager will allocate the new, expanded
/// AdbcDriver struct and attempt to have the driver initialize it with
/// the newer version. This must return an error, after which the driver
/// will try again with successively older versions all the way back to
/// ADBC_VERSION_1_0_0. The driver must not access the new fields,
/// which will carry undefined values.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the manager populate them with functions that fail immediately?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not actually sure what this is trying to say, now that you point it out.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is in line with the current behavior. No attempt is made to pre-populate with functions that fail immediately (likely to avoid having to create separate versions for every callback going forward). We currently leave these members undefined in the struct. If we want to change this behavior, I'd prefer we do it in a different PR, not this one.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we should describe the user perspective and driver perspective separately.

From the user perspective: the manager will return a fully populated struct, but some of the functions may fail with NOT_IMPLEMENTED.

From the driver perspective: they should not access members outside of the version specified by the driver manager. (Though this is already required by the contract on the init func, and also you have no other way of knowing the size of the struct, nor do you know whether it's the driver manager or something else calling you.) I don't think we need to specify what exactly the driver manager does here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping @zeroshade, I think we should at least avoid specifying exactly what the driver manager does here, just that the driver manager expects that if it gives the driver a particular version, it should not try to access any functions defined after that version.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How's the updated version?

///
/// When a driver implementing a newer spec is loaded by an older
/// driver manager, the older manager will allocate the old AdbcDriver
/// struct and attempt to have the driver initialize it with the
/// older version. The driver must not access the new fields,
/// and should initialize the old fields.
///
/// @{

AdbcStatusCode (*StatementNextResultSet)(struct AdbcStatement*,
struct ArrowArrayStream*,
struct AdbcPartitions*, int64_t*,
struct AdbcError*);

/// @}
};

/// \brief The size of the AdbcDriver structure in ADBC 1.0.0.
Expand All @@ -1151,7 +1200,15 @@ struct ADBC_EXPORT AdbcDriver {
/// ADBC_VERSION_1_1_0.
///
/// \since ADBC API revision 1.1.0
#define ADBC_DRIVER_1_1_0_SIZE (sizeof(struct AdbcDriver))
#define ADBC_DRIVER_1_1_0_SIZE (offsetof(struct AdbcDriver, StatementNextResultSet))

/// \brief The size of the AdbcDriver structure in ADBC 1.2.0.
/// Drivers written for ADBC 1.2.0 and later should never touch more
/// than this portion of an AdbcDriver struct when given
/// ADBC_VERSION_1_2_0.
///
/// \since ADBC API revision 1.2.0
#define ADBC_DRIVER_1_2_0_SIZE (sizeof(struct AdbcDriver))

/// @}

Expand Down Expand Up @@ -2013,6 +2070,57 @@ AdbcStatusCode AdbcStatementExecuteQuery(struct AdbcStatement* statement,
struct ArrowArrayStream* out,
int64_t* rows_affected, struct AdbcError* error);

/// \brief Retrieve the next result set from a multi-result-set execution.
///
/// This AdbcStatement must outlive the returned ArrowArrayStream.
///
/// \since ADBC API revision 1.2.0
///
/// For an execution that returns multiple result sets, this can be called after
/// iterating the first result set to get the next and subsequent result sets. A
/// driver MAY support calling AdbcStatementNextResultSet while the previous result is
/// still being consumed (i.e. before the previous ArrowArrayStream is released),
/// but this is not required. If the driver does not support this, it should return
/// ADBC_STATUS_INVALID_STATE if the previous result set is still active. Otherwise
/// a driver should return ADBC_STATUS_OK to indicate successful execution of this
/// function regardless of whether or not an additional result set is available.
///
/// If the original execution was via AdbcStatementExecuteSchema, then the
/// ArrowArrayStream populated by this function should only contain the schema of the
/// result set and not any data.
///
/// Either partitions or out must be NULL to indicate which style of output is desired
/// by the caller. Supplying non-NULL values to both must result in
/// ADBC_STATUS_INVALID_ARGUMENT. If the original execution was via
/// AdbcStatementExecuteQuery and the call to AdbcStatementNextResultSet has a non-NULL
/// partitions, or the original was via AdbcStatementExecutePartitions and this call has a
/// non-NULL out, then the driver may choose to return the data in a different style than
/// the original result set. If it does not (or cannot) then it should return
/// ADBC_STATUS_INVALID_ARGUMENT.
///
/// The driver indicates that no additional result sets are available by setting the
/// release callback on out (or partitions, whichever was not-NULL) to NULL and returning
/// ADBC_STATUS_OK.
///
/// \param[in] statement The statement to fetch a subsequent result for
/// \param[out] out The result stream to populate, or NULL if using partitions
/// \param[out] partitions The partitions to populate, or NULL if using out
/// \param[out] rows_affected The number of rows affected if known, else -1. Pass NULL if
/// the client does not want this information.
/// \param[out] error An optional location to return an error message if necessary.
///
/// \return ADBC_STATUS_NOT_IMPLEMENTED if the driver does not support multi-result set
/// execution, ADBC_STATUS_INVALID_STATE if called at an inappropriate time but the
/// driver does support multi-result set execution, ADBC_STATUS_INVALID_ARGUMENT if both
/// out and partitions are non-NULL, or if the output style is incompatible with the
/// original execution, and ADBC_STATUS_OK (or an appropriate error code) otherwise.
ADBC_EXPORT
AdbcStatusCode AdbcStatementNextResultSet(struct AdbcStatement* statement,
struct ArrowArrayStream* out,
struct AdbcPartitions* partitions,
int64_t* rows_affected,
struct AdbcError* error);
Comment thread
zeroshade marked this conversation as resolved.
Outdated

/// \brief Get the schema of the result set of a query without
/// executing it.
///
Expand Down
130 changes: 119 additions & 11 deletions go/adbc/drivermgr/arrow-adbc/adbc.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion r/adbcdrivermanager/src/radbc.cc
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,8 @@ extern "C" SEXP RAdbcAllocateDriver(void) {
R_RegisterCFinalizer(driver_xptr, &finalize_driver_xptr);

// Make sure we error when the ADBC spec is updated
static_assert(sizeof(AdbcDriver) == ADBC_DRIVER_1_1_0_SIZE);
static_assert(offsetof(struct AdbcDriver, StatementNextResultSet) ==
ADBC_DRIVER_1_1_0_SIZE);
SEXP version_sexp = PROTECT(Rf_ScalarInteger(ADBC_VERSION_1_1_0));

const char* names[] = {"driver", "version", ""};
Expand Down
Loading