Repository navigation
feat: Decode dictionary delta batches - #955
Conversation
Previously, a DictionaryBatch with isDelta set failed with "Dictionary concatenation is not yet supported" once a dictionary had a non-empty value. Now, delta values are appended to the current dictionary value: a new array is built from the existing value and the delta using ArrowArrayAppendStorageFromArrayView() and replaces the stored dictionary. Arrays decoded before the delta keep referencing the previous dictionary value. Tests cover deltas decoded from a hand-flagged dictionary batch and streams written by Arrow C++ with emit_dictionary_deltas for a range of value types. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
paleolimbot
left a comment
There was a problem hiding this comment.
Thank you! This could use a merge of the latest main for a fresh CI run, but no notes from me! Happy to defer the Codex-generated overflow comment to a more general purpose pass on overflow in the code base if you'd like.
| NANOARROW_RETURN_NOT_OK_WITH_ERROR( | ||
| ArrowArrayReserve(combined, current->length + value->length), error); |
There was a problem hiding this comment.
Codex kindly reminded me here that the two lengths here could overflow, so somebody who has run out of all of the normal ways to make software crash could provide huge null-typed dictionaries and make this unpredictable/crash.
I think we have plenty of places already where we assume two lengths won't overflow an int64; however, if this is easy to fix it would be good to do it here.
There was a problem hiding this comment.
Fixed in 544b501. ArrowIpcDictionaryConcatenate() now checks current->length > INT64_MAX - value->length before adding the lengths, returning EOVERFLOW with both lengths in the error message.
The regression test uses null dictionaries to cover INT64_MAX + 1 and 1 + INT64_MAX through both dictionary decoding APIs. It also checks that rejection preserves the current dictionary and that subsequent replacement and delta decoding still work. With the guard removed, the test reproduces the signed overflow under UBSan; with the fix, it passes.
Also merged the latest main (ea215b0) as requested, triggering fresh CI.
Built and tested on aarch64 Linux EC2, Debug with ZSTD/LZ4:
- Arrow C++ 24.0.0: 857 tests passed, 13 skipped, no failures.
- Without Arrow C++, with UBSan: all 387 tests passed (serial CTest).
- Decoder suite under Valgrind: 301 passed, 10 skipped; zero errors and no leaks.
Formatting checked with clang-format 16.0.6.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #955 +/- ##
==========================================
- Coverage 78.24% 78.21% -0.04%
==========================================
Files 106 106
Lines 16867 16894 +27
Branches 1993 2009 +16
==========================================
+ Hits 13198 13214 +16
Misses 2437 2437
- Partials 1232 1243 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Split out of #928 as requested in review. This PR only adds decoding of dictionary delta batches; the writer changes remain in #928.
Previously,
ArrowIpcDecoderDecodeDictionary()failed with "Dictionary concatenation is not yet supported" when it got a DictionaryBatch withisDeltaset and the current dictionary was non-empty. Now the delta values are appended: a new array is built from the current dictionary value and the delta usingArrowArrayAppendStorageFromArrayView()(#930), and it replaces the stored dictionary. Arrays decoded earlier keep referencing their previous dictionary value. Value types thatArrowArrayAppendStorageFromArrayView()can't append (e.g., unions) fail withENOTSUP.Changes from the review of #928:
if (result == NANOARROW_OK)chains with a helper that doesn't own its inputs, plus a single check and release.common/utils.chelper. Each delta copies the dictionary; appending in place when nothing else references the dictionary can come in a follow-up.enum ArrowTypefor parameters, and the dense union rejection test is removed.This also fixes a double release on the error path of
ArrowIpcDecoderDecodeDictionaryInternal():ArrowArrayMoveShared()already releases its input on failure, so the temporary is now only released if it is still live.Tests:
NanoarrowIpcDecodeDictionaryBatchcovers deltas, including a clone held across a delta keeping its old value. WithNANOARROW_BUILD_TESTS_WITH_ARROW, streams written by Arrow C++ withemit_dictionary_deltas = trueare read back for bool, int64 (with nulls), uint64, double, string, binary, decimal128, list, struct and fixed-size list values. Locally (aarch64 Linux, Debug, ZSTD/LZ4), ctest passes with and without Arrow C++ 21.0.0, and the decoder tests are clean under valgrind.🤖 Generated with Claude Code