feat: add ipc RecordBatch encoding

[bkietz]

• added ArrowIpcEncoderEncodeRecordBatch()
• added ArrowIpcEncoderBuildContiguousBodyBuffer() to provide a default buffer encoder implementation which simply concatenates a batch's buffers into one contiguous (properly aligned and padded) body buffer
• testing uses the decoder tests, replacing arrow C++'s encoder with the new nanoarrow encoder

[paleolimbot]

I know you're still working on this, just a few preliminary things to think about!

In general, I do think we are going to need an ArrowIpcEncoder to avoid painful refactors or excessive numbers of arguments when we add additional features. It may also help collapse status = ArrowXXXX() into NANOARROW_RETURN_NOT_OK() (by making the current flatcc builder something that's cleaned up on ArrowIpcEncoderReset()).

[paleolimbot]

A few more thoughts! Feel free to leave the RecordBatch encoding to a follow-up PR (it would be easier to review, although I'm also happy to review together if that's easier for you!).

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 92.27941% with 21 lines in your changes missing coverage. Please review.

Project coverage is 89.10%. Comparing base (41e9e71) to head (f0025a1).
Report is 38 commits behind head on main.

Files	Patch %	Lines
src/nanoarrow/ipc/encoder.c	93.28%	18 Missing ⚠️
src/nanoarrow/ipc/decoder.c	25.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #555      +/-   ##
==========================================
+ Coverage   88.92%   89.10%   +0.17%     
==========================================
  Files          89       95       +6     
  Lines       16339    15421     -918     
==========================================
- Hits        14530    13741     -789     
+ Misses       1809     1680     -129     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[paleolimbot]

Thanks!

The structure of this is great. I know I said earlier that one big PR was OK, but the scope has enlarged to the point where I would prefer to review as multiple PRs. In particular, it makes it easier to ensure full test coverage for each component (also vaguely better for the changelog). Perhaps:

• Add array view null counter
• Add encoder.c with the encoder infrastructure and test for create/destroy
• Add schema writing
• Add batch writing + unit tests
• Add encoder to the ipc_files test (which is our current infrastructure for sending random batches of various types through our IPC library)

I know that there are some testing patterns and style differences with Arrow C++ that take some getting used to here. I'm absolutely open to changing those; however, if they do change they should change in the rest of the code base too (in dedicated PRs). I think the smaller PRs will help with that (to slowly get used to the existing conventions without having to rewrite things).

[bkietz]

Rebased on #568

[paleolimbot]

Does it make sense to have the codec member of ArrowIpcEncoder? It seems like that is a very simple way of handling compression that will have to change at some point (either to handle more complex ways of choosing which buffers to compress or adding options unrelated to compression).

[bkietz]

I think it suffices for now. Even if we later extend with more advanced heuristics for determining if a buffer should be compressed, the IPC format only allows a single codec to be provided per schema/stream (which means we'll still have only one codec per encoder).

[paleolimbot]

I had in mind something more like how it is specified in Arrow C++ (as a member of an Options struct that includes some other options about IPC writing). I don't see any advantages to premptively adding references to a feature that doesn't exist yet (and might never exist, although I hope that it does).

[bkietz]

I'll remove compression support for now. I'm not sure how an options struct would be advantageous here, but we can hash that out later

[paleolimbot]

Agreed (it might not be useful, or might not be useful here!)

[paleolimbot]

Does schema exist here?

[bkietz]

nope, that's a copy paste error. I found this in another PR. It seems that NANOARROW_DEBUG isn't getting defined

[paleolimbot]

Ah, that would be:

arrow-nanoarrow/CMakeLists.txt

Line 148 in f74d57c

target_compile_definitions(nanoarrow PUBLIC "$<$<CONFIG:Debug>:NANOARROW_DEBUG>")

...that doesn't exist for nanoarrow_ipc. (No need to fix that here since it's not your fault 😬 )

[paleolimbot]

Should be fixed by #573

[paleolimbot]

Is it worth just caching the builder for the RecordBatch message and the int64_t* or ns(FieldNode)* to the start of each (to avoid allocating a builder for every batch?). The decoder requires initializing with a schema to make some allocations that only need to happen once (here an ArrowArrayView would do).

[bkietz]

I'm not sure what you mean; the builder is allocated in Init and only deallocated in Reset. The buffers used to store ns(FieldNode) or ns(Buffer) are also reused for each batch until Reset.

[paleolimbot]

I was wondering if we could avoid creating the new root/fields and copying our cached buffer into it (updating in place instead), but flatcc is probably be very good at anticipating repeated message building and avoiding allocations (and the copy is probably not expensive).

[bkietz]

I'm not sure what state flatcc maintains inside its builders. Some of the example code suggests we could have it maintain independent vectors which are not specifically part of a field which is currently being built. I've tried to use that a few times and gotten odd errors when I strayed from strict state machine style, so I'd prefer to experiment independent of other changes in a follow up.

[paleolimbot]

Agreed!

[paleolimbot]

This is awesome! The negative matches and messages here are also not tested and I am not sure anybody looking to see if we had a utility to help with array equality would look in decoder_test.cc to find it. Something like:

void AssertArrayViewIdentical(actual, expected) {
  NANOARROW_DCHECK(actual->dictionary != nullptr);
  NANOARROW_DCHECK(expected->dictionary != nullptr);

  ASSERT_EQ(actual->storage_type, expected->storage_type);
  ASSERT_EQ(actual->offset, expected->offset);
  ASSERT_EQ(actual->length, expected->length);
  for (int i = 0; i < 3; i++) {
    auto a_buf = actual->buffer_views[i];
    auto e_buf = expected->buffer_views[i];
    ASSERT_EQ(a_buf.size_bytes,  e_buf->size_bytes);
    if (a_buf.size_bytes != 0) {
      ASSERT_EQ(memcmp(a_buf.data.data, e_buf.data.data, a_buf.size_bytes), 0);
    }
  }

  ASSERT_EQ(actual->n_children, expected->n_children);
  for (int i = 0; i < actual->n_children; i++) {
    AssertArrayViewIdentical(actual->children[i], expected->children[i]);
  }
}

...will give terrible error messages but can be followed up with a tested version of this helper (or a version that partly lives in the C library since this comes up in C situations as well).

[bkietz]

can we leave extraction of an array equality helper for a follow up?

[paleolimbot]

Yes! But since you still need something to test the roundtrip, this suggestion was to use something more compact than the equality helper that is currently here.

[paleolimbot]

Thanks! In the meantime I bet we can pin the sha of the action before the change or something (I'll try Monday) (meant for another thread! 😬 )

[paleolimbot]

Thank you!