Copilot commented on code in PR #9141:
URL: https://github.com/apache/arrow-rs/pull/9141#discussion_r2679776755
##########
arrow-array/src/array/byte_array.rs:
##########
@@ -154,6 +154,30 @@ impl<T: ByteArrayType> GenericByteArray<T> {
})
}
+ /// It returns a new array with the same data and a new null buffer.
Review Comment:
The documentation starts with "It returns" which is grammatically awkward.
The sentence should start with "Returns" to match the style of the
PrimitiveArray implementation and follow Rust documentation conventions.
##########
arrow-array/src/array/byte_array.rs:
##########
@@ -154,6 +154,30 @@ impl<T: ByteArrayType> GenericByteArray<T> {
})
}
+ /// It returns a new array with the same data and a new null buffer.
+ ///
+ /// The resulting null buffer is the union of the existing nulls and the
provided nulls.
+ /// In other words, a slot is valid in the result only if it is valid in
BOTH
+ /// the existing array AND the provided `nulls`.
+ ///
+ /// # Panics
+ ///
+ /// Panics if `nulls` has a different length than the array.
+ pub fn with_nulls(self, nulls: Option<NullBuffer>) -> Self {
+ if let Some(n) = &nulls {
+ assert_eq!(n.len(), self.len(), "Null buffer length mismatch");
+ }
+
+ let new_nulls = NullBuffer::union(self.nulls.as_ref(), nulls.as_ref());
+
+ Self {
+ data_type: T::DATA_TYPE,
+ value_offsets: self.value_offsets,
+ value_data: self.value_data,
+ nulls: new_nulls,
+ }
+ }
Review Comment:
The new with_nulls method lacks test coverage. Consider adding tests to
verify behavior such as:
- Merging nulls with an array that has no existing nulls
- Merging nulls with an array that already has nulls
- Passing None as the nulls parameter
- Verifying the panic behavior when null buffer length mismatches
Similar methods in this file have comprehensive test coverage, and tests
would help ensure this method works correctly and prevent regressions.
##########
arrow-array/src/array/byte_array.rs:
##########
@@ -154,6 +154,30 @@ impl<T: ByteArrayType> GenericByteArray<T> {
})
}
+ /// It returns a new array with the same data and a new null buffer.
+ ///
+ /// The resulting null buffer is the union of the existing nulls and the
provided nulls.
+ /// In other words, a slot is valid in the result only if it is valid in
BOTH
+ /// the existing array AND the provided `nulls`.
+ ///
Review Comment:
The documentation for GenericByteArray::with_nulls lacks a usage example,
unlike PrimitiveArray::with_nulls which includes a comprehensive example.
Adding an example would improve consistency across the API and help users
understand how to use this method effectively.
```suggestion
///
/// # Examples
///
/// ```
/// use arrow_array::StringArray;
/// use arrow_buffer::NullBuffer;
///
/// // Create an array with an existing null in the second position
/// let array = StringArray::from(vec![Some("a"), None, Some("c")]);
///
/// // Create an additional null buffer to combine with the existing one
/// // Here, the third position is marked as null
/// let nulls = NullBuffer::from(vec![true, true, false]);
///
/// let result = array.with_nulls(Some(nulls));
///
/// assert_eq!(result.len(), 3);
/// // Still valid, as it is valid in both null buffers
/// assert!(result.is_valid(0));
/// // Remains null, as it is null in the original array
/// assert!(result.is_null(1));
/// // Now null, as it is null in the provided null buffer
/// assert!(result.is_null(2));
/// ```
///
```
##########
arrow-array/src/array/boolean_array.rs:
##########
@@ -93,6 +93,28 @@ impl BooleanArray {
Self { values, nulls }
}
+ /// It returns a new array with the same data and a new null buffer.
+ ///
+ /// The resulting null buffer is the union of the existing nulls and the
provided nulls.
+ /// In other words, a slot is valid in the result only if it is valid in
BOTH
+ /// the existing array AND the provided `nulls`.
+ ///
Review Comment:
The documentation for BooleanArray::with_nulls lacks a usage example, unlike
PrimitiveArray::with_nulls which includes a comprehensive example. Adding an
example would improve consistency across the API and help users understand how
to use this method effectively.
```suggestion
///
/// # Example
///
/// ```
/// # use arrow_array::{Array, BooleanArray};
/// # use arrow_buffer::NullBuffer;
/// let array = BooleanArray::from(vec![true, false, true]);
/// let nulls = NullBuffer::from(vec![true, false, true]);
///
/// let array = array.with_nulls(Some(nulls));
///
/// assert_eq!(array.len(), 3);
/// assert!(array.is_valid(0));
/// assert!(array.is_null(1));
/// ```
///
```
##########
arrow-array/src/array/boolean_array.rs:
##########
@@ -93,6 +93,28 @@ impl BooleanArray {
Self { values, nulls }
}
+ /// It returns a new array with the same data and a new null buffer.
+ ///
+ /// The resulting null buffer is the union of the existing nulls and the
provided nulls.
+ /// In other words, a slot is valid in the result only if it is valid in
BOTH
+ /// the existing array AND the provided `nulls`.
+ ///
+ /// # Panics
+ ///
+ /// Panics if `nulls` has a different length than the array.
+ pub fn with_nulls(self, nulls: Option<NullBuffer>) -> Self {
+ if let Some(n) = &nulls {
+ assert_eq!(n.len(), self.len(), "Null buffer length mismatch");
+ }
+
+ let new_nulls = NullBuffer::union(self.nulls.as_ref(), nulls.as_ref());
+
+ Self {
+ values: self.values,
+ nulls: new_nulls,
+ }
+ }
Review Comment:
The new with_nulls method lacks test coverage. Consider adding tests to
verify behavior such as:
- Merging nulls with an array that has no existing nulls
- Merging nulls with an array that already has nulls
- Passing None as the nulls parameter
- Verifying the panic behavior when null buffer length mismatches
Similar methods in this file have comprehensive test coverage, and tests
would help ensure this method works correctly and prevent regressions.
##########
arrow-array/src/array/byte_array.rs:
##########
@@ -154,6 +154,30 @@ impl<T: ByteArrayType> GenericByteArray<T> {
})
}
+ /// It returns a new array with the same data and a new null buffer.
+ ///
+ /// The resulting null buffer is the union of the existing nulls and the
provided nulls.
+ /// In other words, a slot is valid in the result only if it is valid in
BOTH
+ /// the existing array AND the provided `nulls`.
+ ///
+ /// # Panics
+ ///
+ /// Panics if `nulls` has a different length than the array.
+ pub fn with_nulls(self, nulls: Option<NullBuffer>) -> Self {
+ if let Some(n) = &nulls {
+ assert_eq!(n.len(), self.len(), "Null buffer length mismatch");
+ }
+
+ let new_nulls = NullBuffer::union(self.nulls.as_ref(), nulls.as_ref());
+
+ Self {
+ data_type: T::DATA_TYPE,
Review Comment:
For consistency with PrimitiveArray::with_nulls and to future-proof the
implementation, this should use `self.data_type` instead of `T::DATA_TYPE`.
While they are currently always the same for GenericByteArray, PrimitiveArray
supports overriding the data type via `with_data_type`, and using
`self.data_type` would maintain the pattern of preserving the original
instance's data type field.
```suggestion
data_type: self.data_type,
```
##########
arrow-array/src/array/boolean_array.rs:
##########
@@ -93,6 +93,28 @@ impl BooleanArray {
Self { values, nulls }
}
+ /// It returns a new array with the same data and a new null buffer.
Review Comment:
The documentation starts with "It returns" which is grammatically awkward.
The sentence should start with "Returns" to match the style of the
PrimitiveArray implementation and follow Rust documentation conventions.
```suggestion
/// Returns a new array with the same data and a new null buffer.
```
##########
arrow-array/src/array/primitive_array.rs:
##########
@@ -672,6 +672,42 @@ impl<T: ArrowPrimitiveType> PrimitiveArray<T> {
})
}
+ /// Returns a new array with the same data and a new null buffer.
+ ///
+ /// The resulting null buffer is the union of the existing nulls and the
provided nulls.
+ /// In other words, a slot is valid in the result only if it is valid in
BOTH
+ /// the existing array AND the provided `nulls`.
+ ///
+ /// # Panics
+ ///
+ /// Panics if `nulls` has a different length than the array.
+ ///
+ /// # Example
+ ///
+ /// ```
+ /// # use arrow_array::{Int32Array, Array};
+ /// # use arrow_buffer::NullBuffer;
+ /// let array = Int32Array::from(vec![Some(1), Some(2), Some(3)]);
+ /// // Mask out the second element
+ /// let mask = NullBuffer::from(vec![true, false, true]);
+ /// let masked = array.with_nulls(Some(mask));
+ /// assert_eq!(masked.values(), &[1, 2, 3]); // Values unchanged
+ /// assert!(masked.is_null(1)); // Second element is now null
+ /// ```
+
+ pub fn with_nulls(self, nulls: Option<NullBuffer>) -> Self {
+ if let Some(n) = &nulls {
+ assert_eq!(n.len(), self.len(), "Null buffer length mismatch");
+ }
+
+ let new_nulls = NullBuffer::union(self.nulls.as_ref(), nulls.as_ref());
+
+ Self {
+ data_type: self.data_type,
+ values: self.values,
+ nulls: new_nulls,
+ }
+ }
Review Comment:
The new with_nulls method lacks test coverage. Consider adding tests to
verify behavior such as:
- Merging nulls with an array that has no existing nulls
- Merging nulls with an array that already has nulls
- Passing None as the nulls parameter
- Verifying the panic behavior when null buffer length mismatches
Similar methods in this file have comprehensive test coverage, and tests
would help ensure this method works correctly and prevent regressions.
--
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]
