alamb commented on code in PR #3450:
URL: https://github.com/apache/arrow-rs/pull/3450#discussion_r1062413249
##########
arrow-array/src/builder/map_builder.rs:
##########
@@ -143,56 +142,49 @@ impl<K: ArrayBuilder, V: ArrayBuilder> MapBuilder<K, V> {
/// Builds the [`MapArray`]
pub fn finish(&mut self) -> MapArray {
let len = self.len();
-
// Build the keys
let keys_arr = self.key_builder.finish();
let values_arr = self.value_builder.finish();
-
- let keys_field = Field::new(
- self.field_names.key.as_str(),
- keys_arr.data_type().clone(),
- false, // always nullable
- );
- let values_field = Field::new(
- self.field_names.value.as_str(),
- values_arr.data_type().clone(),
- true,
- );
-
- let struct_array =
- StructArray::from(vec![(keys_field, keys_arr), (values_field,
values_arr)]);
-
let offset_buffer = self.offsets_builder.finish();
- let null_bit_buffer = self.null_buffer_builder.finish();
self.offsets_builder.append(0);
- let map_field = Box::new(Field::new(
- self.field_names.entry.as_str(),
- struct_array.data_type().clone(),
- false, // always non-nullable
- ));
- let array_data = ArrayData::builder(DataType::Map(map_field, false))
// TODO: support sorted keys
- .len(len)
- .add_buffer(offset_buffer)
- .add_child_data(struct_array.into_data())
- .null_bit_buffer(null_bit_buffer);
-
- let array_data = unsafe { array_data.build_unchecked() };
+ let null_bit_buffer = self.null_buffer_builder.finish();
- MapArray::from(array_data)
+ self.finish_helper(keys_arr, values_arr, offset_buffer,
null_bit_buffer, len)
}
/// Builds the [`MapArray`] without resetting the builder.
pub fn finish_cloned(&self) -> MapArray {
let len = self.len();
-
// Build the keys
let keys_arr = self.key_builder.finish_cloned();
let values_arr = self.value_builder.finish_cloned();
+ let offset_buffer =
Buffer::from_slice_ref(self.offsets_builder.as_slice());
+ let null_bit_buffer = self
+ .null_buffer_builder
+ .as_slice()
+ .map(Buffer::from_slice_ref);
+
+ self.finish_helper(keys_arr, values_arr, offset_buffer,
null_bit_buffer, len)
+ }
+
+ fn finish_helper(
Review Comment:
this is a nice cleanup
##########
arrow-array/src/builder/map_builder.rs:
##########
@@ -143,56 +142,49 @@ impl<K: ArrayBuilder, V: ArrayBuilder> MapBuilder<K, V> {
/// Builds the [`MapArray`]
pub fn finish(&mut self) -> MapArray {
let len = self.len();
-
// Build the keys
let keys_arr = self.key_builder.finish();
let values_arr = self.value_builder.finish();
-
- let keys_field = Field::new(
- self.field_names.key.as_str(),
- keys_arr.data_type().clone(),
- false, // always nullable
- );
- let values_field = Field::new(
- self.field_names.value.as_str(),
- values_arr.data_type().clone(),
- true,
- );
-
- let struct_array =
- StructArray::from(vec![(keys_field, keys_arr), (values_field,
values_arr)]);
-
let offset_buffer = self.offsets_builder.finish();
- let null_bit_buffer = self.null_buffer_builder.finish();
self.offsets_builder.append(0);
- let map_field = Box::new(Field::new(
- self.field_names.entry.as_str(),
- struct_array.data_type().clone(),
- false, // always non-nullable
- ));
- let array_data = ArrayData::builder(DataType::Map(map_field, false))
// TODO: support sorted keys
- .len(len)
- .add_buffer(offset_buffer)
- .add_child_data(struct_array.into_data())
- .null_bit_buffer(null_bit_buffer);
-
- let array_data = unsafe { array_data.build_unchecked() };
+ let null_bit_buffer = self.null_buffer_builder.finish();
- MapArray::from(array_data)
+ self.finish_helper(keys_arr, values_arr, offset_buffer,
null_bit_buffer, len)
}
/// Builds the [`MapArray`] without resetting the builder.
pub fn finish_cloned(&self) -> MapArray {
let len = self.len();
-
// Build the keys
let keys_arr = self.key_builder.finish_cloned();
let values_arr = self.value_builder.finish_cloned();
+ let offset_buffer =
Buffer::from_slice_ref(self.offsets_builder.as_slice());
+ let null_bit_buffer = self
+ .null_buffer_builder
+ .as_slice()
+ .map(Buffer::from_slice_ref);
+
+ self.finish_helper(keys_arr, values_arr, offset_buffer,
null_bit_buffer, len)
+ }
+
+ fn finish_helper(
+ &self,
+ keys_arr: Arc<dyn Array>,
+ values_arr: Arc<dyn Array>,
+ offset_buffer: Buffer,
+ null_bit_buffer: Option<Buffer>,
+ len: usize,
+ ) -> MapArray {
+ assert!(
+ keys_arr.null_count() == 0,
+ "Keys array must have no null values, found {} null value(s)",
+ keys_arr.null_count()
+ );
Review Comment:
In theory the API for adding keys wouldn't allow adding null values for keys
-- but I see that is not possible with the current API
I agree keeping the current (non fallable) API is a reasonable choice
--
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]
