Jefffrey commented on code in PR #7101:
URL: https://github.com/apache/arrow-rs/pull/7101#discussion_r1948630292
##########
arrow-array/src/builder/map_builder.rs:
##########
@@ -107,13 +108,32 @@ impl<K: ArrayBuilder, V: ArrayBuilder> MapBuilder<K, V> {
field_names: field_names.unwrap_or_default(),
key_builder,
value_builder,
+ key_field: None,
value_field: None,
}
}
/// Override the field passed to [`MapBuilder::new`]
///
- /// By default a nullable field is created with the name `values`
+ /// By default, a nullable field is created with the name `keys`
+ ///
+ /// This function panics if the given field is nullable as map keys are not
+ /// allowed to be null
+ ///
+ /// Note: [`Self::finish`] and [`Self::finish_cloned`] will panic if the
+ /// field's data type does not match that of `K`
+ pub fn with_keys_field(self, field: impl Into<FieldRef>) -> Self {
+ let field: FieldRef = field.into();
+ assert!(!field.is_nullable(), "Key field must not be nullable");
Review Comment:
I think we still need this check; the check in `finish()` only checks for
null values, but the datatype of the key field itself should always be
non-nullable. An array could have no null values but still a datatype with
nullable = true I believe.
I can see how it's awkward for `with_keys_field` to be the only one
returning `Result`, but it makes sense to me since it has a pre-condition to
guard against, and we should take advantage of Rust's type system where
possible instead of documenting panics and hoping users read the documentation.
--
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]
