Skip to content

feat: Decode dictionary delta batches - #955

Merged
paleolimbot merged 3 commits into
apache:mainfrom
Query-farm:feat/ipc-dictionary-delta-decoding-v2
Oct 16, 2026
Merged

paleolimbot merged 3 commits into
apache:mainfrom
Query-farm:feat/ipc-dictionary-delta-decoding-v2

Conversation

@rustyconover

Copy link
Copy Markdown
Contributor

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 with isDelta set 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 using ArrowArrayAppendStorageFromArrayView() (#930), and it replaces the stored dictionary. Arrays decoded earlier keep referencing their previous dictionary value. Value types that ArrowArrayAppendStorageFromArrayView() can't append (e.g., unions) fail with ENOTSUP.

Changes from the review of #928:

  • Removed the string/binary-view-specific handling (IPC doesn't support views yet, StringView not supported in IPC reader or writer #824).
  • Replaced the if (result == NANOARROW_OK) chains with a helper that doesn't own its inputs, plus a single check and release.
  • Dropped the "try to unshare" optimization and the internal common/utils.c helper. Each delta copies the dictionary; appending in place when nothing else references the dictionary can come in a follow-up.
  • Tests use enum ArrowType for 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: NanoarrowIpcDecodeDictionaryBatch covers deltas, including a clone held across a delta keeping its old value. With NANOARROW_BUILD_TESTS_WITH_ARROW, streams written by Arrow C++ with emit_dictionary_deltas = true are 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

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 paleolimbot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +369 to +370
NANOARROW_RETURN_NOT_OK_WITH_ERROR(
ArrowArrayReserve(combined, current->length + value->length), error);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 47.05882% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.21%. Comparing base (ea215b0) to head (544b501).

Files with missing lines Patch % Lines
src/nanoarrow/ipc/decoder.c 47.05% 4 Missing and 14 partials ⚠️
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.
📢 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.

@paleolimbot paleolimbot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@paleolimbot paleolimbot changed the title feat(ipc): Decode dictionary delta batches feat: Decode dictionary delta batches Oct 16, 2026
@paleolimbot
paleolimbot merged commit b8a0d3c into apache:main Oct 16, 2026
43 checks passed
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.

3 participants