Minor: improve zip kernel docs, add examples#6928
Conversation
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
LGTM, minor comment 👍
On a side note, I noticed here that scalars have their length checked:
arrow-rs/arrow-select/src/zip.rs
Lines 46 to 50 in f7263e2
Which seems odd that this check is being done this 'late' so to speak; one would think being able to create a Scalar should enforce this invariant such that we don't have to check it for downstream kernels each time (though I think zip is the only one doing this anyway?)
| /// Zip two arrays by some boolean mask. | ||
| /// | ||
| /// - Where `mask` is `true`, values of `truthy` are taken | ||
| /// - Where `mask` is `false` or `NULL`, values of `falsy` are taken | ||
| /// | ||
| /// # Example | ||
| /// ``` | ||
| /// # use std::sync::Arc; | ||
| /// # use arrow_array::{ArrayRef, BooleanArray, Int32Array}; | ||
| /// # use arrow_select::zip::zip; | ||
| /// // mask: [true, true, false, NULL, true] | ||
| /// let mask = BooleanArray::from(vec![ | ||
| /// Some(true), Some(true), Some(false), None, Some(true) | ||
| /// ]); | ||
| /// // truthy array: [1, NULL, 3, 4, 5] | ||
| /// let truthy = Int32Array::from(vec![ | ||
| /// Some(1), None, Some(3), Some(4), Some(5) | ||
| /// ]); | ||
| /// // falsy array: [10, 20, 30, 40, 50] | ||
| /// let falsy = Int32Array::from(vec![ | ||
| /// Some(10), Some(20), Some(30), Some(40), Some(50) | ||
| /// ]); | ||
| /// // zip with this mask select the first, second and last value from `truthy` | ||
| /// // and the third and fourth value from `falsy` | ||
| /// let result = zip(&mask, &truthy, &falsy).unwrap(); | ||
| /// // Expected: [1, NULL, 30, 40, 5] | ||
| /// let expected: ArrayRef = Arc::new(Int32Array::from(vec![ | ||
| /// Some(1), None, Some(30), Some(40), Some(5) | ||
| /// ])); | ||
| /// assert_eq!(&result, &expected); |
There was a problem hiding this comment.
Would it be necessary to add details on how Scalars interact with this kernel, or is that unnecessary as that should be implied via knowledge of how Scalars behave in general?
There was a problem hiding this comment.
I will research this later likely tomorrow and respond
There was a problem hiding this comment.
Would it be necessary to add details on how Scalars interact with this kernel, or is that unnecessary as that should be implied via knowledge of how Scalars behave in general?
I think in theory it should be implied but I also think it might not be obvious how it works / scalar is not as widely used / supported so I also added a new example use of scalars (to fill in certain elements with a constant)
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
(Forgot to select Approve)
alamb
changed the title
zip kernel docszip kernel docs, add examples
|
Thank you (very much!) for the review @Jefffrey ❤️ |
|
Thanks again @Jefffrey -- I am also happy to add more examples, or updates as a follow on PR |
* Minor: improve `zip` kernel docs` * Add example for zip with scalar
* Minor: improve `zip` kernel docs` * Add example for zip with scalar
* Minor: improve `zip` kernel docs` * Add example for zip with scalar
2 participants
Which issue does this PR close?
Rationale for this change
While reviewing apache/datafusion#13953 from @aweltsch I learned about the
zipkernel but found its documentation somewhat confusinghttps://docs.rs/arrow/latest/arrow/compute/kernels/zip/fn.zip.html
What changes are included in this PR?
Changes
Are there any user-facing changes?
Docs improved. No functional changes
