feat(c/driver_manager): add new function to allow loading by manifest #2918

zeroshade · 2025-06-03T20:58:39Z

Following the discussions on the mailing list and on https://docs.google.com/document/d/1ANeoDI7MCi9xFt_q8z726xKweWS2vgqO8Ut6BSTt5zM/edit?tab=t.0#heading=h.rd6unnsl7uy this PR implements the proposed behavior by adding a new function to the driver manager rather than changing the existing AdbcLoadDriver (for now).

The new function, AdbcFindLoadDriver adds a parameter for LoadOptions along with behavior to search directories at multiple levels (environment, user, system) to search for either a manifest or a driver library. This should improve the developer experience and make it easier for users and applications to leverage the driver manager.

zeroshade

I've tagged everyone who commented on the google doc or on the mailing list as reviewers so that this can be reviewed and discussed here.

zeroshade

duplicate comment, oops

zeroshade · 2025-06-03T21:36:28Z

@paleolimbot I think i'm gonna need a little help getting the R build to recognize the vendored toml++ header, as far as I can tell looking at bootstrap-c.R it should be copying it over correctly from the vendor dir, so I'm not sure what's wrong. Any ideas?

zeroshade · 2025-06-03T23:28:59Z

@paleolimbot looks like i figured it out! 😄

c/driver_manager/adbc_driver_manager.cc

c/driver_manager/CMakeLists.txt

c/vendor/toml++/toml.hpp

c/include/arrow-adbc/adbc_driver_manager.h

c/driver_manager/adbc_driver_manager_test.cc

lidavidm · 2025-06-04T01:45:34Z

c/driver_manager/adbc_driver_manager.cc

-#include <dlfcn.h>
-#endif  // defined(_WIN32)
+ADBC_EXPORT
+std::vector<std::filesystem::path> AdbcParsePath(const std::string_view& path);


Can we use InternalAdbc instead?

It has to start with Adbc because of the symbols.map, so we can't use InternalAdbc unless we change the symbols.map

We could change it to AdbcInternal if you like, or just not have the separate unit test for it and not export it, but I prefer to test it

Er, the symbols.map accepts InternalAdbc.

Ah, must have missed that. Why doesn't the other internal function use that too then?

They don't necessarily need to be exposed. This is only for internal functions which we want to expose for unit testing.

Right, I'm talking about AdbcDriverManagerDefaultEntrypoint which is the other function we only expose for unit testing. It's not using InternalAdbc that's why I followed the same pattern. It feels like either both should be InternalAdbc or neither should

Sure, we can rename it. That probably predates the symbols.map.

Sure, I'll rename both then

Looks like I needed to add InternalAdbc* to the extern "C++" portion of the symbols.map for this to work, otherwise it didn't export the symbol

c/driver_manager/adbc_driver_manager.cc

c/driver_manager/adbc_driver_manager_test.cc

pitrou · 2025-06-04T13:19:01Z

c/driver_manager/adbc_driver_manager.cc

+std::filesystem::path UserConfigDir() {
+  std::filesystem::path config_dir;
+#if defined(_WIN32)
+  auto dir = std::getenv("AppData");


Since this is dealing with filesystem paths, I would recommend that you use the Unicode API functions (either _wgetenv or GetEnvironmentVariableW) and then encode to utf8.

Generally I think you should use the convention that std::string and std::string_view paths on Windows are utf8-encoded. If you use the bytewise ("ASCII") APIs provided by Windows, they use the current codepage which is brittle and limited (it cannot represent everything).

A realistic test for this would be to create a Windows user with a é (e.g.) in the name and ensure that this works (at least that's what happened the last time I had to debug this!)

Also, does this need to use the Windows API? (The only other place I've seen this does:)

https://github.com/r-lib/rappdirs/blob/955f2e7997ed6d65cc47a3d77d0cf324c2685897/src/win-path.c#L13

...create a Windows user with a é (e.g.)

Everyone goes through this ritual with Windows programming, huh? :)

@paleolimbot I don't think we need to use the Windows API that you're pointing at there, as the only available options for that are %LOCALAPPDATA%\Desktop, %LOCALAPPDATA%\Documents, %LOCALAPPDATA%\Favorites and %LOCALAPPDATA%\ProgramData so it looks like you can't just get the %AppData% folder with that API

I think there's a constant for CSIDL_APPDATA?

https://github.com/tox-dev/platformdirs/blob/02a42615b7d8b9ffd08aed8fd63bf6076fe9fea6/src/platformdirs/windows.py#L222-L224

go/adbc/drivermgr/adbc_driver_manager.cc

paleolimbot

Thank you for handling the R builds! This is going to be great and I'm excited for the opportunities it will create. My comments here are mostly from trying to wrap various spatial libraries that have various types of search paths (e.g., PROJ looking for its database or data files, trying to get GDAL built against PROJ to look in the right place, etc.)...writing bindings to those libraries from R required some flexibility and I was glad the libraries exposed this.

c/include/arrow-adbc/adbc_driver_manager.h

paleolimbot · 2025-06-04T21:04:03Z

c/vendor/toml++/toml.hpp

Just pointing out the change in scope introduced here (the current driver manager is ~2000 lines, and this is ~18,000 lines of vendored parser). Opting out of the driver manager is pretty easy (e.g., how we were able to get GDAL to support ADBC in its default build) and so I think this is OK, but there's a chance it could result in more opting out of the driver manager.

the release build of the driver manager with these changes and updates is only 320K, so I don't think this will necessarily result in people opting out of the driver manager. Unless I'm missing something?

I suppose GDAL didn't want to vendor the driver manager even when it was 2000 lines, so perhaps there's no change here. As another data point, QGIS is using vcpkg where this wouldn't be a problem (as long as it can use the vcpkg version of the toml parser).

I don't know much about vcpkg configs but I can probably set up some option so that it can use the vcpkg version instead of the vendored one? Right?

Yes, I think you can (no need to do it in this PR).

@paleolimbot So, I've also seen smaller toml libraries in C or C++ if that's a concern (but they tend not to be header only) such as https://github.com/cktan/tomlc17

If you think it's worthwhile I can rework this to use a smaller toml library instead.

It sounds like there are a few options if this comes up as an issue (it may not)!

r/adbcdrivermanager/src/Makevars

paleolimbot · 2025-06-04T21:14:44Z

c/driver_manager/adbc_driver_manager.cc

+std::filesystem::path UserConfigDir() {
+  std::filesystem::path config_dir;
+#if defined(_WIN32)
+  auto dir = std::getenv("AppData");


A realistic test for this would be to create a Windows user with a é (e.g.) in the name and ensure that this works (at least that's what happened the last time I had to debug this!)

paleolimbot · 2025-06-04T21:19:45Z

c/driver_manager/adbc_driver_manager.cc

+std::filesystem::path UserConfigDir() {
+  std::filesystem::path config_dir;
+#if defined(_WIN32)
+  auto dir = std::getenv("AppData");


Also, does this need to use the Windows API? (The only other place I've seen this does:)

https://github.com/r-lib/rappdirs/blob/955f2e7997ed6d65cc47a3d77d0cf324c2685897/src/win-path.c#L13

c/driver_manager/adbc_driver_manager.cc

felipecrv · 2025-06-05T01:07:33Z

c/driver_manager/adbc_driver_manager.cc

+std::filesystem::path UserConfigDir() {
+  std::filesystem::path config_dir;
+#if defined(_WIN32)
+  auto dir = std::getenv("AppData");


...create a Windows user with a é (e.g.)

Everyone goes through this ritual with Windows programming, huh? :)

go/adbc/drivermgr/adbc_driver_manager.cc

go/adbc/drivermgr/arrow-adbc/adbc_driver_manager.h

c/include/arrow-adbc/adbc_driver_manager.h

paleolimbot · 2025-06-10T14:55:49Z

c/driver_manager/adbc_driver_manager.cc

+}
+#endif
+
+const std::string& current_arch() {


It may be worth mirroring DuckDB's implementation (since that code has seen some action). (Can also be sorted in a follow-up to improve this later on):

https://github.com/duckdb/duckdb/blob/985150a688889981dd039822174569e262b79b72/src/include/duckdb/common/platform.hpp#L80

I'll take a look at theirs and improve this :)

looks like what they were doing is mostly the same as what I've got here, there were a few extra macros they check so I added those here so that we're basically the same as them

zeroshade · 2025-06-10T15:50:36Z

Hey @kou any ideas on how to fix the std::filesystem issue on almalinux-8? I've been toying around locally trying to add -lstdc++fs but it doesn't seem to fix the linking issue which is odd. Hoping you might have some ideas on how to get it building properly as I really don't want to drop std::filesystem::path and have to do all the file path nonsense myself.

kou · 2025-06-10T21:05:42Z

I think I can fix it. :-)
I'll take a look at it later.

kou · 2025-06-11T02:28:20Z

Fixed:

zeroshade · 2025-06-11T15:19:48Z

@pitrou @paleolimbot @lidavidm Any further comments here?

Co-authored-by: Dewey Dunnington <[email protected]>

Co-authored-by: Sutou Kouhei <[email protected]>

Co-authored-by: David Li <[email protected]>

zeroshade · 2025-06-23T15:45:10Z

Thanks everyone for all the comments and work here! Glad to get this in. I'll follow this up with changes for the docs and updates to the bindings

zeroshade requested a review from lidavidm as a code owner June 3, 2025 20:58

github-actions bot modified the milestone: ADBC Libraries 19 Jun 3, 2025

zeroshade requested review from felipecrv, jbonofre, jduo, paleolimbot and pitrou June 3, 2025 20:59

zeroshade commented Jun 3, 2025

View reviewed changes

zeroshade requested a review from kou as a code owner June 3, 2025 23:18

kou reviewed Jun 4, 2025

View reviewed changes

zeroshade requested a review from CurtHagenlocher June 4, 2025 00:14

lidavidm reviewed Jun 4, 2025

View reviewed changes

c/driver_manager/CMakeLists.txt Outdated Show resolved Hide resolved

c/vendor/toml++/toml.hpp Show resolved Hide resolved

c/include/arrow-adbc/adbc_driver_manager.h Outdated Show resolved Hide resolved

lidavidm reviewed Jun 4, 2025

View reviewed changes

pitrou reviewed Jun 4, 2025

View reviewed changes

c/driver_manager/adbc_driver_manager_test.cc Show resolved Hide resolved

pitrou reviewed Jun 4, 2025

View reviewed changes

go/adbc/drivermgr/adbc_driver_manager.cc Outdated Show resolved Hide resolved

paleolimbot reviewed Jun 4, 2025

View reviewed changes

felipecrv reviewed Jun 5, 2025

View reviewed changes

paleolimbot reviewed Jun 10, 2025

View reviewed changes

zeroshade requested review from felipecrv, paleolimbot and pitrou June 11, 2025 15:19

zeroshade and others added 23 commits June 20, 2025 10:51

ensure we ignore io.EOF in flightsql driver

f7416c7

add more tests

c5ab1da

fix python 3.13 windows build

03f3f49

system-level test with sudo

5030e44

why is postgres tests being weird

6a986e9

lint

04de8c4

Update .github/workflows/native-unix.yml

d0e6e07

Co-authored-by: Dewey Dunnington <[email protected]>

Update c/driver_manager/adbc_driver_manager_test.cc

7ace53a

Co-authored-by: Dewey Dunnington <[email protected]>

more updates from comments

d1cfb5e

linting

f54694b

Update c/cmake_modules/AdbcDefines.cmake

b773074

Co-authored-by: Sutou Kouhei <[email protected]>

fix windows build

fb7ca7e

use shlobj.h instead of shlobj_core.h for R build

c91ef38

windows libs

188fd4b

possibly fix R build

535bae3

Makevars.win R build

d00d8c0

needs -luuid

2dcc385

add link args for python

7e89beb

fix python windows build

4a60ac4

add test with non-ascii filepath

aa73891

Update c/include/arrow-adbc/adbc_driver_manager.h

efa2ee7

Co-authored-by: David Li <[email protected]>

Update c/include/arrow-adbc/adbc_driver_manager.h

96504f4

Co-authored-by: David Li <[email protected]>

update options for consistency

12feba7

zeroshade force-pushed the driver-manager-configs branch from 1579bb4 to 12feba7 Compare June 20, 2025 14:51

zeroshade added 2 commits June 20, 2025 11:02

propagate updated header

60d0d52

updates from comments

480c249

zeroshade merged commit 8e1b028 into apache:main Jun 23, 2025
86 checks passed

zeroshade deleted the driver-manager-configs branch June 23, 2025 15:45

serramatutu mentioned this pull request Aug 12, 2025

Sync upstream dbt-labs/arrow-adbc#52

Closed

feat(c/driver_manager): add new function to allow loading by manifest #2918

feat(c/driver_manager): add new function to allow loading by manifest #2918

Uh oh!

Conversation

zeroshade commented Jun 3, 2025

Uh oh!

zeroshade left a comment

Choose a reason for hiding this comment

Uh oh!

zeroshade left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeroshade commented Jun 3, 2025

Uh oh!

zeroshade commented Jun 3, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

paleolimbot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeroshade left a comment •

edited

Loading