Skip to content

feat(ipc): Add ArrowIpcWriterWriteArrayViewWithDictionaries - #968

Open
rustyconover wants to merge 1 commit into
apache:mainfrom
rustyconover:feat/ipc-writer-write-array-view-with-dictionaries
Open

rustyconover wants to merge 1 commit into
apache:mainfrom
rustyconover:feat/ipc-writer-write-array-view-with-dictionaries

Conversation

@rustyconover

Copy link
Copy Markdown
Contributor

ArrowIpcWriterWriteArrayStream() writes the DictionaryBatch messages each array depends on, but only when it is given a whole ArrowArrayStream. A caller that produces batches one at a time and writes them with ArrowIpcWriterWriteArrayView() has no public way to get those dictionaries written. It would have to work out the dictionary IDs itself, matching the depth-first order the schema encoder assigns them in, and call ArrowIpcWriterWriteDictionaryBatch() for each one.

This PR adds ArrowIpcWriterWriteArrayViewWithDictionaries(writer, view, error). It writes a DictionaryBatch for each dictionary-encoded field of the view and then the RecordBatch. That is the same per-array step WriteArrayStream() already performs, and WriteArrayStream() now calls the new function, so there is one code path. The behavior is unchanged: every dictionary is written in full before every RecordBatch, and file mode has the same limits as WriteArrayStream(). The doc comment says so rather than promising anything beyond that.

Things worth a look in review:

Motivation: I'm moving the IPC layer of a DuckDB extension from Arrow C++ to nanoarrow. It writes one batch at a time to a worker process, and DuckDB ENUM columns arrive as dictionary arrays. This is the batch-at-a-time version of what WriteArrayStream() already does. Together with #967 (ArrowIpcWriterSetMessageMetadata), it covers what the extension needed from Arrow C++'s RecordBatchWriter::WriteRecordBatch().

Tests:

  • WriteArrayViewWithDictionariesMatchesArrayStream writes a dictionary-encoded batch through the new function and through WriteArrayStream(), and checks the bytes are identical.
  • WriteArrayViewWithDictionariesPerBatch writes two batches with different dictionaries one at a time. It checks the message sequence (schema, dictionary, batch, dictionary, batch) and that the reader resolves each batch against its own dictionary.

ctest passes locally (macOS arm64, Debug, ZSTD/LZ4, 390 tests). One run hit a single failure in NanoarrowIntegrationTestBatch, which doesn't touch the writer. It didn't recur in isolation or in five more full runs. The IPC writer and reader tests are clean under ASan/UBSan.

🤖 Generated with Claude Code

A writer that produces batches one at a time, rather than from an
ArrowArrayStream, had no public way to write the DictionaryBatch messages a
dictionary-encoded batch depends on: ArrowIpcWriterWriteArrayStream() did
this internally, but only for a whole stream. Expose that per-batch path,
which ArrowIpcWriterWriteArrayStream() now uses as well.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.24%. Comparing base (e5f3d0a) to head (c450bdf).

Files with missing lines Patch % Lines
src/nanoarrow/ipc/writer.c 60.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #968   +/-   ##
=======================================
  Coverage   78.24%   78.24%           
=======================================
  Files         106      106           
  Lines       16901    16902    +1     
  Branches     2012     2012           
=======================================
+ Hits        13224    13225    +1     
  Misses       2435     2435           
  Partials     1242     1242           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

This branch has not been deployed

No deployments
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

Successfully merging this pull request may close these issues.

2 participants