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/01/02 15:34:24 UTC
[GitHub] [arrow-rs] alamb opened a new pull request #1129: Implement Array for ArrayRef
alamb opened a new pull request #1129:
URL: https://github.com/apache/arrow-rs/pull/1129
# Which issue does this PR close?
Closes https://github.com/apache/arrow-rs/issues/1128
# Rationale for this change
See ticket
# What changes are included in this PR?
1. `impl Array for ArrayRef`
2. tests for same
# Are there any user-facing changes?
Better ergonomics (users can avoid `.as_ref()` in some places)
--
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 #1129: Implement Array for ArrayRef, Improve as_* kernels to take `&dyn Array`
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1129:
URL: https://github.com/apache/arrow-rs/pull/1129#discussion_r777225544
##########
File path: arrow/src/array/cast.rs
##########
@@ -21,7 +21,7 @@ use crate::array::*;
use crate::datatypes::*;
/// Force downcast ArrayRef to PrimitiveArray<T>
-pub fn as_primitive_array<T>(arr: &ArrayRef) -> &PrimitiveArray<T>
+pub fn as_primitive_array<T>(arr: &dyn Array) -> &PrimitiveArray<T>
Review comment:
As `ArrayRef` now implements `Array` all previous calls to this function will still compile and we can also pass `&dyn Array` as well
--
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 #1129: Implement Array for ArrayRef, Improve as_* kernels to take `&dyn Array`
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1129:
URL: https://github.com/apache/arrow-rs/pull/1129#discussion_r778779669
##########
File path: arrow/src/array/cast.rs
##########
@@ -21,7 +21,7 @@ use crate::array::*;
use crate::datatypes::*;
/// Force downcast ArrayRef to PrimitiveArray<T>
-pub fn as_primitive_array<T>(arr: &ArrayRef) -> &PrimitiveArray<T>
+pub fn as_primitive_array<T>(arr: &dyn Array) -> &PrimitiveArray<T>
Review comment:
FWIW this change means that calls that pass in a `&ArrayRef` will generate an extra dispatch as pointed out by @tustvold on https://github.com/apache/arrow-rs/issues/1128#issuecomment-1003747568 -- this can be avoided by the user calling `as_primitive_array(arr.as_ref())` or `as_primitive_array(&*arr)` if it is important to avoid the double dispatch
--
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 #1129: Implement Array for ArrayRef, Improve as_* kernels to take `&dyn Array`
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1129:
URL: https://github.com/apache/arrow-rs/pull/1129#issuecomment-1005616808
For other reviewers, there are a bunch of good comments from @tustvold on https://github.com/apache/arrow-rs/issues/1128#issuecomment-1003747568 related to alternate approaches
--
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 #1129: Implement Array for ArrayRef, Improve as_* kernels to take `&dyn Array`
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1129:
URL: https://github.com/apache/arrow-rs/pull/1129#issuecomment-1003737295
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1129?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 [#1129](https://codecov.io/gh/apache/arrow-rs/pull/1129?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4e82202) into [master](https://codecov.io/gh/apache/arrow-rs/commit/89d52733bdff651f0ed27dc687e48c371d77bc85?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (89d5273) will **increase** coverage by `0.02%`.
> The diff coverage is `87.50%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1129?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1129 +/- ##
==========================================
+ Coverage 82.36% 82.39% +0.02%
==========================================
Files 169 169
Lines 49962 50008 +46
==========================================
+ Hits 41152 41202 +50
+ Misses 8810 8806 -4
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1129?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/pyarrow.rs](https://codecov.io/gh/apache/arrow-rs/pull/1129/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-YXJyb3cvc3JjL3B5YXJyb3cucnM=) | `0.00% <ø> (ø)` | |
| [arrow/src/array/array.rs](https://codecov.io/gh/apache/arrow-rs/pull/1129/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-YXJyb3cvc3JjL2FycmF5L2FycmF5LnJz) | `85.14% <84.21%> (-0.11%)` | :arrow_down: |
| [arrow/src/array/cast.rs](https://codecov.io/gh/apache/arrow-rs/pull/1129/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-YXJyb3cvc3JjL2FycmF5L2Nhc3QucnM=) | `91.66% <93.75%> (+5.95%)` | :arrow_up: |
| [arrow/src/array/equal\_json.rs](https://codecov.io/gh/apache/arrow-rs/pull/1129/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-YXJyb3cvc3JjL2FycmF5L2VxdWFsX2pzb24ucnM=) | `89.91% <100.00%> (+0.05%)` | :arrow_up: |
| [arrow/src/datatypes/field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1129/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.68% <0.00%> (+0.30%)` | :arrow_up: |
| [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/1129/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.42%)` | :arrow_up: |
| [arrow/src/array/array\_primitive.rs](https://codecov.io/gh/apache/arrow-rs/pull/1129/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-YXJyb3cvc3JjL2FycmF5L2FycmF5X3ByaW1pdGl2ZS5ycw==) | `94.66% <0.00%> (+0.42%)` | :arrow_up: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1129/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/1129?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/1129?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 [89d5273...4e82202](https://codecov.io/gh/apache/arrow-rs/pull/1129?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 #1129: Implement Array for ArrayRef, Improve as_* kernels to take `&dyn Array`
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1129:
URL: https://github.com/apache/arrow-rs/pull/1129#discussion_r778779669
##########
File path: arrow/src/array/cast.rs
##########
@@ -21,7 +21,7 @@ use crate::array::*;
use crate::datatypes::*;
/// Force downcast ArrayRef to PrimitiveArray<T>
-pub fn as_primitive_array<T>(arr: &ArrayRef) -> &PrimitiveArray<T>
+pub fn as_primitive_array<T>(arr: &dyn Array) -> &PrimitiveArray<T>
Review comment:
FWIW this change means that calls that pass in a `&ArrayRef` will generate an extra dispatch as pointed out by @tustvold on https://github.com/apache/arrow-rs/issues/1128#issuecomment-1003747568 -- this can be avoided by the user calling `as_primitive_array(arr.as_ref())` or `as_primitive_array(&*arr)` if the double virtual dispatch is bad
--
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 #1129: Implement Array for ArrayRef, Improve as_* kernels to take `&dyn Array`
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1129:
URL: https://github.com/apache/arrow-rs/pull/1129#discussion_r777226273
##########
File path: arrow/src/pyarrow.rs
##########
@@ -148,16 +148,6 @@ impl PyArrowConvert for ArrayData {
}
}
-impl PyArrowConvert for ArrayRef {
Review comment:
this is now covered by the impl of `Array` for ArrayRef
--
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 #1129: Implement Array for ArrayRef, Improve as_* kernels to take `&dyn Array`
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1129:
URL: https://github.com/apache/arrow-rs/pull/1129
--
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