docs: describe how drivers/driver manager relate #1655
Conversation
docs/source/format/how_manager.rst
Outdated
| .. note:: This document focuses on drivers/applications using the C API | ||
| definitions in adbc.h. That means C/C++/Python/Ruby, and possibly | ||
| C#/Go. |
There was a problem hiding this comment.
Shouldn't all drivers be using the definitions in adbc.h?
There was a problem hiding this comment.
Hmm, maybe we should reword this then? drivers/applications that provide C shared objects?
There was a problem hiding this comment.
It's not necessarily about shared objects either. It's about drivers that use those API definitions/that ABI.
There was a problem hiding this comment.
If they are providing a loadable driver for ADBC, it should be using those API definitions/that ABI. Otherwise it wouldn't be compatible, right?
There was a problem hiding this comment.
But you don't have to have a shared object. You could provide a static library for C/C++ users. You could (as R does) embed multiple driver definitions in one library and not provide the Adbc* functions in the first place.
There was a problem hiding this comment.
It's not related to whether a shared object is involved in the first place
There was a problem hiding this comment.
Hmm. fair, just seems like there's a better way to describe it but I can't think of one at the moment
There was a problem hiding this comment.
Changed to
.. note:: This document focuses on drivers/applications that implement or
consume the C API definitions in adbc.h. That includes C/C++,
Python, and Ruby; and possibly C#, Go, and Rust (when implementing
or consuming drivers via FFI).
There was a problem hiding this comment.
also, Java never uses the header
| Unfortunately, this doesn't work as well in other scenarios. For example, if | ||
| an application wishes to use multiple ADBC drivers, this no longer works: both | ||
| drivers define the same functions (the ones in adbc.h), and when the | ||
| application links both of them, the linker has no way of telling which | ||
| driver's function is meant when the application calls an ADBC function. On | ||
| top of that, this violates the `One Definition Rule`_. |
There was a problem hiding this comment.
If two drivers try to expose both the Adbc.... methods and their own aliases, won't we still get a duplicate symbol issue when dynamically loading both? Should we indicate that the symbols need to be weak symbols to prevent that?
There was a problem hiding this comment.
I don't think that's an issue at this point (well, I suppose it's an issue if you still try and use the symbols), but we don't mark any of our symbols as weak.
There was a problem hiding this comment.
It might end up being an issue in the future that we should keep in mind then at least.
There was a problem hiding this comment.
Can you file a separate issue then?
| ADBC anticipated this, and defined :cpp:struct:`AdbcDriver`. This is just a | ||
| table of function pointers with one entry per ADBC function. That way, an | ||
| application can dynamically load a driver and call an entrypoint function that | ||
| returns this table of function pointers. (It does have to hardcode or guess | ||
| the name of the entrypoint; the ADBC spec lists a set of names it can try, | ||
| based on the name of the driver library itself.) |
There was a problem hiding this comment.
We should probably explain why we didn't just only do this, what benefit do we get from allowing simple case over just requiring this every time
There was a problem hiding this comment.
Well, because you can link to the driver and use it. Do we have to spell it out more than we already did?
There was a problem hiding this comment.
I added this comment before I saw the additional stuff below it. I think you've covered it pretty well
docs/source/format/how_manager.rst
Outdated
| In More Detail | ||
| ============== | ||
|
|
||
| The adbc.h header ties everything together. It is the abstract API |
There was a problem hiding this comment.
Can we add a link to the header file? https://github.com/apache/arrow/blob/main/format/adbc.h
Fixes #1651.