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 2022/03/20 12:59:52 UTC
[GitHub] [arrow-rs] HaoYang670 opened a new pull request #1465: Support the `length` kernel on Binary Array
HaoYang670 opened a new pull request #1465:
URL: https://github.com/apache/arrow-rs/pull/1465
# Which issue does this PR close?
Closes #1464.
# Rationale for this change
The `length` kernel can work with `BinaryArray` now!.
# What changes are included in this PR?
1. add `length` and `bit_length` functions for Binary Array
2. rewrite `unary_offsets` using macro
3. add tests.
# Are there any user-facing changes?
Some private functions' APIs are changed.
--
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] HaoYang670 commented on pull request #1465: Support the `length` kernel on Binary Array
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on pull request #1465:
URL: https://github.com/apache/arrow-rs/pull/1465#issuecomment-1075847535
@alamb Could you please help to review?
--
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 change in pull request #1465: Support the `length` kernel on Binary Array
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1465:
URL: https://github.com/apache/arrow-rs/pull/1465#discussion_r833584111
##########
File path: arrow/src/compute/kernels/length.rs
##########
@@ -15,59 +15,62 @@
// specific language governing permissions and limitations
// under the License.
-//! Defines kernel for length of a string array
+//! Defines kernel for length of string arrays and binary arrays
-use crate::{
- array::*,
- buffer::Buffer,
- datatypes::{ArrowNativeType, ArrowPrimitiveType},
-};
+use crate::{array::*, buffer::Buffer, datatypes::ArrowPrimitiveType};
use crate::{
datatypes::{DataType, Int32Type, Int64Type},
error::{ArrowError, Result},
};
-fn unary_offsets_string<O, F>(
- array: &GenericStringArray<O>,
- data_type: DataType,
- op: F,
+macro_rules! unary_offsets {
+ ($array: expr, $data_type: expr, $op: expr) => {{
+ // note: offsets are stored as u8, but they can be interpreted as OffsetSize
Review comment:
I know this comment was just copy/pasted but I don't think it is accurate -- the offsets are i32 or i64
--
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] viirya commented on a change in pull request #1465: Support the `length` kernel on Binary Array
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1465:
URL: https://github.com/apache/arrow-rs/pull/1465#discussion_r833848650
##########
File path: arrow/src/compute/kernels/length.rs
##########
@@ -115,13 +134,15 @@ pub fn length(array: &dyn Array) -> Result<ArrayRef> {
/// Returns an array of Int32/Int64 denoting the number of bits in each string in the array.
Review comment:
ditto
--
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] viirya commented on a change in pull request #1465: Support the `length` kernel on Binary Array
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1465:
URL: https://github.com/apache/arrow-rs/pull/1465#discussion_r833848532
##########
File path: arrow/src/compute/kernels/length.rs
##########
@@ -94,18 +111,20 @@ where
.downcast_ref::<GenericStringArray<O>>()
.unwrap();
let bits_in_bytes = O::from_usize(8).unwrap();
- unary_offsets_string::<O, _>(array, T::DATA_TYPE, |x| x * bits_in_bytes)
+ unary_offsets!(array, T::DATA_TYPE, |x| x * bits_in_bytes)
}
/// Returns an array of Int32/Int64 denoting the number of bytes in each string in the array.
Review comment:
```suggestion
/// Returns an array of Int32/Int64 denoting the number of bytes in each string/binary in the array.
```
--
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 #1465: Support the `length` kernel on Binary Array
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1465:
URL: https://github.com/apache/arrow-rs/pull/1465
--
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] codecov-commenter edited a comment on pull request #1465: Support the `length` kernel on Binary Array
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1465:
URL: https://github.com/apache/arrow-rs/pull/1465#issuecomment-1073249501
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1465?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1465](https://codecov.io/gh/apache/arrow-rs/pull/1465?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (af7c4e5) into [master](https://codecov.io/gh/apache/arrow-rs/commit/d02425dc0114c4e192e950aeea13e3c1e750a2b0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d02425d) will **increase** coverage by `0.01%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #1465 +/- ##
==========================================
+ Coverage 82.70% 82.72% +0.01%
==========================================
Files 187 187
Lines 54169 54216 +47
==========================================
+ Hits 44801 44849 +48
+ Misses 9368 9367 -1
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1465?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/compute/kernels/length.rs](https://codecov.io/gh/apache/arrow-rs/pull/1465/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9sZW5ndGgucnM=) | `100.00% <100.00%> (ø)` | |
| [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/1465/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9kYXRhdHlwZS5ycw==) | `66.40% <0.00%> (-0.40%)` | :arrow_down: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1465/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldF9kZXJpdmUvc3JjL3BhcnF1ZXRfZmllbGQucnM=) | `66.21% <0.00%> (+0.45%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1465?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1465?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [d02425d...af7c4e5](https://codecov.io/gh/apache/arrow-rs/pull/1465?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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 change in pull request #1465: Support the `length` kernel on Binary Array
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1465:
URL: https://github.com/apache/arrow-rs/pull/1465#discussion_r834611924
##########
File path: arrow/src/compute/kernels/length.rs
##########
@@ -25,11 +25,7 @@ use crate::{
macro_rules! unary_offsets {
($array: expr, $data_type: expr, $op: expr) => {{
- // note: offsets are stored as u8, but they can be interpreted as OffsetSize
- let offsets = &$array.data_ref().buffers()[0];
- // this is a 30% improvement over iterating over u8s and building OffsetSize, which
- // justifies the usage of `unsafe`.
- let slice: &[O] = &unsafe { offsets.typed_data::<O>() }[$array.offset()..];
+ let slice = $array.value_offsets();
Review comment:
❤️ that is much nicer
--
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 change in pull request #1465: Support the `length` kernel on Binary Array
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1465:
URL: https://github.com/apache/arrow-rs/pull/1465#discussion_r833585907
##########
File path: arrow/src/compute/kernels/length.rs
##########
@@ -188,9 +220,23 @@ mod tests {
})
}
+ #[test]
+ fn length_test_binary() -> Result<()> {
+ let value: Vec<&[u8]> = vec![b"zero", b"one", &[0xff, 0xf8]];
+ let result: Vec<i32> = vec![4, 3, 2];
+ length_binary_helper!(i32, Int32Array, length, value, result)
+ }
+
+ #[test]
+ fn length_test_large_binary() -> Result<()> {
+ let value: Vec<&[u8]> = vec![b"zero", &[0xff, 0xf8], b"two"];
Review comment:
💯 for non UTF8
--
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] codecov-commenter commented on pull request #1465: Support the `length` kernel on Binary Array
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1465:
URL: https://github.com/apache/arrow-rs/pull/1465#issuecomment-1073249501
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1465?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1465](https://codecov.io/gh/apache/arrow-rs/pull/1465?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (af7c4e5) into [master](https://codecov.io/gh/apache/arrow-rs/commit/d02425dc0114c4e192e950aeea13e3c1e750a2b0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d02425d) will **increase** coverage by `0.01%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #1465 +/- ##
==========================================
+ Coverage 82.70% 82.72% +0.01%
==========================================
Files 187 187
Lines 54169 54216 +47
==========================================
+ Hits 44801 44849 +48
+ Misses 9368 9367 -1
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1465?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/compute/kernels/length.rs](https://codecov.io/gh/apache/arrow-rs/pull/1465/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9sZW5ndGgucnM=) | `100.00% <100.00%> (ø)` | |
| [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/1465/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9kYXRhdHlwZS5ycw==) | `66.40% <0.00%> (-0.40%)` | :arrow_down: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1465/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldF9kZXJpdmUvc3JjL3BhcnF1ZXRfZmllbGQucnM=) | `66.21% <0.00%> (+0.45%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1465?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1465?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [d02425d...af7c4e5](https://codecov.io/gh/apache/arrow-rs/pull/1465?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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