Skip to content
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

c: add 'helpers' header for C++ users #598

Open
lidavidm opened this issue Apr 21, 2023 · 7 comments
Open

c: add 'helpers' header for C++ users #598

lidavidm opened this issue Apr 21, 2023 · 7 comments
Assignees

Comments

@lidavidm
Copy link
Member

lidavidm commented Apr 21, 2023

Mostly this should contain RAII utilities like those in the PostgreSQL driver. It would make life much easier for C++ users. (There is a question of what to do with errors in Release(); we could make it configurable to throw/ignore/abort perhaps.) We could distribute it as a header alongside the driver manager that can also be copy-pasted.

@lidavidm lidavidm added this to the ADBC Libraries 0.4.0 milestone Apr 21, 2023
@lidavidm
Copy link
Member Author

lidavidm commented Apr 21, 2023

One step further might be to provide an object-oriented wrapper around the C APIs, e.g.

namespace adbc {
class Connection {
public:
  StatusOr<Statement> NewStatement();
};
}

@lidavidm
Copy link
Member Author

Oh, and potentially helpers to work with AdbcStatusCode as arrow::Status or similar...

@paleolimbot
Copy link
Member

For nanoarrow the C++ helpers header has been absolutely essential...without it it's very easy to leak memory when mixed with other C++ idioms (mostly just this part https://github.com/apache/arrow-nanoarrow/blob/main/src/nanoarrow/nanoarrow.hpp#L107-L147 ). That implementation was basically trying to replicate the syntax + move-only semantics of std::unique_ptr<T, Deleter> while avoiding a heap allocation.

I should almost certainly provide a return value helper, too, because the nanoarrow user code that I've read so far does a lot of ignoring of return values. NANOARROW_RETURN_NOT_OK() works pretty well but forces the use of pointer output arguments (e.g., https://github.com/apache/arrow-adbc/blob/main/c/driver/postgresql/postgres_type.h#L252-L254 ).

@lidavidm
Copy link
Member Author

Yeah, if we had std::expected (C++23) we would have a standard library version of arrow::Result, but alas...

@paleolimbot
Copy link
Member

By the time C++23 is an acceptable constraint we'll all be using Rust anyway 🙂

@lidavidm
Copy link
Member Author

Yeah, we're slowly inching to an arrowlib (in analogy to glib etc) which I suppose just happens naturally when your standard library is so small

@paleolimbot
Copy link
Member

I'd like to pick this up soon since it would be very helpful for driver authors to do low level tests on their implementations like:

TEST(TestDriverBase, TestVoidDriverOptions) {

One could also go even further and make something that is ergonomically C++-y, but my immediate use case is something portable that doesn't involve a driver author managing C memory. This is vaguely how writing nanoarrow C++ works: there are still some C function calls, but no C memory management.

There is a question of what to do with errors in Release();

One could either have some "Session" that has a trash can for unreleasable objects (and/or a mechanism for a notice/warning that this has happened), or just stderr and leaking the object (which is probably sufficient for testing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants