alamb commented on code in PR #9445:
URL: https://github.com/apache/arrow-rs/pull/9445#discussion_r3227342611
##########
arrow-array/src/record_batch.rs:
##########
@@ -451,6 +516,46 @@ impl RecordBatch {
&mut schema.metadata
}
+ /// Returns the per-batch custom metadata, or `None` if not set.
+ ///
+ /// This corresponds to the `custom_metadata` field on the IPC `Message`
+ /// flatbuffer, separate from schema-level metadata.
+ pub fn custom_metadata(&self) -> Option<&HashMap<String, String>> {
+ self.custom_metadata.as_deref()
+ }
+
+ /// Returns a mutable reference to the per-batch custom metadata,
allocating
+ /// an empty map on first access.
+ ///
+ /// Cheap if this [`RecordBatch`] uniquely owns the metadata; otherwise the
+ /// underlying map is cloned via [`Arc::make_mut`]. An empty map left after
+ /// clearing entries still reports as `Some(_)` from
+ /// [`Self::custom_metadata`]; two batches that differ only in this respect
+ /// still compare equal.
+ pub fn custom_metadata_mut(&mut self) -> &mut HashMap<String, String> {
+ Arc::make_mut(
+ self.custom_metadata
+ .get_or_insert_with(|| Arc::new(HashMap::new())),
+ )
+ }
+
+ /// Sets the per-batch custom metadata, returning `self`.
+ ///
+ /// Takes an [`Arc`] so the same metadata map can be shared across many
+ /// [`RecordBatch`]es without cloning. Callers with a fresh `HashMap`
+ /// can wrap it via [`Arc::new`].
+ ///
+ /// An empty map is normalized to "no metadata", so a batch built with an
+ /// empty map compares equal to one built without calling this method.
+ pub fn with_custom_metadata(mut self, metadata: CustomMetadata) -> Self {
+ self.custom_metadata = if metadata.is_empty() {
+ None
Review Comment:
I am confused -- are you trying to enforce the invariant that
`custom_metadata` never is `Some(empty_map)`? If so
If so, then the custom PartialEq implementation is not needed -- as it seems
you are now enforcing the invariant empty metadata is none.
I would personally suggest not trying to be fancy here, and instead just do
```rust
pub fn with_custom_metadata(mut self, metadata: Option<CustomMetadata>)
-> Self {
```
Which makes the caller have to decide how they want to treat None vs an
Empty hash map. Also, it would give people an escape valvue to set the metadata
exactly as they wanted
##########
arrow-array/src/record_batch.rs:
##########
@@ -451,6 +516,46 @@ impl RecordBatch {
&mut schema.metadata
}
+ /// Returns the per-batch custom metadata, or `None` if not set.
+ ///
+ /// This corresponds to the `custom_metadata` field on the IPC `Message`
+ /// flatbuffer, separate from schema-level metadata.
+ pub fn custom_metadata(&self) -> Option<&HashMap<String, String>> {
Review Comment:
I think this should return `Option<&Arc<HashMap<String, String>>>` so that
callers can clone the arc if they want
```suggestion
pub fn custom_metadata(&self) -> Option<&Arc<HashMap<String, String>>> {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
