Repository navigation
GH-49704: [C++] Read and write DictionaryEncoding.custom_metadata for dictionary value types - #53994
rustyconover wants to merge 3 commits into
Conversation
|
|
401634e to
0cd8838
Compare
…ionary values Implements the draft Arrow format 1.6 change from apache/arrow#49704 (apache/arrow#53994): an optional custom_metadata on DictionaryEncoding that holds the metadata of the dictionary value type. It is the IPC counterpart of ArrowSchema.dictionary->metadata, and Field.custom_metadata corresponds to the parent ArrowSchema's metadata. - flatcc_generated.h is regenerated with dev/update_fbs.sh (flatcc e3e44836) from apache/arrow main plus the draft Schema.fbs change. - The encoder writes schema->dictionary->metadata (all keys) to DictionaryEncoding.custom_metadata, omitted when it is NULL or empty, and keeps writing schema->metadata to Field.custom_metadata. Before, the value metadata was dropped. - The decoder sets schema->dictionary->metadata from DictionaryEncoding.custom_metadata and keeps Field.custom_metadata on the parent schema, as before. This covers nested dictionaries and footer decoding. Footer encoding carries the member when the caller fills footer.dictionaries. - The integration JSON reader and writer support the optional "metadata" of a field's "dictionary" object, and the JSON comparison compares the dictionary value metadata, including nested dictionaries. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| if (encoding->custom_metadata() != nullptr) { | ||
| ARROW_ASSIGN_OR_RAISE(auto value_metadata, | ||
| internal::GetKeyValueMetadata(encoding->custom_metadata())); | ||
| RETURN_NOT_OK(MaybeDeserializeExtensionType(value_metadata.get(), &type)); |
There was a problem hiding this comment.
I am wondering whether there would be a way of retaining this value_metadata even if this is a not known extension type. So the reader could retain it and a new writer would write it. I am thinking on the use case of data passing through:
app knows extension type -> app does not know extension type -> app knows extension type
Maybe not entirely related and just an implementation detail that can be done as a follow up.
There was a problem hiding this comment.
Hi @raulcd,
Do you think we need to make a different IPC reader options flag for that? Where we can do extension type pass through?
Rusty
|
@rustyconover Can you split the Format changes into a separate PR to make them easier to review? Thanks! |
Sure I can and I will! thanks @pitrou |
… value type In IPC, a dictionary-encoded field is a single Field whose type is the dictionary value type, and the only metadata slot was Field.custom_metadata. As a result, dictionary<values=extension<X>> and extension<X, storage=dictionary<...>> were serialized identically. Add an optional custom_metadata member at the end of the DictionaryEncoding table. It holds the custom metadata of the dictionary value type, with the same semantics as Field.custom_metadata (including the reserved ARROW:extension:* keys), and is the IPC counterpart of ArrowSchema.dictionary->metadata in the C Data Interface. Document it in the columnar format (format version 1.6), the versioning history, the C Data Interface, and the integration JSON format, which gains a matching optional "metadata" member in the "dictionary" object. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ta for dictionary value types Implement the DictionaryEncoding.custom_metadata format addition in C++. The IPC writer now puts the extension keys of a dictionary value type there, and the reader deserializes the value extension type from it before wrapping it in the dictionary type and applying Field.custom_metadata as before. Regenerate Schema_generated.h, support the "metadata" member of the "dictionary" object in the integration JSON reader and writer, and add a "dictionary_extension" archery case, which C++ and Go produce and consume (Go skips the C schema format, Rust is skipped as it can read but not write the case). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
0cd8838 to
b267de8
Compare
|
@pitrou Done: the format change is now in #54959 (issue #54958). This PR is stacked on it and contains only the C++ implementation, so please review just the second commit. Note that #54959 was auto-closed by the 3-open-PR limit for contributors without repository access; I've asked on it for it to be reopened. |
… trips Retain unregistered extension annotations and arbitrary value metadata on DictionaryType through IPC, integration JSON and the C Data Interface. Preserve metadata-aware equality and schema promotion, avoid conflicting reserved extension keys, and traverse nested dictionaries inside extension values. Validate unknown-extension pass-through and restoration after registration, nested dictionaries, binary metadata and unchanged field metadata. All 826 affected C++ tests and C++/Go IPC interoperability checks pass.
…ionary values Implements the draft Arrow format 1.6 change from apache/arrow#49704 (apache/arrow#53994): an optional custom_metadata on DictionaryEncoding that holds the metadata of the dictionary value type. It is the IPC counterpart of ArrowSchema.dictionary->metadata, and Field.custom_metadata corresponds to the parent ArrowSchema's metadata. - flatcc_generated.h is regenerated with dev/update_fbs.sh (flatcc e3e44836) from apache/arrow main plus the draft Schema.fbs change. - The encoder writes schema->dictionary->metadata (all keys) to DictionaryEncoding.custom_metadata, omitted when it is NULL or empty, and keeps writing schema->metadata to Field.custom_metadata. Before, the value metadata was dropped. - The decoder sets schema->dictionary->metadata from DictionaryEncoding.custom_metadata and keeps Field.custom_metadata on the parent schema, as before. This covers nested dictionaries and footer decoding. Footer encoding carries the member when the caller fills footer.dictionaries. - The integration JSON reader and writer support the optional "metadata" of a field's "dictionary" object, and the JSON comparison compares the dictionary value metadata, including nested dictionaries. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Note
Depends on #54959, which holds the format change, split out at review request. This PR is stacked on it: please review the C++ implementation commits after the format commit. Once #54959 merges, this will be rebased onto main.
Rationale for this change
In IPC a dictionary-encoded column is a single
Fieldwhose type is the value type. The only metadata slot wasField.custom_metadata, sodictionary<values=extension<X>>andextension<X, storage=dictionary<...>>serialized identically. Arrow C++ reads both as the latter, so pyarrow cannot read back its own stream ofpa.dictionary(pa.int32(), pa.uuid()):Invalid storage type for UuidType: dictionary<...>.#54959 adds an optional
custom_metadatatoDictionaryEncodingfor the dictionary value type (format version 1.6). This PR is the C++ reference implementation. Companion PRs:Fieldmetadata until Field metadata for dictionary-encoded extension types is lost arrow-rs#7982 is fixed: Addcustom_metadatato IPCDictionaryEncodingarrow-rs#11622ArrowSchema.dictionary->metadata: feat(ipc): Read and write DictionaryEncoding.custom_metadata for dictionary values arrow-nanoarrow#972What changes are included in this PR?
Schema_generated.hregenerated with flatc 24.3.6, limited to the new member."dictionary": {..., "metadata": [...]}).ARROW:extension:*keys inDictionaryEncoding.custom_metadata.Field.custom_metadataas before.DictionaryType::value_metadata()retains unconsumed metadata, including unregistered extension annotations and arbitrary keys, separately from the enclosing field. IPC and integration JSON write it back to the dictionary encoding. No reader option is needed.ArrowSchema.dictionary->metadata.dictionary_extensioncase, run for C++ only for now.ruby/red-arrow-format/.../dictionary_encoding.rb) were not regenerated, because no Ruby toolchain was available. They needrake flat_buffers:generate.ARROW:schemaround-trip test.Are these changes tested?
Yes.
e3ee6e33604767c2d3122b1c2ea4336c3ae96f06: repeated C++ → Go → C++ and Go → C++ → Go IPC file/JSON conversions preserve unknown extension annotations, ordinary metadata (including duplicate keys and NUL bytes), separate parent metadata, nested fields and dictionary data.Are there any user-facing changes?
Yes:
pa.dictionary(pa.int32(), pa.uuid())) through IPC.Field.custom_metadatatoDictionaryEncoding.custom_metadata.arrow.opaque, which older readers used to keep, though with the wrong nesting (as an extension type with dictionary storage).arrow.uuidon a dictionary still fails to read, as it does today.Was AI used for this PR?
PR code and description written by:
Latest follow-up reviewed before submission by:
Initial implementation generated with Claude Code; metadata-preservation follow-up prepared with Codex.