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

Define the relationship between nanoarrow and C++ #599

Open
bkietz opened this issue Aug 27, 2024 · 11 comments
Open

Define the relationship between nanoarrow and C++ #599

bkietz opened this issue Aug 27, 2024 · 11 comments

Comments

@bkietz
Copy link
Member

bkietz commented Aug 27, 2024

Currently it's not explicit what level of C++ is recommended for use in the nanoarrow project. The library itself is of course a minimal API in pure C, but a C++ helper library is packaged alongside and the unit tests are all C++.

This is part of general tension in the project between ergonomics and minimalism of the API. New helper functions are suspect for fear of bloating the API, but this leads to extremely verbose unit tests and code examples.

WRT using unit tests as documentation: although I think it's useful to have complete and self-contained examples of API usage, I think the priority for unit tests should be coverage rather than didactics.

I'd propose:

  • we survey the community to find out whether the C++ layer is seriously used or could be left internal
    • if the C++ layer is widely used, does strict minimalism apply to that as well or can the C++ layer grow more freely?
    • document these decision in .rst
  • we should write a large body of pure C example code, explicitly freeing up unit tests to be more terse and idiomatic
    • perhaps limiting unit tests to C++17, or relying more on the python layer for unit testing
  • add DSLs available to the tests allowing schema and data literals to be written concisely
@WillAyd
Copy link
Contributor

WillAyd commented Aug 28, 2024

This is a great question. I've documented using the C++ layer myself in some youtube videos on nanoarrow. I've personally been hoping to see it grow a little bit more to make it easier to iterate some of the nanoarrow structures (for example, I think it would be nice to have an iterator for StructArrays that helps you work with dataframe data), but I also don't have a clear point of view on where exactly the line should be drawn

@paleolimbot
Copy link
Member

Definitely a great point that this is not well defined! A few initial thoughts:

this leads to extremely verbose unit tests and code examples.

Things like IPC, ADBC, and integration testing are disproportionately affected here, since they are essentially nanoarrow "users" more so than the nanoarrow C library itself. These are places that there is significant verbosity unrelated to the function being tested (e.g., creating an array to roundtrip through IPC), and where I wish I had things like ArrayFromJSON(). We are a C library and C is verbose, but I think the key to all of this is limiting the C-verbosity to the function being tested.

find out whether the C++ layer is seriously used

I am aware of nanoarrow::UniqueXXX helper usage (Snowflake Python connector, cudf, ADBC) and array stream helpers (ADBC). I recently used the buffer-from-std-vector helpers for some GeoArrow tests also.

does strict minimalism apply to that as well or can the C++ layer grow more freely?

There could definitely be C++ bindings to the C library (or standalone header-only C++ like sparrow)...I don't think that until now there has been anybody with interest in designing and maintaining the interface. C++17 is definitely the way to go here (but there would need to be discussion on the mailing list of whether this is a good idea since it's possibly stepping on Arrow C++'s/sparrow's toes and there would need to be significant interest in contributing/reviewing).

WRT using unit tests as documentation

We definitely should have examples (the fact that we don't have them is just an issue of time) and the intention of the unit testing has never been to replace that. We are a C library, though, and the C library tests will need to have some C library calls in them and should vaguely follow our own advice (e.g., we advise C++ users to use the nanoarrow::UniqueXXX classes to manage cleanup).

@lidavidm
Copy link
Member

WRT using unit tests as documentation: although I think it's useful to have complete and self-contained examples of API usage, I think the priority for unit tests should be coverage rather than didactics.

Just chiming in here, while it's not the goal for here, this might be useful (as janky as it is) https://github.com/apache/arrow-adbc/blob/main/docs/source/ext/adbc_cookbook.py

Demo here: https://arrow.apache.org/adbc/current/cpp/quickstart.html

@eddelbuettel
Copy link
Contributor

Thumbs-up for building on top of nanoarrow for use via C++.

The fact that we have a minimal C interface here is so valuable for more complex build setups as (just about) everybody can use a C-level foreign-function interface. And placing elegant and usable C++ header-only layers on top is then pure magic. I have been told this is called the hour-glass principle. Last time this came up (as well as above) @paleolimbot pointed at sparrow so what would be a differentiator to that (alpha ?) project?

@bkietz
Copy link
Member Author

bkietz commented Aug 29, 2024

IIUC sparrow is designed for consumption exclusively as a C++20 library. Although this makes their API seriously expressive, it makes the library less accessible as an ingredient in other APIs. As I understand it, nanoarrow's priority is ease of consumption of the core C API; a primary example would be its own R and python bindings:

  • (as you said) C APIs are accessible to any binding language's FFI
  • C being entirely unopinonated about lifetimes allows the binding language unambiguous control over objects (no negotiating with a shared_ptr)
  • likewise binding languages are fully capable of expressing iterators and other conveniences by calling C functions directly (without negotiating intervening C++ equivalents)
  • in process built with more than a single binding language, the shared foundation of basic objects makes interoperation between the two languages far simpler to negotiate.

In light of this, I think strictly framing nanoarrow's C++ layer as just one more higher-level language binding makes the most sense. I think it's a goal to provide a usefully, predictably uniform kernel of functions across languages in the same original spirit of the C-ABI itself, almost like an extension of the C-ABI. This is in contrast to sparrow which AFAICT is intended to wrap and use the C-ABI for ergonomic and idiomatic C++20 usage.

We are a C library, though, and the C library tests will need to have some C library calls in them

I'm hesitant to agree with this since it feels that the implication is once again that the unit tests should contain mostly recognizable usages of the API. Instead I'd say test writers should have a free hand to use and extend C++ helpers as needed, so that the only C library call which appears in a given unit test is the function exercised by the test. For example, no unit test in ipc/ should need to call ArrowSchemaInit*; that's tested elsewhere and the test writers should use something less verbose to get their schemas.

this might be useful (as janky as it is) https://github.com/apache/arrow-adbc/blob/main/docs/source/ext/adbc_cookbook.py

I love this. I'll definitely consider borrowing it for writing nanoarrow examples

@paleolimbot
Copy link
Member

this might be useful (as janky as it is) https://github.com/apache/arrow-adbc/blob/main/docs/source/ext/adbc_cookbook.py

I have been looking for a reasonable way to write tutorials/examples for a long time...thank you for writing it/passing this on!

so that the only C library call which appears in a given unit test is the function exercised by the test

We are on the same page here! I do think that leaning too hard on advanced C++ and/or advanced gtest/gmock in tests limits the ability of those not familiar to contribute, but given that most of the existing tests use pretty much no features of C++/gmock/gtest, I think we can get substantial improvement without going there.

strictly framing nanoarrow's C++ layer as just one more higher-level language binding makes the most sense

Definitely on board if there is interest (and it sounds like there is)!

@paleolimbot
Copy link
Member

Do we want wrappers like:

class Buffer {
  public:
    void SetAllocator();
    void Append(int32_t x) { ArrowBufferAppend(obj_.get(), &x, sizeof(x)); }
    
    uint8_t* data() { return obj_.data; }
    int64_t size() { return obj_.size_bytes(); }

 private:
  UniqueBuffer obj_;
}

...in the C++ bindings? If we do, what do we use for error handling?

@WillAyd
Copy link
Contributor

WillAyd commented Nov 1, 2024

Are we definitely locked into C++17? If not, I wonder if it wouldn't be better to return a std::span instead of a data pointer.

As far as error handling is concerned, maybe we could use C++ exceptions? Having used this downstream, I've always found the NANOARROW_THROW_NOT_OK macro to be a nice convenience, so might be nice to continue using that pattern for members that can throw

@bkietz
Copy link
Member Author

bkietz commented Nov 1, 2024

+1 for returning span. FTR, that class can be written in C++11 (arrow-cpp has a polyfill)

@paleolimbot
Copy link
Member

As far as error handling is concerned, maybe we could use C++ exceptions?

I quite like exceptions although I'm vaguely aware that there's a large and vocal contingent who feel differently. There are some pretty serious performance issues that can result from using them inappropriately (e.g., one time I used them for something like StopIteration and it made the framework I'd written unusably slow), but I don't know that any of the types of errors that usually occur from a non-zero ArrowErrorCode fall into that category. If we did go that route we'd have a great workaround (just use the nanoarrow/hpp/unique.hpp header and make C library calls directly).

Are we definitely locked into C++17?

Anything that we want to use in our own tests still has to be C++11 (we still have a rather important user, DuckDB, who supports C++11, so I think it's reasonable that our tests still run), but we can do things with higher standards if they are fenced with an opt-out define. I don't think we have any users interested in helpful C++ wrappers that are still using C++11 but we do have users that are specifically C++17 that might be (cudf, ADBC), and so I think anything higher than that is maybe not all that useful (but maybe possible with an opt-in define).

I wonder if it wouldn't be better to return a std::span instead of a data pointer.

+1! It looks like that's our ArrowBufferView

@paleolimbot
Copy link
Member

After #668 merges I'll put together a draft PR. Maybe a better place to start would be the Schema wrapper since most errors that occur there are very small allocation failures of the variety that even non-exception using C++ libraries would end up throwing std::bad_alloc.

paleolimbot added a commit that referenced this issue Nov 1, 2024
This PR splits `nanoarrow.hpp`, which was getting large, into multiple
files (bundling them, of course, when creating the bundled
distribution). This is intended to make space for new helpers that are
being proposed in #599 although I think it's also just a good idea in
general since the utilities to create streams and buffers using
templating are in a pretty different category from the things that keep
you from leaking memory and many users will only need part of the API.
Because it's header-only on top of the C runtime, I think this is
probably a good pattern to enable.

This doesn't quite solve `nanoarrow_device.hpp` and
`nanoarrow_ipc.hpp`...they currently still just include all of
`nanoarrow.hpp`. I think we can probably solve that in the bundler but I
might punt on that since it's not actually hurting anything at the
moment.
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

5 participants