-
Notifications
You must be signed in to change notification settings - Fork 95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs(c/driver/framework): Add documentation for building a driver using the driver framework #2186
Conversation
ci/scripts/cpp_recipe_driver.sh
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm not mistaken we already have cpp_recipe.sh for testing? Can we just fold all the test scripts into one?
(We could even have a single parent CMake that adds the quickstart/driver example as child CMakes, though I can understand if you want the driver example to be fully self-contained there)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a self-contained CMakeLists.txt is more realistic although I'm open to any suggestions here! I mostly just tried to emulate the existing art here which is great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I'm mostly just saying, we can just have a single shell script to invoke all the CMake builds for recipes. Not too big deal to me though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I did this (but happy to restructure as required!)
d6bebfd
to
84403a0
Compare
84403a0
to
24ab3d7
Compare
../../../../c/include) | ||
target_link_libraries(adbc_driver_framework PRIVATE nanoarrow::nanoarrow) | ||
|
||
# TODO: Do we want any symbol visiblity presets here to ensure that the only one exposed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be nice but we don't have this worked out in the main project yet either do we? Maybe worth punting until then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed!
target_link_libraries(driver_example PRIVATE adbc_driver_framework | ||
nanoarrow::nanoarrow_ipc) | ||
|
||
# TODO: Do we want to have this as part of the example? We could make the validation library |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am +1 to including this in the example - I think the test is helpful to read
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok! I leaned into this comment and made it a part of the recipe!
# TODO: This currently depends on about-to-be-released nanoarrow, which allows | ||
# access to both nanoarrow and the IPC reader using a single fetchcontent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we update this now?
/// Next, we'll bring a few essential framework types into the namespace | ||
/// to reduce the verbosity of the implementation: | ||
/// | ||
/// * ``Option``: Options can be set on an ADBC database, connection, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think something like
:cpp:class:`adbc::driver::Option`
should work now (and eventually result in a proper link to the API docs from the prose docs), though no big deal either way (it can be finicky to set up and test locally)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll give it a try!
Hmm, there's something not right with the scrape...I'll see if I can take a look too
|
I ran out of time today to debug but I can take a look tomorrow too! |
I took a look and interestingly, Status::Impl is documented in the XML, but not in the HTML. |
Ok I think the main issue is that I just haven't implemented enough to also scrape C++ headers well... |
Ok, I think that should fix the main things... |
Sorry, I pushed a bunch of crap here...let's hope the docs work now 😅 |
This PR adds a tutorial with how to build a basic driver using the C++ framework. You can read the recipe by downloading the docs.tgz asset from the CI job:
https://github.com/apache/arrow-adbc/actions/runs/11431710883/artifacts/2080179525
Example driver in action: