Repository navigation
feat(ipc): Add ArrowIpcWriterWriteArrayViewWithDictionaries - #968
Open
rustyconover wants to merge 1 commit into
Open
rustyconover wants to merge 1 commit into
rustyconover wants to merge 1 commit into
Conversation
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 Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ArrowIpcWriterWriteArrayStream()writes the DictionaryBatch messages each array depends on, but only when it is given a wholeArrowArrayStream. A caller that produces batches one at a time and writes them withArrowIpcWriterWriteArrayView()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 callArrowIpcWriterWriteDictionaryBatch()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 stepWriteArrayStream()already performs, andWriteArrayStream()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 asWriteArrayStream(). The doc comment says so rather than promising anything beyond that.Things worth a look in review:
WriteArrayStream()writes dictionaries. It skips a dictionary that is unchanged from the previous batch, and writes nested dictionaries before their parent. Because it does that by keeping the previousArrowArrayalive and comparing buffer addresses, the skip can't apply here: this function only gets a borrowed view. The other feat(ipc): Don't re-emit unchanged dictionaries when writing streams; fix nested dictionary and file writing #928 changes (dependency-first order, file-mode bookkeeping) would apply here too, because the two paths share the helper. Whichever lands second needs a small rebase inwriter.cand inMakeDictionaryStructArray, which I've extended with the samevalue1parameter feat(ipc): Don't re-emit unchanged dictionaries when writing streams; fix nested dictionary and file writing #928 adds. I can do that rebase.inmust not be NULL (a DCHECK, matchingArrowIpcWriterWriteDictionaryBatch()). EOS still goes throughArrowIpcWriterWriteArrayView(writer, NULL, ...).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
ENUMcolumns arrive as dictionary arrays. This is the batch-at-a-time version of whatWriteArrayStream()already does. Together with #967 (ArrowIpcWriterSetMessageMetadata), it covers what the extension needed from Arrow C++'sRecordBatchWriter::WriteRecordBatch().Tests:
WriteArrayViewWithDictionariesMatchesArrayStreamwrites a dictionary-encoded batch through the new function and throughWriteArrayStream(), and checks the bytes are identical.WriteArrayViewWithDictionariesPerBatchwrites 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