lidavidm commented on code in PR #2918:
URL: https://github.com/apache/arrow-adbc/pull/2918#discussion_r2157839136
##########
c/include/arrow-adbc/adbc_driver_manager.h:
##########
@@ -51,6 +63,52 @@ ADBC_EXPORT
AdbcStatusCode AdbcLoadDriver(const char* driver_name, const char* entrypoint,
int version, void* driver, struct AdbcError*
error);
+/// \brief Common entry point to search for and load a driver or manifest.
+///
+/// The driver manager can fill in default implementations of some ADBC
functions
+/// for drivers. Drivers must implement a minimum level of functionality for
this
+/// to be possible, however, and some functions must be implemented by the
driver.
+///
+/// This function is different from AdbcLoadDriver in that it also accepts the
name
+/// of a driver manifest file, and allows specifying options to control what
+/// directories it will search through. The behavior is as follows:
+///
+/// If the passed in driver_name is an absolute path:
+/// - If the path has a `.toml` extension, it will attempt to parse the
manifest and load
+/// the driver specified within it. Erroring if this fails.
+/// - If the path has an extension other than `.toml`, it will attempt to load
the path as
+/// a shared library. Erroring if this fails.
+///
+/// If the passed in driver_name does not have an extension and is not an
absolute path:
+/// - The load_options parameter will control whether the driver manager will
search
+/// the ADBC_CONFIG_PATH environment variable, the user configuration
directory, and/or
+/// the system level directory of /etc/adbc for either a manifest file or a
shared
+/// library.
+/// - For each path to be search, it will first look for
<path>/<driver_name>.toml. If
+/// that file exists, it will attempt to parse the manifest and load the
driver
+/// specified within it. Erroring if this fails.
Review Comment:
```suggestion
/// specified within it, erroring if this fails.
```
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -15,26 +15,45 @@
// specific language governing permissions and limitations
// under the License.
-#include "arrow-adbc/adbc_driver_manager.h"
+#if defined(_WIN32)
+#include <windows.h> // Must come first
Review Comment:
should we define _NOMINMAX too just to be safe?
##########
c/include/arrow-adbc/adbc_driver_manager.h:
##########
@@ -51,6 +63,52 @@ ADBC_EXPORT
AdbcStatusCode AdbcLoadDriver(const char* driver_name, const char* entrypoint,
int version, void* driver, struct AdbcError*
error);
+/// \brief Common entry point to search for and load a driver or manifest.
+///
+/// The driver manager can fill in default implementations of some ADBC
functions
+/// for drivers. Drivers must implement a minimum level of functionality for
this
+/// to be possible, however, and some functions must be implemented by the
driver.
+///
+/// This function is different from AdbcLoadDriver in that it also accepts the
name
+/// of a driver manifest file, and allows specifying options to control what
+/// directories it will search through. The behavior is as follows:
+///
+/// If the passed in driver_name is an absolute path:
+/// - If the path has a `.toml` extension, it will attempt to parse the
manifest and load
+/// the driver specified within it. Erroring if this fails.
+/// - If the path has an extension other than `.toml`, it will attempt to load
the path as
+/// a shared library. Erroring if this fails.
+///
+/// If the passed in driver_name does not have an extension and is not an
absolute path:
+/// - The load_options parameter will control whether the driver manager will
search
+/// the ADBC_CONFIG_PATH environment variable, the user configuration
directory, and/or
+/// the system level directory of /etc/adbc for either a manifest file or a
shared
+/// library.
+/// - For each path to be search, it will first look for
<path>/<driver_name>.toml. If
Review Comment:
```suggestion
/// - For each path to be searched, it will first look for
<path>/<driver_name>.toml. If
```
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -1716,6 +2176,12 @@ AdbcStatusCode AdbcLoadDriver(const char* driver_name,
const char* entrypoint,
return status;
}
+AdbcStatusCode AdbcLoadDriver(const char* driver_name, const char* entrypoint,
+ int version, void* raw_driver, struct AdbcError*
error) {
+ return AdbcFindLoadDriver(driver_name, entrypoint, version,
ADBC_LOAD_FLAG_DEFAULT,
+ raw_driver, error);
+}
Review Comment:
Maybe this should default to no options enabled?
##########
ci/scripts/cpp_build.sh:
##########
@@ -27,6 +27,8 @@ set -e
: ${BUILD_DRIVER_BIGQUERY:=${BUILD_ALL}}
# Must be explicitly enabled
: ${BUILD_INTEGRATION_DUCKDB:=0}
+: ${ADBC_DRIVER_MANAGER_USER_CONFIG_TEST:=0}
+: ${ADBC_DRIVER_MANAGER_SYSTEM_CONFIG_TEST:=0}
Review Comment:
Can we make these options consistent with the Windows options and the
existing options?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
