refactor: Narrow message metadata API - #937
Conversation
…latcc Address review feedback on the Message.custom_metadata support: - Remove ArrowIpcDecoderGetMessageMetadataValue(). Get and Visit already cover the same ground, and every public function is permanent surface; callers who want one key use ArrowMetadataGetValue() on the buffer from ArrowIpcDecoderGetMessageMetadata(). This also removes the _NANOARROW_IPC_VISIT_STOP sentinel, which was not an errno and existed only to unwind that lookup. - Rename ArrowIpcMetadataVisitFunction to ArrowIpcMessageMetadataVisitFunction, scoping the name to the one public entry point that accepts it. - Build the keyless-KeyValue message in NanoarrowIpcDecodeMetadataWithoutKey with flatcc rather than embedding a hand-generated flatbuffer blob, so the test documents itself and survives a flatcc regeneration. Verified that it still segfaults when the strlen() it guards against is restored. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KYFwnKHTw7dhiJeDxy7APV
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #937 +/- ##
==========================================
- Coverage 79.49% 79.47% -0.03%
==========================================
Files 106 106
Lines 16564 16547 -17
Branches 1986 1982 -4
==========================================
- Hits 13167 13150 -17
Misses 2164 2164
Partials 1233 1233 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Fixed the bundled-build failure in cd327f5. The new test included the generated FlatBuffers header through the source include root, which is intentionally absent when NANOARROW_BUNDLE=ON. The header now uses a same-directory include, preserving the bundled include-path check while keeping the flatcc-built fixture. Reproduced locally with NANOARROW_BUNDLE=ON and NANOARROW_IPC=ON: build succeeded and all 365 tests passed (16 codec-dependent tests skipped). |
paleolimbot
left a comment
paleolimbot
left a comment
There was a problem hiding this comment.
I don't particularly mind any of this, but I also didn't mind the hard-coded fixture or the special-cased ArrowIpcDecoderGetMessageMetadataValue() (I don't spend much time with message-level metadata but presumably this would be a reasonably common pattern, like fetching a geo key or something?). Is there a specific motivation for removing it?
|
It was a commit I had in my tree and I think I forgot to push it as a cleanup. |
paleolimbot
left a comment
paleolimbot
left a comment
There was a problem hiding this comment.
Works for me! We can always add the zero copy single key accessor later.
paleolimbot
changed the title
3 participants
Follow-up to #924.
This removes ArrowIpcDecoderGetMessageMetadataValue() before the API ships in a release. Callers that need one key can use ArrowMetadataGetValue() with the packed metadata returned by ArrowIpcDecoderGetMessageMetadata(); callers that need zero-copy iteration can continue using ArrowIpcDecoderVisitMessageMetadata().
It also scopes the visitor callback type as ArrowIpcMessageMetadataVisitFunction and replaces the hand-generated keyless KeyValue FlatBuffer test fixture with a flatcc-built message.
Tests: