You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2023/01/04 04:31:41 UTC
[GitHub] [arrow-rs] Jefffrey opened a new pull request, #3450: Meaningful error message for map builder with null keys
Jefffrey opened a new pull request, #3450:
URL: https://github.com/apache/arrow-rs/pull/3450
# Which issue does this PR close?
Follow-on from discussion in PR: https://github.com/apache/arrow-rs/pull/3205#discussion_r1033776747
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
Have clearer error message for when attempting to finish a MapBuilder which has null keys. Also do some refactoring.
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb merged pull request #3450: Meaningful error message for map builder with null keys
Posted by GitBox <gi...@apache.org>.
alamb merged PR #3450:
URL: https://github.com/apache/arrow-rs/pull/3450
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] ursabot commented on pull request #3450: Meaningful error message for map builder with null keys
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #3450:
URL: https://github.com/apache/arrow-rs/pull/3450#issuecomment-1374032044
Benchmark runs are scheduled for baseline = 7805a81157152d161c172bbf8ac5ec671568b480 and contender = 39eeeaf62fbee0555c9a5ca925e2f277cba9cbbe. 39eeeaf62fbee0555c9a5ca925e2f277cba9cbbe is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/806bff44ab054133aa45e576975a7641...ab524765396349f4a49c0e0ddcd68e51/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/582b2edac6bb49828e3cbe46230f9eaa...a25871cf39a74237b496ab2934b10d01/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/55bd760e30ab4ffd98c0c47a14afbc93...eda64e5f6ac74a08a216d4334d7ff077/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/fecda82c3855486db0ac236863957692...127ba11c4a7f4ca3a1bb0fcde21d88ba/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Jefffrey commented on a diff in pull request #3450: Meaningful error message for map builder with null keys
Posted by GitBox <gi...@apache.org>.
Jefffrey commented on code in PR #3450:
URL: https://github.com/apache/arrow-rs/pull/3450#discussion_r1061127939
##########
arrow-array/src/array/map_array.rs:
##########
@@ -110,11 +110,12 @@ impl From<MapArray> for ArrayData {
impl MapArray {
fn try_new_from_array_data(data: ArrayData) -> Result<Self, ArrowError> {
- assert!(
- matches!(data.data_type(), DataType::Map(_, _)),
- "MapArray expected ArrayData with DataType::Map got {}",
- data.data_type()
- );
+ if !matches!(data.data_type(), DataType::Map(_, _)) {
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "MapArray expected ArrayData with DataType::Map got {}",
+ data.data_type()
+ )));
+ }
Review Comment:
refactoring out a panic into an Err
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a diff in pull request #3450: Meaningful error message for map builder with null keys
Posted by GitBox <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Jefffrey commented on a diff in pull request #3450: Meaningful error message for map builder with null keys
Posted by GitBox <gi...@apache.org>.
Jefffrey commented on code in PR #3450:
URL: https://github.com/apache/arrow-rs/pull/3450#discussion_r1061128427
##########
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:
ideally would return an Err instead of panicking, but would change signature of finish & finish_cloned, unsure if that is desirable? especially since other structs like StructBuilder has a similar signature (finish doesn't return a Result, just the StructArray)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Jefffrey commented on pull request #3450: Meaningful error message for map builder with null keys
Posted by GitBox <gi...@apache.org>.
Jefffrey commented on PR #3450:
URL: https://github.com/apache/arrow-rs/pull/3450#issuecomment-1370479310
cc @tustvold
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org