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/29 17:12:00 UTC
[GitHub] [arrow-rs] alamb opened a new pull request #1506: Implement size_hint and ExactSizedIterator for DecimalArray
alamb opened a new pull request #1506:
URL: https://github.com/apache/arrow-rs/pull/1506
# Which issue does this PR close?
Closes https://github.com/apache/arrow-rs/issues/1505
# Rationale for this change
I was doing some small cleanups in DataFusion https://github.com/apache/arrow-datafusion/pull/2107 and noticed that the DecimalArray iterators do not report their exact size. The exact size is used in several places for optimizations in Arrow
# What changes are included in this PR?
1. Implement size_hint and ExactSizedIterator for DecimalArray
2. Tests for same
<!---
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?
Can use DecimalArray::iter() in more situations
--
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 #1506: Implement `size_hint` and `ExactSizedIterator` for `DecimalArray`
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1506:
URL: https://github.com/apache/arrow-rs/pull/1506#discussion_r837800300
##########
File path: arrow/src/array/array_binary.rs
##########
@@ -1020,8 +1020,17 @@ impl<'a> std::iter::Iterator for DecimalIter<'a> {
}
}
}
+
+ #[inline]
+ fn size_hint(&self) -> (usize, Option<usize>) {
+ let remain = self.array.len() - self.current;
+ (remain, Some(remain))
+ }
}
+/// iterator has have known size.
+impl<'a> std::iter::ExactSizeIterator for DecimalIter<'a> {}
Review comment:
🤔 that is a good idea. I can certainly move it there.
--
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 pull request #1506: Implement `size_hint` and `ExactSizedIterator` for `DecimalArray`
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1506:
URL: https://github.com/apache/arrow-rs/pull/1506#issuecomment-1082350575
lgtm. thanks @alamb !
--
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 #1506: Implement `size_hint` and `ExactSizedIterator` for `DecimalArray`
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1506:
URL: https://github.com/apache/arrow-rs/pull/1506#discussion_r837800577
##########
File path: arrow/src/array/array_binary.rs
##########
@@ -1020,8 +1020,17 @@ impl<'a> std::iter::Iterator for DecimalIter<'a> {
}
}
}
+
+ #[inline]
+ fn size_hint(&self) -> (usize, Option<usize>) {
+ let remain = self.array.len() - self.current;
Review comment:
```suggestion
let remain = self.current_end - self.current;
```
--
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 pull request #1506: Implement `size_hint` and `ExactSizedIterator` for `DecimalArray`
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1506:
URL: https://github.com/apache/arrow-rs/pull/1506#issuecomment-1082323072
Thanks for the review @viirya !
--
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 #1506: Implement `size_hint` and `ExactSizedIterator` for `DecimalArray`
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1506:
URL: https://github.com/apache/arrow-rs/pull/1506#issuecomment-1082171552
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1506?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 [#1506](https://codecov.io/gh/apache/arrow-rs/pull/1506?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (50d5fe0) into [master](https://codecov.io/gh/apache/arrow-rs/commit/4022c11586dda1aa3d5c04cd8fb389b681c116fa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4022c11) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #1506 +/- ##
=======================================
Coverage 82.70% 82.71%
=======================================
Files 188 188
Lines 54404 54420 +16
=======================================
+ Hits 44993 45011 +18
+ Misses 9411 9409 -2
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1506?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/array/array\_binary.rs](https://codecov.io/gh/apache/arrow-rs/pull/1506/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-YXJyb3cvc3JjL2FycmF5L2FycmF5X2JpbmFyeS5ycw==) | `93.06% <100.00%> (+0.44%)` | :arrow_up: |
| [arrow/src/datatypes/field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1506/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-YXJyb3cvc3JjL2RhdGF0eXBlcy9maWVsZC5ycw==) | `53.79% <0.00%> (-0.31%)` | :arrow_down: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1506/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%> (ø)` | |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1506/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-YXJyb3cvc3JjL2FycmF5L3RyYW5zZm9ybS9tb2QucnM=) | `86.57% <0.00%> (+0.11%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1506?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/1506?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 [4022c11...50d5fe0](https://codecov.io/gh/apache/arrow-rs/pull/1506?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] codecov-commenter edited a comment on pull request #1506: Implement `size_hint` and `ExactSizedIterator` for `DecimalArray`
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1506:
URL: https://github.com/apache/arrow-rs/pull/1506#issuecomment-1082171552
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1506?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 [#1506](https://codecov.io/gh/apache/arrow-rs/pull/1506?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a4f4206) into [master](https://codecov.io/gh/apache/arrow-rs/commit/4022c11586dda1aa3d5c04cd8fb389b681c116fa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4022c11) will **increase** coverage by `0.04%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #1506 +/- ##
==========================================
+ Coverage 82.70% 82.74% +0.04%
==========================================
Files 188 188
Lines 54404 54370 -34
==========================================
- Hits 44993 44988 -5
+ Misses 9411 9382 -29
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1506?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/array/array\_binary.rs](https://codecov.io/gh/apache/arrow-rs/pull/1506/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-YXJyb3cvc3JjL2FycmF5L2FycmF5X2JpbmFyeS5ycw==) | `93.06% <100.00%> (+0.44%)` | :arrow_up: |
| [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/1506/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: |
| [arrow/src/compute/kernels/take.rs](https://codecov.io/gh/apache/arrow-rs/pull/1506/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-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy90YWtlLnJz) | `95.41% <0.00%> (+0.13%)` | :arrow_up: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/1506/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-cGFycXVldC9zcmMvZW5jb2RpbmdzL2VuY29kaW5nLnJz) | `93.56% <0.00%> (+0.18%)` | :arrow_up: |
| [parquet/src/arrow/array\_reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/1506/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-cGFycXVldC9zcmMvYXJyb3cvYXJyYXlfcmVhZGVyLnJz) | `86.70% <0.00%> (+2.34%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1506?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/1506?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 [4022c11...a4f4206](https://codecov.io/gh/apache/arrow-rs/pull/1506?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] codecov-commenter edited a comment on pull request #1506: Implement `size_hint` and `ExactSizedIterator` for `DecimalArray`
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1506:
URL: https://github.com/apache/arrow-rs/pull/1506#issuecomment-1082171552
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1506?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 [#1506](https://codecov.io/gh/apache/arrow-rs/pull/1506?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7218586) into [master](https://codecov.io/gh/apache/arrow-rs/commit/4c223159be0ec0f463ff67db22d3ed6823a81c7a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4c22315) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #1506 +/- ##
=======================================
Coverage 82.73% 82.74%
=======================================
Files 188 188
Lines 54354 54370 +16
=======================================
+ Hits 44970 44987 +17
+ Misses 9384 9383 -1
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1506?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/array/array\_binary.rs](https://codecov.io/gh/apache/arrow-rs/pull/1506/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-YXJyb3cvc3JjL2FycmF5L2FycmF5X2JpbmFyeS5ycw==) | `92.93% <100.00%> (+0.31%)` | :arrow_up: |
| [arrow/src/array/iterator.rs](https://codecov.io/gh/apache/arrow-rs/pull/1506/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-YXJyb3cvc3JjL2FycmF5L2l0ZXJhdG9yLnJz) | `96.11% <100.00%> (+0.30%)` | :arrow_up: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1506/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=) | `65.75% <0.00%> (-0.46%)` | :arrow_down: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/1506/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-cGFycXVldC9zcmMvZW5jb2RpbmdzL2VuY29kaW5nLnJz) | `93.37% <0.00%> (-0.19%)` | :arrow_down: |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1506/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-YXJyb3cvc3JjL2FycmF5L3RyYW5zZm9ybS9tb2QucnM=) | `86.57% <0.00%> (+0.11%)` | :arrow_up: |
| [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/1506/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.80% <0.00%> (+0.39%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1506?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/1506?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 [4c22315...7218586](https://codecov.io/gh/apache/arrow-rs/pull/1506?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] viirya commented on a change in pull request #1506: Implement `size_hint` and `ExactSizedIterator` for `DecimalArray`
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1506:
URL: https://github.com/apache/arrow-rs/pull/1506#discussion_r837797417
##########
File path: arrow/src/array/array_binary.rs
##########
@@ -1020,8 +1020,17 @@ impl<'a> std::iter::Iterator for DecimalIter<'a> {
}
}
}
+
+ #[inline]
+ fn size_hint(&self) -> (usize, Option<usize>) {
+ let remain = self.array.len() - self.current;
+ (remain, Some(remain))
+ }
}
+/// iterator has have known size.
+impl<'a> std::iter::ExactSizeIterator for DecimalIter<'a> {}
Review comment:
Should we have it in `iterator.rs`? I see other `ExactSizeIterator`s implementations located there.
--
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 #1506: Implement `size_hint` and `ExactSizedIterator` for `DecimalArray`
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1506:
URL: https://github.com/apache/arrow-rs/pull/1506
--
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 #1506: Implement `size_hint` and `ExactSizedIterator` for `DecimalArray`
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1506:
URL: https://github.com/apache/arrow-rs/pull/1506#discussion_r837796885
##########
File path: arrow/src/array/array_binary.rs
##########
@@ -1020,8 +1020,17 @@ impl<'a> std::iter::Iterator for DecimalIter<'a> {
}
}
}
+
+ #[inline]
+ fn size_hint(&self) -> (usize, Option<usize>) {
+ let remain = self.array.len() - self.current;
+ (remain, Some(remain))
+ }
}
+/// iterator has have known size.
Review comment:
```suggestion
/// iterator has known size.
```
--
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