Jefffrey commented on code in PR #11625:
URL: https://github.com/apache/arrow-rs/pull/11625#discussion_r4301858821
##########
arrow-select/src/zip.rs:
##########
@@ -814,10 +814,12 @@ impl<T: ByteViewType> ByteViewScalarImpl<T> {
let true_count = predicate.count_set_bits();
let mut buffers: Vec<Buffer> = truthy_buffers.to_vec();
- // If the falsy buffers are empty, we can use the falsy view
as it is, because the value
- // is completely inlined. Otherwise, we have non-inlined
values in the buffer, and we need
- // to recalculate the falsy view
- let view_falsy = if falsy_buffers.is_empty() {
+ // A short falsy value is stored entirely inside its view, so
we can use the
+ // view as it is. We check the value's length rather than
whether the scalar has
+ // buffers, because a scalar cut from a larger array can still
have buffers
+ // even when its own value is short. A longer value lives in a
buffer, so we
+ // need to update its view to point at that buffer's new
position
+ let view_falsy = if (falsy_view as u32) <= MAX_INLINE_VIEW_LEN
{
Review Comment:
```suggestion
// Check if we need to keep falsy buffer in case view isn't
inline
```
can keep it simple
##########
arrow-select/src/zip.rs:
##########
Review Comment:
technically is beyond scope of this PR, but noticed we always keep all
truthy & falsy buffers even though a scalar would reference only one (in each
at worst)
maybe in followup we can fix this too
##########
arrow-select/src/zip.rs:
##########
@@ -1748,4 +1750,83 @@ mod test {
]);
assert_eq!(actual, &expected);
}
+
+ #[test]
+ fn test_zip_kernel_scalar_strings_array_view_sliced_inline() {
+ // Both scalars are inlined, but each slice keeps its array's data
buffer
+ let truthy =
+ StringViewArray::from(vec!["x", "a string longer than twelve
bytes"]).slice(0, 1);
+ let falsy =
+ StringViewArray::from(vec!["hello", "another string longer than
twelve"]).slice(0, 1);
+
+ let mask = BooleanArray::from(vec![true, false, true, false]);
+ let out = zip(&mask, &Scalar::new(&truthy),
&Scalar::new(&falsy)).unwrap();
+ out.to_data().validate_full().unwrap();
+ let actual = out.as_string_view();
+ let expected = StringViewArray::from(vec!["x", "hello", "x", "hello"]);
+ assert_eq!(actual, &expected);
+ }
+
+ #[test]
+ fn test_zip_kernel_scalar_binary_array_view_sliced_inline_short() {
+ // A falsy value of at most 4 bytes keeps zero padding in view bytes 8
to 11
Review Comment:
this seems like a weird test; how does it differ from above (other than
being for binary), and why is there mention of 0 padding?
--
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]
