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 2021/12/21 07:07:41 UTC
[GitHub] [arrow-rs] matthewmturner opened a new pull request #1074: Define eq_dyn_scalar API
matthewmturner opened a new pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074
# Which issue does this PR close?
<!---
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Working on this in relation to #984 and #1068 with the end goal being to finalize how we want `eq_dyn_scalar` to work.
# Rationale for this change
<!---
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!---
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?
<!---
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
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 #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1000071023
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1074?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 [#1074](https://codecov.io/gh/apache/arrow-rs/pull/1074?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4ed0c36) into [master](https://codecov.io/gh/apache/arrow-rs/commit/f3e452cb29b379d284b19e641c6b0b460f5bb0fa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f3e452c) will **increase** coverage by `0.02%`.
> The diff coverage is `75.38%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1074?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 #1074 +/- ##
==========================================
+ Coverage 82.27% 82.30% +0.02%
==========================================
Files 168 168
Lines 49281 49497 +216
==========================================
+ Hits 40547 40738 +191
- Misses 8734 8759 +25
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1074?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/comparison.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jb21wYXJpc29uLnJz) | `91.63% <75.38%> (-1.64%)` | :arrow_down: |
| [parquet/src/encodings/rle.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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-cGFycXVldC9zcmMvZW5jb2RpbmdzL3JsZS5ycw==) | `92.70% <0.00%> (ø)` | |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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.98% <0.00%> (ø)` | |
| [arrow/src/compute/kernels/cast.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jYXN0LnJz) | `95.06% <0.00%> (+0.08%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1074?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/1074?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 [f3e452c...4ed0c36](https://codecov.io/gh/apache/arrow-rs/pull/1074?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 #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1000071023
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1074?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 [#1074](https://codecov.io/gh/apache/arrow-rs/pull/1074?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9d03e72) into [master](https://codecov.io/gh/apache/arrow-rs/commit/2ad99ec33c757d6c7b4a5e69e19a4a096813b53b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2ad99ec) will **decrease** coverage by `0.02%`.
> The diff coverage is `69.38%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1074?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 #1074 +/- ##
==========================================
- Coverage 82.34% 82.31% -0.03%
==========================================
Files 168 168
Lines 49479 49577 +98
==========================================
+ Hits 40743 40810 +67
- Misses 8736 8767 +31
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1074?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/comparison.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jb21wYXJpc29uLnJz) | `90.87% <69.38%> (-2.60%)` | :arrow_down: |
| [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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.38% <0.00%> (-0.43%)` | :arrow_down: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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.98% <0.00%> (-0.23%)` | :arrow_down: |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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=) | `85.56% <0.00%> (ø)` | |
| [arrow/src/datatypes/field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1074?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/1074?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 [2ad99ec...9d03e72](https://codecov.io/gh/apache/arrow-rs/pull/1074?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 merged pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074
--
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] matthewmturner commented on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1000663951
@alamb to confirm, we arent handling floats with this kernel right? My understanding is our current `T` wouldnt work with that.
--
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 #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1000071023
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1074?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 [#1074](https://codecov.io/gh/apache/arrow-rs/pull/1074?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e461a6a) into [master](https://codecov.io/gh/apache/arrow-rs/commit/f3e452cb29b379d284b19e641c6b0b460f5bb0fa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f3e452c) will **decrease** coverage by `0.00%`.
> The diff coverage is `41.30%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1074?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 #1074 +/- ##
==========================================
- Coverage 82.27% 82.27% -0.01%
==========================================
Files 168 168
Lines 49281 49440 +159
==========================================
+ Hits 40547 40675 +128
- Misses 8734 8765 +31
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1074?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/comparison.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jb21wYXJpc29uLnJz) | `90.15% <41.30%> (-3.13%)` | :arrow_down: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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.52% <0.00%> (-0.20%)` | :arrow_down: |
| [parquet/src/encodings/rle.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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-cGFycXVldC9zcmMvZW5jb2RpbmdzL3JsZS5ycw==) | `92.70% <0.00%> (ø)` | |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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.22%)` | :arrow_up: |
| [arrow/src/compute/kernels/cast.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jYXN0LnJz) | `95.23% <0.00%> (+0.24%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1074?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/1074?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 [f3e452c...e461a6a](https://codecov.io/gh/apache/arrow-rs/pull/1074?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] matthewmturner commented on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1001212905
actually - are `bool` values currently supported with `DictionaryArray`?
I get the following when trying to make one:
```
a value of type `array_dictionary::DictionaryArray<datatypes::types::Int8Type>` cannot be built from an iterator over elements of type `bool`
the trait `FromIterator<bool>` is not implemented for `array_dictionary::DictionaryArray<datatypes::types::Int8Type>`
```
i also dont see any tests for `DictionaryArray` that has that as value type.
--
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] matthewmturner edited a comment on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner edited a comment on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1000663951
@alamb to confirm, we arent handling floats with this kernel right? My understanding is our current `T` wouldnt work with that.
ive also done some cleaning up - can you let me know if you think this is ok? In particular:
1. It requires the user of the kernel to wrap the array in an `Arc` so that we can use the `as_xx_array` functions. we can put that in the kernel code itself if you dont think users should have to do that.
2. I provided two signatures for `dyn_compare_scalar` - one for primitive and one for dictionary.
--
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 #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1000071023
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1074?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 [#1074](https://codecov.io/gh/apache/arrow-rs/pull/1074?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (91fc62e) into [master](https://codecov.io/gh/apache/arrow-rs/commit/f3e452cb29b379d284b19e641c6b0b460f5bb0fa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f3e452c) will **increase** coverage by `0.01%`.
> The diff coverage is `70.45%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1074?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 #1074 +/- ##
==========================================
+ Coverage 82.27% 82.29% +0.01%
==========================================
Files 168 168
Lines 49281 49518 +237
==========================================
+ Hits 40547 40751 +204
- Misses 8734 8767 +33
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1074?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/comparison.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jb21wYXJpc29uLnJz) | `90.71% <70.45%> (-2.56%)` | :arrow_down: |
| [arrow/src/datatypes/field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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.37% <0.00%> (-0.31%)` | :arrow_down: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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.52% <0.00%> (-0.20%)` | :arrow_down: |
| [parquet/src/encodings/rle.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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-cGFycXVldC9zcmMvZW5jb2RpbmdzL3JsZS5ycw==) | `92.70% <0.00%> (ø)` | |
| [arrow/src/compute/kernels/cast.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jYXN0LnJz) | `95.06% <0.00%> (+0.08%)` | :arrow_up: |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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=) | `84.86% <0.00%> (+0.13%)` | :arrow_up: |
| [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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.43% <0.00%> (+0.45%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1074?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/1074?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 [f3e452c...91fc62e](https://codecov.io/gh/apache/arrow-rs/pull/1074?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 #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1000071023
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1074?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 [#1074](https://codecov.io/gh/apache/arrow-rs/pull/1074?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2aa16b8) into [master](https://codecov.io/gh/apache/arrow-rs/commit/f3e452cb29b379d284b19e641c6b0b460f5bb0fa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f3e452c) will **increase** coverage by `0.01%`.
> The diff coverage is `60.00%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1074?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 #1074 +/- ##
==========================================
+ Coverage 82.27% 82.29% +0.01%
==========================================
Files 168 168
Lines 49281 49471 +190
==========================================
+ Hits 40547 40712 +165
- Misses 8734 8759 +25
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1074?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/comparison.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jb21wYXJpc29uLnJz) | `91.38% <60.00%> (-1.90%)` | :arrow_down: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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.23%)` | :arrow_down: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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.52% <0.00%> (-0.20%)` | :arrow_down: |
| [parquet/src/encodings/rle.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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-cGFycXVldC9zcmMvZW5jb2RpbmdzL3JsZS5ycw==) | `92.70% <0.00%> (ø)` | |
| [arrow/src/compute/kernels/cast.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jYXN0LnJz) | `95.06% <0.00%> (+0.08%)` | :arrow_up: |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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=) | `85.00% <0.00%> (+0.27%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1074?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/1074?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 [f3e452c...2aa16b8](https://codecov.io/gh/apache/arrow-rs/pull/1074?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 pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-998873067
cc @shepmaster , @jorgecarleitao and @alippai
--
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] matthewmturner commented on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1001187993
> How shall we proceed -- get this one polished up (maybe also add `eq_dyn_scalar_bool`?) and then start hammering on the other kernels (e.g. `neq`, `lt`, `gt`, etc?)
Sure sounds good. Should I just add those on this PR?
--
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] matthewmturner edited a comment on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner edited a comment on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1000663951
@alamb to confirm, we arent handling floats with this kernel right? My understanding is our current `T` wouldnt work with that.
ive also done some cleaning up - can you let me know if you think this is ok? In particular:
1. It requires the user of the kernel to wrap the array in an `Arc` so that we can use the `as_xx_array` functions
2. I provided two signatures for `dyn_compare_scalar` - one for primitive and one for dictionary.
--
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] matthewmturner commented on a change in pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on a change in pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#discussion_r774044776
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -898,6 +898,126 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
compare_op_scalar!(left, right, |a, b| a >= b)
}
+macro_rules! dyn_cmp_scalar {
+ ($LEFT: expr, $RIGHT: expr, $T: ident, $OP: ident, $TT: tt) => {{
+ let left = $LEFT.as_any().downcast_ref::<$T>().ok_or_else(|| {
+ ArrowError::CastError(format!(
+ "Left array cannot be cast to {}",
+ type_name::<$T>()
+ ))
+ })?;
+ let right = $RIGHT.as_any().downcast_ref::<$T>().ok_or_else(|| {
+ ArrowError::CastError(format!(
+ "Right array cannot be cast to {}",
+ type_name::<$T>(),
+ ))
+ })?;
+ $OP::<$TT>(left, right)
+ }};
+}
+
+macro_rules! dyn_compare_scalar {
+ ($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
+ let right = $RIGHT.try_into().map_err(|_| {
Review comment:
I just hadn't yet updated where $RIGHT is used to use right instead
--
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 #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1000071023
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1074?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 [#1074](https://codecov.io/gh/apache/arrow-rs/pull/1074?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e8c5e46) into [master](https://codecov.io/gh/apache/arrow-rs/commit/2ad99ec33c757d6c7b4a5e69e19a4a096813b53b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2ad99ec) will **decrease** coverage by `0.02%`.
> The diff coverage is `69.38%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1074?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 #1074 +/- ##
==========================================
- Coverage 82.34% 82.31% -0.03%
==========================================
Files 168 168
Lines 49479 49577 +98
==========================================
+ Hits 40743 40810 +67
- Misses 8736 8767 +31
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1074?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/comparison.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jb21wYXJpc29uLnJz) | `90.87% <69.38%> (-2.60%)` | :arrow_down: |
| [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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.38% <0.00%> (-0.43%)` | :arrow_down: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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.98% <0.00%> (-0.23%)` | :arrow_down: |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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=) | `85.69% <0.00%> (+0.13%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1074?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/1074?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 [2ad99ec...e8c5e46](https://codecov.io/gh/apache/arrow-rs/pull/1074?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 pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1003549371
Thanks again @matthewmturner and @liukun4515 -- this is great.
--
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 #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#discussion_r775856676
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -898,6 +900,305 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
compare_op_scalar!(left, right, |a, b| a >= b)
}
+macro_rules! dyn_compare_scalar {
+ ($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
+ let right: i128 = $RIGHT.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from("Can not convert scalar to i128"))
+ })?;
+ match $LEFT.data_type() {
+ DataType::Int8 => {
+ let right: i8 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
Review comment:
It affects the messages below as well
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -898,6 +900,305 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
compare_op_scalar!(left, right, |a, b| a >= b)
}
+macro_rules! dyn_compare_scalar {
+ ($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
+ let right: i128 = $RIGHT.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from("Can not convert scalar to i128"))
+ })?;
+ match $LEFT.data_type() {
+ DataType::Int8 => {
+ let right: i8 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left = as_primitive_array::<Int8Type>($LEFT);
+ $OP::<Int8Type>(left, right)
+ }
+ DataType::Int16 => {
+ let right: i16 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left = as_primitive_array::<Int16Type>($LEFT);
+ $OP::<Int16Type>(left, right)
+ }
+ DataType::Int32 => {
+ let right: i32 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left = as_primitive_array::<Int32Type>($LEFT);
+ $OP::<Int32Type>(left, right)
+ }
+ DataType::Int64 => {
+ let right: i64 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left = as_primitive_array::<Int64Type>($LEFT);
+ $OP::<Int64Type>(left, right)
+ }
+ DataType::UInt8 => {
+ let right: u8 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left = as_primitive_array::<UInt8Type>($LEFT);
+ $OP::<UInt8Type>(left, right)
+ }
+ DataType::UInt16 => {
+ let right: u16 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left = as_primitive_array::<UInt16Type>($LEFT);
+ $OP::<UInt16Type>(left, right)
+ }
+ DataType::UInt32 => {
+ let right: u32 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left = as_primitive_array::<UInt32Type>($LEFT);
+ $OP::<UInt32Type>(left, right)
+ }
+ DataType::UInt64 => {
+ let right: u64 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left = as_primitive_array::<UInt64Type>($LEFT);
+ $OP::<UInt64Type>(left, right)
+ }
+ _ => Err(ArrowError::ComputeError(String::from(
+ "Unsupported data type",
+ ))),
+ }
+ }};
+ ($LEFT: expr, $RIGHT: expr, $KT: ident, $OP: ident) => {{
Review comment:
```suggestion
/// Applies `LEFT OP RIGHT` when `LEFT` is a `DictionaryArray` with keys of type `KT`
($LEFT: expr, $RIGHT: expr, $KT: ident, $OP: ident) => {{
```
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -898,6 +900,305 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
compare_op_scalar!(left, right, |a, b| a >= b)
}
+macro_rules! dyn_compare_scalar {
+ ($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
Review comment:
```suggestion
/// Applies `LEFT OP RIGHT` when `LEFT` is a `DictionaryArray` with keys of type `KT`
($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
```
--
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] matthewmturner commented on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1000349303
@alamb to confirm, are you expecting this kernel to handle boolean and utf8 comparisons 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] matthewmturner edited a comment on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner edited a comment on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1000349303
@alamb to confirm, are you expecting this kernel to handle boolean and utf8 comparisons as well? Or is that only available with the future scalar API?
--
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 #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1003406889
> i can make an issue for it - and im hoping to work on implementing it this weekend.
Thanks @matthewmturner ❤️
> do you have a preference for how small (i.e how many kernels) youd like each pr / issue to be?
My personal bias is one PR per kernel (as that makes it easiest to review) but I am also happy to review a single PR too
--
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 #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#discussion_r774535495
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -898,6 +899,224 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
compare_op_scalar!(left, right, |a, b| a >= b)
}
+macro_rules! dyn_compare_scalar {
+ ($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
+ let right: i128 = $RIGHT.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from("Can not convert scalar to i128"))
+ })?;
+ match ($LEFT.data_type(), $RIGHT.get_arrow_type()) {
+ // (DataType::Boolean, DataType::Boolean) => {
+ // typed_cmp_scalar!($LEFT, $RIGHT, BooleanArray, $OP_BOOL)
+ // }
+ (DataType::Int8, DataType::Int8) => {
+ let right: i8 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left =
+ $LEFT.as_any().downcast_ref::<Int8Array>().ok_or_else(|| {
+ ArrowError::CastError(String::from("Left array cannot be cast"))
+ })?;
+ $OP::<Int8Type>(left, right)
+ }
+ (DataType::Int16, DataType::Int16) => {
+ let right: i16 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left =
+ $LEFT.as_any().downcast_ref::<Int16Array>().ok_or_else(|| {
+ ArrowError::CastError(String::from("Left array cannot be cast"))
+ })?;
+ $OP::<Int16Type>(left, right)
+ }
+ (DataType::Int32, DataType::Int32) => {
+ let right: i32 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left =
+ $LEFT.as_any().downcast_ref::<Int32Array>().ok_or_else(|| {
+ ArrowError::CastError(String::from("Left array cannot be cast"))
+ })?;
+ $OP::<Int32Type>(left, right)
+ }
+ (DataType::Int64, DataType::Int64) => {
+ let right: i64 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left =
+ $LEFT.as_any().downcast_ref::<Int64Array>().ok_or_else(|| {
+ ArrowError::CastError(String::from("Left array cannot be cast"))
+ })?;
+ $OP::<Int64Type>(left, right)
+ }
+ // (DataType::UInt8, DataType::UInt8) => {
+ // dyn_cmp_scalar!($LEFT, right, UInt8Array, $OP, UInt8Type)
+ // }
+ // (DataType::UInt16, DataType::UInt16) => {
+ // dyn_cmp_scalar!($LEFT, right, UInt16Array, $OP, UInt16Type)
+ // }
+ // (DataType::UInt32, DataType::UInt32) => {
+ // dyn_cmp_scalar!($LEFT, right, UInt32Array, $OP, UInt32Type)
+ // }
+ // (DataType::UInt64, DataType::UInt64) => {
+ // dyn_cmp_scalar!($LEFT, right, UInt64Array, $OP, UInt64Type)
+ // }
+ // (DataType::Float32, DataType::Float32) => {
+ // dyn_cmp_scalar!($LEFT, right, Float32Array, $OP, Float32Type)
+ // }
+ // (DataType::Float64, DataType::Float64) => {
+ // dyn_cmp_scalar!($LEFT, right, Float64Array, $OP, Float64Type)
+ // }
+ // (DataType::Utf8, DataType::Utf8) => {
+ // let right: i32 = right.try_into().map_err(|_| {
+ // ArrowError::ComputeError(String::from(
+ // "Can not convert scalar to i128",
+ // ))
+ // })?;
+ // let left =
+ // $LEFT
+ // .as_any()
+ // .downcast_ref::<StringArray>()
+ // .ok_or_else(|| {
+ // ArrowError::CastError(String::from(
+ // "Left array cannot be cast",
+ // ))
+ // })?;
+ // $OP::<Int32Type>(left, right)
+ // }
+ // (DataType::LargeUtf8, DataType::LargeUtf8) => {
+ // dyn_cmp_scalar!($LEFT, $RIGHT, LargeStringArray, $OP, i64)
+ // }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing arrays of type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare an array with a scalar of different type ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+pub trait IntoArrowNumericType {
+ // type Arrow: ArrowNumericType<Native = Self>;
+ fn get_arrow_type(&self) -> &DataType;
+}
+
+impl IntoArrowNumericType for i8 {
+ // type Arrow = Int8Type;
+ fn get_arrow_type(&self) -> &DataType {
+ &DataType::Int8
+ }
+}
+
+impl IntoArrowNumericType for i16 {
+ // type Arrow = Int8Type;
+ fn get_arrow_type(&self) -> &DataType {
+ &DataType::Int16
+ }
+}
+
+impl IntoArrowNumericType for i32 {
+ // type Arrow = Int8Type;
+ fn get_arrow_type(&self) -> &DataType {
+ &DataType::Int32
+ }
+}
+
+/// Perform `left == right` operation on an array and a numeric scalar
+/// value. Supports PrimitiveArrays, and DictionaryArrays that have primitive values
+pub fn eq_dyn_scalar<T>(left: &dyn Array, right: T) -> Result<BooleanArray>
+where
+ T: IntoArrowNumericType + TryInto<i128> + Copy + std::fmt::Debug,
+{
+ let result = match left.data_type() {
+ DataType::Dictionary(key_type, _) => match key_type.as_ref() {
+ DataType::UInt8 => {
+ let left = left
+ .as_any()
+ .downcast_ref::<DictionaryArray<UInt8Type>>()
+ .unwrap();
Review comment:
You might be able to use https://docs.rs/arrow/6.4.0/arrow/array/fn.as_dictionary_array.html here too:
```suggestion
let left = as_dictionary_array<DictionaryArray<UInt8Type>>()?;
```
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -898,6 +899,224 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
compare_op_scalar!(left, right, |a, b| a >= b)
}
+macro_rules! dyn_compare_scalar {
+ ($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
+ let right: i128 = $RIGHT.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from("Can not convert scalar to i128"))
+ })?;
+ match ($LEFT.data_type(), $RIGHT.get_arrow_type()) {
+ // (DataType::Boolean, DataType::Boolean) => {
+ // typed_cmp_scalar!($LEFT, $RIGHT, BooleanArray, $OP_BOOL)
+ // }
+ (DataType::Int8, DataType::Int8) => {
+ let right: i8 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left =
+ $LEFT.as_any().downcast_ref::<Int8Array>().ok_or_else(|| {
+ ArrowError::CastError(String::from("Left array cannot be cast"))
+ })?;
+ $OP::<Int8Type>(left, right)
+ }
+ (DataType::Int16, DataType::Int16) => {
+ let right: i16 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left =
+ $LEFT.as_any().downcast_ref::<Int16Array>().ok_or_else(|| {
+ ArrowError::CastError(String::from("Left array cannot be cast"))
+ })?;
+ $OP::<Int16Type>(left, right)
+ }
+ (DataType::Int32, DataType::Int32) => {
+ let right: i32 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left =
+ $LEFT.as_any().downcast_ref::<Int32Array>().ok_or_else(|| {
+ ArrowError::CastError(String::from("Left array cannot be cast"))
+ })?;
+ $OP::<Int32Type>(left, right)
+ }
+ (DataType::Int64, DataType::Int64) => {
+ let right: i64 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left =
+ $LEFT.as_any().downcast_ref::<Int64Array>().ok_or_else(|| {
+ ArrowError::CastError(String::from("Left array cannot be cast"))
+ })?;
+ $OP::<Int64Type>(left, right)
+ }
+ // (DataType::UInt8, DataType::UInt8) => {
+ // dyn_cmp_scalar!($LEFT, right, UInt8Array, $OP, UInt8Type)
+ // }
+ // (DataType::UInt16, DataType::UInt16) => {
+ // dyn_cmp_scalar!($LEFT, right, UInt16Array, $OP, UInt16Type)
+ // }
+ // (DataType::UInt32, DataType::UInt32) => {
+ // dyn_cmp_scalar!($LEFT, right, UInt32Array, $OP, UInt32Type)
+ // }
+ // (DataType::UInt64, DataType::UInt64) => {
+ // dyn_cmp_scalar!($LEFT, right, UInt64Array, $OP, UInt64Type)
+ // }
+ // (DataType::Float32, DataType::Float32) => {
+ // dyn_cmp_scalar!($LEFT, right, Float32Array, $OP, Float32Type)
+ // }
+ // (DataType::Float64, DataType::Float64) => {
+ // dyn_cmp_scalar!($LEFT, right, Float64Array, $OP, Float64Type)
+ // }
+ // (DataType::Utf8, DataType::Utf8) => {
+ // let right: i32 = right.try_into().map_err(|_| {
+ // ArrowError::ComputeError(String::from(
+ // "Can not convert scalar to i128",
+ // ))
+ // })?;
+ // let left =
+ // $LEFT
+ // .as_any()
+ // .downcast_ref::<StringArray>()
+ // .ok_or_else(|| {
+ // ArrowError::CastError(String::from(
+ // "Left array cannot be cast",
+ // ))
+ // })?;
+ // $OP::<Int32Type>(left, right)
+ // }
+ // (DataType::LargeUtf8, DataType::LargeUtf8) => {
+ // dyn_cmp_scalar!($LEFT, $RIGHT, LargeStringArray, $OP, i64)
+ // }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing arrays of type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare an array with a scalar of different type ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+pub trait IntoArrowNumericType {
+ // type Arrow: ArrowNumericType<Native = Self>;
+ fn get_arrow_type(&self) -> &DataType;
+}
+
+impl IntoArrowNumericType for i8 {
+ // type Arrow = Int8Type;
+ fn get_arrow_type(&self) -> &DataType {
+ &DataType::Int8
+ }
+}
+
+impl IntoArrowNumericType for i16 {
+ // type Arrow = Int8Type;
+ fn get_arrow_type(&self) -> &DataType {
+ &DataType::Int16
+ }
+}
+
+impl IntoArrowNumericType for i32 {
+ // type Arrow = Int8Type;
+ fn get_arrow_type(&self) -> &DataType {
+ &DataType::Int32
+ }
+}
+
+/// Perform `left == right` operation on an array and a numeric scalar
+/// value. Supports PrimitiveArrays, and DictionaryArrays that have primitive values
+pub fn eq_dyn_scalar<T>(left: &dyn Array, right: T) -> Result<BooleanArray>
+where
+ T: IntoArrowNumericType + TryInto<i128> + Copy + std::fmt::Debug,
Review comment:
If `T` is going to have `TryInto<i128>` , I wonder if we still `IntoArrowNumericType` at all? I think it may not be necessary any more.
My thinking is that since `dyn_compare_scalar` converts `right` into `i128` immediately there are then conversion rules back to all of the primitive types needed to call `eq_scalar`
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -898,6 +899,224 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
compare_op_scalar!(left, right, |a, b| a >= b)
}
+macro_rules! dyn_compare_scalar {
+ ($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
+ let right: i128 = $RIGHT.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from("Can not convert scalar to i128"))
+ })?;
+ match ($LEFT.data_type(), $RIGHT.get_arrow_type()) {
+ // (DataType::Boolean, DataType::Boolean) => {
+ // typed_cmp_scalar!($LEFT, $RIGHT, BooleanArray, $OP_BOOL)
+ // }
+ (DataType::Int8, DataType::Int8) => {
+ let right: i8 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left =
+ $LEFT.as_any().downcast_ref::<Int8Array>().ok_or_else(|| {
+ ArrowError::CastError(String::from("Left array cannot be cast"))
+ })?;
Review comment:
I think you can use https://docs.rs/arrow/6.4.0/arrow/array/fn.as_primitive_array.html to simplify this
So something like
```suggestion
as_primitive_array::<Int8Array>($LEFT)?;
```
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -898,6 +898,126 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
compare_op_scalar!(left, right, |a, b| a >= b)
}
+macro_rules! dyn_cmp_scalar {
+ ($LEFT: expr, $RIGHT: expr, $T: ident, $OP: ident, $TT: tt) => {{
+ let left = $LEFT.as_any().downcast_ref::<$T>().ok_or_else(|| {
+ ArrowError::CastError(format!(
+ "Left array cannot be cast to {}",
+ type_name::<$T>()
+ ))
+ })?;
+ let right = $RIGHT.as_any().downcast_ref::<$T>().ok_or_else(|| {
+ ArrowError::CastError(format!(
+ "Right array cannot be cast to {}",
+ type_name::<$T>(),
+ ))
+ })?;
+ $OP::<$TT>(left, right)
+ }};
+}
+
+macro_rules! dyn_compare_scalar {
+ ($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
+ let right = $RIGHT.try_into().map_err(|_| {
+ ArrowError::ComputeError(format!(
+ "Can not convert scalar {:?} to i128",
+ $RIGHT
+ ))
+ });
+ match ($LEFT.data_type(), $RIGHT::Arrow) {
+ // (DataType::Boolean, DataType::Boolean) => {
+ // typed_cmp_scalar!($LEFT, $RIGHT, BooleanArray, $OP_BOOL)
+ // }
+ (DataType::Int8, DataType::Int8) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Int8Array, $OP, Int8Type)
+ }
+ (DataType::Int16, DataType::Int16) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Int16Array, $OP, Int16Type)
+ }
+ (DataType::Int32, DataType::Int32) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Int32Array, $OP, Int32Type)
+ }
+ (DataType::Int64, DataType::Int64) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Int64Array, $OP, Int64Type)
+ }
+ (DataType::UInt8, DataType::UInt8) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, UInt8Array, $OP, UInt8Type)
+ }
+ (DataType::UInt16, DataType::UInt16) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, UInt16Array, $OP, UInt16Type)
+ }
+ (DataType::UInt32, DataType::UInt32) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, UInt32Array, $OP, UInt32Type)
+ }
+ (DataType::UInt64, DataType::UInt64) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, UInt64Array, $OP, UInt64Type)
+ }
+ (DataType::Float32, DataType::Float32) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Float32Array, $OP, Float32Type)
+ }
+ (DataType::Float64, DataType::Float64) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Float64Array, $OP, Float64Type)
+ }
+ (DataType::Utf8, DataType::Utf8) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, StringArray, $OP, i32)
+ }
+ (DataType::LargeUtf8, DataType::LargeUtf8) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, LargeStringArray, $OP, i64)
+ }
+ (DataType::Dictionary(DataType::UInt8, DataType::UInt8), DataType::UInt8) => {
+ let values_comp =
+ typed_compare_scalar!($LEFT.values(), $RIGHT, eq_scalar);
+ unpack_dict_comparison($LEFT, values_comp)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing arrays of type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare an array with a scalar of different type ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+/// Perform `left == right` operation on an array and a numeric scalar
+/// value. Supports PrimtiveArrays, and DictionaryArrays that have primitive values
+pub fn eq_dyn_scalar<T>(left: &dyn Array, right: T) -> Result<BooleanArray>
+where
+ T: IntoArrowNumericType + TryInto<i128> + Copy + std::fmt::Debug,
+{
+ dyn_compare_scalar!(left, right, eq_scalar)
+}
+
+/// unpacks the results of comparing left.values (as a boolean)
+///
+/// TODO add example
+///
+fn unpack_dict_comparison<K>(
+ left: &DictionaryArray<K>,
+ dict_comparison: BooleanArray,
+) -> Result<BooleanArray>
+where
+ K: ArrowNumericType,
+{
+ assert_eq!(dict_comparison.len(), left.values().len());
Review comment:
I think it is an invariant (namely that `left` is the dictionary and `dict_comparison` is the result of comparing those values).
Perhaps we could rename the `left` parameter to `dict` to make this clearer
--
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] matthewmturner commented on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1000071551
Ive made a number of updates to simplify this, such as de-macroing a level, and have finally made our test pass. Of course there would need to be considerable cleanup to his but I think that this could demonstrate an approach to making this work. To be fair, ive thought ive been close about 5 times now and have been wrong each time so definitely happy to get more insight from others.
@alamb as always your feedback greatly appreciated.
@shepmaster thank you for the review before. ive cleaned up based on some of your feedback with the goal of trying to demonstrate in simpler terms (i.e. less macros) what were trying to do. im still quite new to rust so im not sure how to manage inlining `dyn_compare_scalar`. ive started to read up on it but will definitely take more time to get an understanding. If you could provide any additional color on that it would be greatly appreciated, else I can follow up once i have a better understanding). also would be interested in what @alamb has to say about it.
--
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] matthewmturner commented on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1000074976
also as a note, i added the `IntoArrowNumericType` just for testing purposes here, of course it wouldnt actually be added there. I also wasnt sure how to go about accessing the associated type `Arrow` (i.e. in a match) so added a method to get the relevant arrow type.
--
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] matthewmturner edited a comment on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner edited a comment on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1000071551
Ive made a number of updates to simplify this, such as de-macroing a level, and have finally made our test pass. Of course there would need to be considerable cleanup to this but I think that this could demonstrate an approach to making this work. To be fair, ive thought ive been close about 5 times now and have been wrong each time so definitely looking to get more insight from others.
@alamb as always your feedback greatly appreciated.
@shepmaster thank you for the review before. ive cleaned up based on some of your feedback with the goal of trying to demonstrate in simpler terms (i.e. less macros) what were trying to do. im still quite new to rust so im not sure how to manage inlining `dyn_compare_scalar`. ive started to read up on it but will definitely take more time to get an understanding. If you could provide any additional color on that it would be greatly appreciated, else I can follow up once i have a better understanding. also would be interested in what @alamb has to say about it.
--
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 #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-999105444
> Just to confirm, we're still expecting type annotation on the scalar right? I.e 123u8
I am hoping that we can avoid those annotations, to be honest
--
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] matthewmturner commented on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-998530309
@alamb ive started the work on defining the `eq_dyn_scalar` api. ive tried to combine some of the different points we discussed / you proposed over the course of #984. do you think this is going in the right direction?
--
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 #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1003008624
> seems im getting hit with that from_str_unchecked issue
I think it has been resolved if you merge up from master again
--
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] matthewmturner commented on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1003140851
after wandering through git hell for a bit i think i figured it out. hopefully CI passes and were good here.
--
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] matthewmturner commented on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-999191919
@alamb i think i might be getting close. I used your `TryInto<i128>` idea to enable using the kernel without type annotations.
Not sure why im hitting a recursion limit error though - will need to do some more work on that.
Do you think this is going in the right direction?
--
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] matthewmturner commented on a change in pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on a change in pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#discussion_r774753656
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -898,6 +899,224 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
compare_op_scalar!(left, right, |a, b| a >= b)
}
+macro_rules! dyn_compare_scalar {
+ ($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
+ let right: i128 = $RIGHT.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from("Can not convert scalar to i128"))
+ })?;
+ match ($LEFT.data_type(), $RIGHT.get_arrow_type()) {
+ // (DataType::Boolean, DataType::Boolean) => {
+ // typed_cmp_scalar!($LEFT, $RIGHT, BooleanArray, $OP_BOOL)
+ // }
+ (DataType::Int8, DataType::Int8) => {
+ let right: i8 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left =
+ $LEFT.as_any().downcast_ref::<Int8Array>().ok_or_else(|| {
+ ArrowError::CastError(String::from("Left array cannot be cast"))
+ })?;
Review comment:
I think these functions require an `Arc<dyn Array>` /`ArrayRef` which would mean we need the end user to do that before passing the array to the kernel or do you think its okay to add the `Arc` in the kernel?
--
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] matthewmturner commented on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1000677300
Ive added `eq_dyn_utf8_scalar` 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] codecov-commenter edited a comment on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1000071023
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1074?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 [#1074](https://codecov.io/gh/apache/arrow-rs/pull/1074?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (275b9fa) into [master](https://codecov.io/gh/apache/arrow-rs/commit/f3e452cb29b379d284b19e641c6b0b460f5bb0fa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f3e452c) will **increase** coverage by `0.00%`.
> The diff coverage is `43.90%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1074?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 #1074 +/- ##
========================================
Coverage 82.27% 82.28%
========================================
Files 168 168
Lines 49281 49471 +190
========================================
+ Hits 40547 40707 +160
- Misses 8734 8764 +30
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1074?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/comparison.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jb21wYXJpc29uLnJz) | `90.67% <43.90%> (-2.61%)` | :arrow_down: |
| [parquet/src/encodings/rle.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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-cGFycXVldC9zcmMvZW5jb2RpbmdzL3JsZS5ycw==) | `92.70% <0.00%> (ø)` | |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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.98% <0.00%> (ø)` | |
| [arrow/src/compute/kernels/cast.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jYXN0LnJz) | `95.06% <0.00%> (+0.08%)` | :arrow_up: |
| [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1074?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/1074?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 [f3e452c...275b9fa](https://codecov.io/gh/apache/arrow-rs/pull/1074?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 #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#discussion_r775856330
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -1104,27 +1104,71 @@ where
T: TryInto<i128> + Copy + std::fmt::Debug,
{
match left.data_type() {
- DataType::Dictionary(key_type, _) => {
- return dyn_compare_scalar!(&left, right, key_type, eq_scalar);
+ DataType::Dictionary(key_type, value_type) => match value_type.as_ref() {
+ DataType::Int8
+ | DataType::Int16
+ | DataType::Int32
+ | DataType::Int64
+ | DataType::UInt8
+ | DataType::UInt16
+ | DataType::UInt32
+ | DataType::UInt64 => {dyn_compare_scalar!(&left, right, key_type, eq_scalar)}
+ _ => Err(ArrowError::ComputeError(
+ "Kernel only supports PrimitiveArray or DictionaryArray with Primitive values".to_string(),
+ ))
}
- _ => {
- return dyn_compare_scalar!(&left, right, eq_scalar);
+ DataType::Int8
+ | DataType::Int16
+ | DataType::Int32
+ | DataType::Int64
+ | DataType::UInt8
+ | DataType::UInt16
+ | DataType::UInt32
+ | DataType::UInt64 => {
+ dyn_compare_scalar!(&left, right, eq_scalar)
}
- };
+ _ => Err(ArrowError::ComputeError(
+ "Kernel only supports PrimitiveArray or DictionaryArray with Primitive values".to_string(),
+ ))
+ }
}
/// Perform `left == right` operation on an array and a numeric scalar
/// value. Supports StringArrays, and DictionaryArrays that have string values
pub fn eq_dyn_utf8_scalar(left: Arc<dyn Array>, right: &str) -> Result<BooleanArray> {
- match left.data_type() {
- DataType::Dictionary(key_type, _) => {
- return dyn_compare_utf8_scalar!(&left, right, key_type, eq_utf8_scalar);
- }
- _ => {
+ let result = match left.data_type() {
+ DataType::Dictionary(key_type, value_type) => match value_type.as_ref() {
+ DataType::Utf8 | DataType::LargeUtf8 => {
+ dyn_compare_utf8_scalar!(&left, right, key_type, eq_utf8_scalar)
+ }
+ _ => Err(ArrowError::ComputeError(
+ "Kernel only supports Utf8 or LargeUtf8 arrays or DictionaryArray with Utf8 or LargeUtf8 values".to_string(),
+ )),
+ },
+ DataType::Utf8 | DataType::LargeUtf8 => {
let left = as_string_array(&left);
- return eq_utf8_scalar(left, right);
+ eq_utf8_scalar(left, right)
}
+ _ => Err(ArrowError::ComputeError(
+ "Kernel only supports Utf8 or LargeUtf8 arrays".to_string(),
+ )),
};
+ result
+}
+
+/// Perform `left == right` operation on an array and a numeric scalar
+/// value. Supports BooleanArrays, and DictionaryArrays that have string values
+pub fn eq_dyn_bool_scalar(left: Arc<dyn Array>, right: bool) -> Result<BooleanArray> {
+ let result = match left.data_type() {
+ DataType::Boolean => {
+ let left = as_boolean_array(&left);
+ eq_bool_scalar(left, right)
+ }
+ _ => Err(ArrowError::ComputeError(
+ "Kernel only supports BooleanArray".to_string(),
Review comment:
Yeah -- users can call `cast` if they want to convert their array into boolean first
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -1104,27 +1104,71 @@ where
T: TryInto<i128> + Copy + std::fmt::Debug,
{
match left.data_type() {
- DataType::Dictionary(key_type, _) => {
- return dyn_compare_scalar!(&left, right, key_type, eq_scalar);
+ DataType::Dictionary(key_type, value_type) => match value_type.as_ref() {
+ DataType::Int8
+ | DataType::Int16
+ | DataType::Int32
+ | DataType::Int64
+ | DataType::UInt8
+ | DataType::UInt16
+ | DataType::UInt32
+ | DataType::UInt64 => {dyn_compare_scalar!(&left, right, key_type, eq_scalar)}
+ _ => Err(ArrowError::ComputeError(
+ "Kernel only supports PrimitiveArray or DictionaryArray with Primitive values".to_string(),
+ ))
}
- _ => {
- return dyn_compare_scalar!(&left, right, eq_scalar);
+ DataType::Int8
+ | DataType::Int16
+ | DataType::Int32
+ | DataType::Int64
+ | DataType::UInt8
+ | DataType::UInt16
+ | DataType::UInt32
+ | DataType::UInt64 => {
+ dyn_compare_scalar!(&left, right, eq_scalar)
}
- };
+ _ => Err(ArrowError::ComputeError(
+ "Kernel only supports PrimitiveArray or DictionaryArray with Primitive values".to_string(),
+ ))
+ }
}
/// Perform `left == right` operation on an array and a numeric scalar
/// value. Supports StringArrays, and DictionaryArrays that have string values
pub fn eq_dyn_utf8_scalar(left: Arc<dyn Array>, right: &str) -> Result<BooleanArray> {
Review comment:
love it
--
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] shepmaster commented on a change in pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
shepmaster commented on a change in pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#discussion_r774003760
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -898,6 +898,126 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
compare_op_scalar!(left, right, |a, b| a >= b)
}
+macro_rules! dyn_cmp_scalar {
+ ($LEFT: expr, $RIGHT: expr, $T: ident, $OP: ident, $TT: tt) => {{
Review comment:
Should `$T` and `$TT` be [`ty`](https://doc.rust-lang.org/stable/reference/macros-by-example.html#metavariables)?
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -898,6 +898,126 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
compare_op_scalar!(left, right, |a, b| a >= b)
}
+macro_rules! dyn_cmp_scalar {
+ ($LEFT: expr, $RIGHT: expr, $T: ident, $OP: ident, $TT: tt) => {{
+ let left = $LEFT.as_any().downcast_ref::<$T>().ok_or_else(|| {
+ ArrowError::CastError(format!(
+ "Left array cannot be cast to {}",
+ type_name::<$T>()
+ ))
+ })?;
+ let right = $RIGHT.as_any().downcast_ref::<$T>().ok_or_else(|| {
+ ArrowError::CastError(format!(
+ "Right array cannot be cast to {}",
+ type_name::<$T>(),
+ ))
+ })?;
+ $OP::<$TT>(left, right)
Review comment:
`$OP::<$TT>` could probably be fused as one `expr` macro argument
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -898,6 +898,126 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
compare_op_scalar!(left, right, |a, b| a >= b)
}
+macro_rules! dyn_cmp_scalar {
+ ($LEFT: expr, $RIGHT: expr, $T: ident, $OP: ident, $TT: tt) => {{
+ let left = $LEFT.as_any().downcast_ref::<$T>().ok_or_else(|| {
+ ArrowError::CastError(format!(
+ "Left array cannot be cast to {}",
+ type_name::<$T>()
+ ))
+ })?;
+ let right = $RIGHT.as_any().downcast_ref::<$T>().ok_or_else(|| {
+ ArrowError::CastError(format!(
+ "Right array cannot be cast to {}",
+ type_name::<$T>(),
+ ))
+ })?;
+ $OP::<$TT>(left, right)
+ }};
+}
+
+macro_rules! dyn_compare_scalar {
+ ($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
+ let right = $RIGHT.try_into().map_err(|_| {
+ ArrowError::ComputeError(format!(
+ "Can not convert scalar {:?} to i128",
+ $RIGHT
+ ))
+ });
+ match ($LEFT.data_type(), $RIGHT::Arrow) {
+ // (DataType::Boolean, DataType::Boolean) => {
+ // typed_cmp_scalar!($LEFT, $RIGHT, BooleanArray, $OP_BOOL)
+ // }
+ (DataType::Int8, DataType::Int8) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Int8Array, $OP, Int8Type)
+ }
+ (DataType::Int16, DataType::Int16) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Int16Array, $OP, Int16Type)
+ }
+ (DataType::Int32, DataType::Int32) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Int32Array, $OP, Int32Type)
+ }
+ (DataType::Int64, DataType::Int64) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Int64Array, $OP, Int64Type)
+ }
+ (DataType::UInt8, DataType::UInt8) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, UInt8Array, $OP, UInt8Type)
+ }
+ (DataType::UInt16, DataType::UInt16) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, UInt16Array, $OP, UInt16Type)
+ }
+ (DataType::UInt32, DataType::UInt32) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, UInt32Array, $OP, UInt32Type)
+ }
+ (DataType::UInt64, DataType::UInt64) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, UInt64Array, $OP, UInt64Type)
+ }
+ (DataType::Float32, DataType::Float32) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Float32Array, $OP, Float32Type)
+ }
+ (DataType::Float64, DataType::Float64) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Float64Array, $OP, Float64Type)
+ }
+ (DataType::Utf8, DataType::Utf8) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, StringArray, $OP, i32)
+ }
+ (DataType::LargeUtf8, DataType::LargeUtf8) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, LargeStringArray, $OP, i64)
+ }
+ (DataType::Dictionary(DataType::UInt8, DataType::UInt8), DataType::UInt8) => {
+ let values_comp =
+ typed_compare_scalar!($LEFT.values(), $RIGHT, eq_scalar);
+ unpack_dict_comparison($LEFT, values_comp)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing arrays of type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare an array with a scalar of different type ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+/// Perform `left == right` operation on an array and a numeric scalar
+/// value. Supports PrimtiveArrays, and DictionaryArrays that have primitive values
Review comment:
```suggestion
/// value. Supports PrimitiveArrays, and DictionaryArrays that have primitive values
```
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -898,6 +898,126 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
compare_op_scalar!(left, right, |a, b| a >= b)
}
+macro_rules! dyn_cmp_scalar {
+ ($LEFT: expr, $RIGHT: expr, $T: ident, $OP: ident, $TT: tt) => {{
+ let left = $LEFT.as_any().downcast_ref::<$T>().ok_or_else(|| {
+ ArrowError::CastError(format!(
+ "Left array cannot be cast to {}",
+ type_name::<$T>()
+ ))
+ })?;
+ let right = $RIGHT.as_any().downcast_ref::<$T>().ok_or_else(|| {
+ ArrowError::CastError(format!(
+ "Right array cannot be cast to {}",
+ type_name::<$T>(),
+ ))
+ })?;
+ $OP::<$TT>(left, right)
+ }};
+}
+
+macro_rules! dyn_compare_scalar {
+ ($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
+ let right = $RIGHT.try_into().map_err(|_| {
+ ArrowError::ComputeError(format!(
+ "Can not convert scalar {:?} to i128",
+ $RIGHT
+ ))
+ });
+ match ($LEFT.data_type(), $RIGHT::Arrow) {
+ // (DataType::Boolean, DataType::Boolean) => {
+ // typed_cmp_scalar!($LEFT, $RIGHT, BooleanArray, $OP_BOOL)
+ // }
+ (DataType::Int8, DataType::Int8) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Int8Array, $OP, Int8Type)
+ }
+ (DataType::Int16, DataType::Int16) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Int16Array, $OP, Int16Type)
+ }
+ (DataType::Int32, DataType::Int32) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Int32Array, $OP, Int32Type)
+ }
+ (DataType::Int64, DataType::Int64) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Int64Array, $OP, Int64Type)
+ }
+ (DataType::UInt8, DataType::UInt8) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, UInt8Array, $OP, UInt8Type)
+ }
+ (DataType::UInt16, DataType::UInt16) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, UInt16Array, $OP, UInt16Type)
+ }
+ (DataType::UInt32, DataType::UInt32) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, UInt32Array, $OP, UInt32Type)
+ }
+ (DataType::UInt64, DataType::UInt64) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, UInt64Array, $OP, UInt64Type)
+ }
+ (DataType::Float32, DataType::Float32) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Float32Array, $OP, Float32Type)
+ }
+ (DataType::Float64, DataType::Float64) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Float64Array, $OP, Float64Type)
+ }
+ (DataType::Utf8, DataType::Utf8) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, StringArray, $OP, i32)
+ }
+ (DataType::LargeUtf8, DataType::LargeUtf8) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, LargeStringArray, $OP, i64)
+ }
+ (DataType::Dictionary(DataType::UInt8, DataType::UInt8), DataType::UInt8) => {
+ let values_comp =
+ typed_compare_scalar!($LEFT.values(), $RIGHT, eq_scalar);
+ unpack_dict_comparison($LEFT, values_comp)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing arrays of type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare an array with a scalar of different type ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+/// Perform `left == right` operation on an array and a numeric scalar
+/// value. Supports PrimtiveArrays, and DictionaryArrays that have primitive values
+pub fn eq_dyn_scalar<T>(left: &dyn Array, right: T) -> Result<BooleanArray>
+where
+ T: IntoArrowNumericType + TryInto<i128> + Copy + std::fmt::Debug,
+{
+ dyn_compare_scalar!(left, right, eq_scalar)
Review comment:
I admit this is a drive-by review, but I'm not seeing the benefit of the macros here yet. They don't do any repetition reduction. It looks like `dyn_compare_scalar` could be inlined and `dyn_cmp_scalar` could be a regular function.
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -898,6 +898,126 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
compare_op_scalar!(left, right, |a, b| a >= b)
}
+macro_rules! dyn_cmp_scalar {
+ ($LEFT: expr, $RIGHT: expr, $T: ident, $OP: ident, $TT: tt) => {{
+ let left = $LEFT.as_any().downcast_ref::<$T>().ok_or_else(|| {
+ ArrowError::CastError(format!(
+ "Left array cannot be cast to {}",
+ type_name::<$T>()
+ ))
+ })?;
+ let right = $RIGHT.as_any().downcast_ref::<$T>().ok_or_else(|| {
+ ArrowError::CastError(format!(
+ "Right array cannot be cast to {}",
+ type_name::<$T>(),
+ ))
+ })?;
+ $OP::<$TT>(left, right)
+ }};
+}
+
+macro_rules! dyn_compare_scalar {
+ ($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
+ let right = $RIGHT.try_into().map_err(|_| {
Review comment:
I'm missing where this `right` value is used...
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -898,6 +898,126 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
compare_op_scalar!(left, right, |a, b| a >= b)
}
+macro_rules! dyn_cmp_scalar {
+ ($LEFT: expr, $RIGHT: expr, $T: ident, $OP: ident, $TT: tt) => {{
+ let left = $LEFT.as_any().downcast_ref::<$T>().ok_or_else(|| {
+ ArrowError::CastError(format!(
+ "Left array cannot be cast to {}",
+ type_name::<$T>()
+ ))
+ })?;
+ let right = $RIGHT.as_any().downcast_ref::<$T>().ok_or_else(|| {
+ ArrowError::CastError(format!(
+ "Right array cannot be cast to {}",
+ type_name::<$T>(),
+ ))
+ })?;
+ $OP::<$TT>(left, right)
+ }};
+}
+
+macro_rules! dyn_compare_scalar {
+ ($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
+ let right = $RIGHT.try_into().map_err(|_| {
+ ArrowError::ComputeError(format!(
+ "Can not convert scalar {:?} to i128",
+ $RIGHT
+ ))
+ });
+ match ($LEFT.data_type(), $RIGHT::Arrow) {
+ // (DataType::Boolean, DataType::Boolean) => {
+ // typed_cmp_scalar!($LEFT, $RIGHT, BooleanArray, $OP_BOOL)
+ // }
+ (DataType::Int8, DataType::Int8) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Int8Array, $OP, Int8Type)
+ }
+ (DataType::Int16, DataType::Int16) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Int16Array, $OP, Int16Type)
+ }
+ (DataType::Int32, DataType::Int32) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Int32Array, $OP, Int32Type)
+ }
+ (DataType::Int64, DataType::Int64) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Int64Array, $OP, Int64Type)
+ }
+ (DataType::UInt8, DataType::UInt8) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, UInt8Array, $OP, UInt8Type)
+ }
+ (DataType::UInt16, DataType::UInt16) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, UInt16Array, $OP, UInt16Type)
+ }
+ (DataType::UInt32, DataType::UInt32) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, UInt32Array, $OP, UInt32Type)
+ }
+ (DataType::UInt64, DataType::UInt64) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, UInt64Array, $OP, UInt64Type)
+ }
+ (DataType::Float32, DataType::Float32) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Float32Array, $OP, Float32Type)
+ }
+ (DataType::Float64, DataType::Float64) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, Float64Array, $OP, Float64Type)
+ }
+ (DataType::Utf8, DataType::Utf8) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, StringArray, $OP, i32)
+ }
+ (DataType::LargeUtf8, DataType::LargeUtf8) => {
+ dyn_cmp_scalar!($LEFT, $RIGHT, LargeStringArray, $OP, i64)
+ }
+ (DataType::Dictionary(DataType::UInt8, DataType::UInt8), DataType::UInt8) => {
+ let values_comp =
+ typed_compare_scalar!($LEFT.values(), $RIGHT, eq_scalar);
+ unpack_dict_comparison($LEFT, values_comp)
+ }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing arrays of type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare an array with a scalar of different type ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+/// Perform `left == right` operation on an array and a numeric scalar
+/// value. Supports PrimtiveArrays, and DictionaryArrays that have primitive values
+pub fn eq_dyn_scalar<T>(left: &dyn Array, right: T) -> Result<BooleanArray>
+where
+ T: IntoArrowNumericType + TryInto<i128> + Copy + std::fmt::Debug,
+{
+ dyn_compare_scalar!(left, right, eq_scalar)
+}
+
+/// unpacks the results of comparing left.values (as a boolean)
+///
+/// TODO add example
+///
+fn unpack_dict_comparison<K>(
+ left: &DictionaryArray<K>,
+ dict_comparison: BooleanArray,
+) -> Result<BooleanArray>
+where
+ K: ArrowNumericType,
+{
+ assert_eq!(dict_comparison.len(), left.values().len());
Review comment:
Why an assertion as opposed to an error or a "no this does not match?"
--
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] matthewmturner commented on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1003488667
@alamb @liukun4515 one PR per kernel it is :)
--
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] matthewmturner commented on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1003122974
having issues with rebasing. for whatever reason when i rebase to upstream master im losing all my recent commits. still looking into it.
--
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 #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1000071023
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1074?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 [#1074](https://codecov.io/gh/apache/arrow-rs/pull/1074?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c0f0a98) into [master](https://codecov.io/gh/apache/arrow-rs/commit/f3e452cb29b379d284b19e641c6b0b460f5bb0fa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f3e452c) will **increase** coverage by `0.03%`.
> The diff coverage is `75.38%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1074?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 #1074 +/- ##
==========================================
+ Coverage 82.27% 82.31% +0.03%
==========================================
Files 168 168
Lines 49281 49497 +216
==========================================
+ Hits 40547 40741 +194
- Misses 8734 8756 +22
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1074?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/comparison.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jb21wYXJpc29uLnJz) | `91.63% <75.38%> (-1.64%)` | :arrow_down: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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.52% <0.00%> (-0.20%)` | :arrow_down: |
| [parquet/src/encodings/rle.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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-cGFycXVldC9zcmMvZW5jb2RpbmdzL3JsZS5ycw==) | `92.70% <0.00%> (ø)` | |
| [arrow/src/compute/kernels/cast.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jYXN0LnJz) | `95.06% <0.00%> (+0.08%)` | :arrow_up: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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.22%)` | :arrow_up: |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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=) | `85.00% <0.00%> (+0.27%)` | :arrow_up: |
| [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/1074/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: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1074?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/1074?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 [f3e452c...c0f0a98](https://codecov.io/gh/apache/arrow-rs/pull/1074?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] matthewmturner commented on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1001331324
@alamb if you're okay with the latest changes and can merge then ill prioritize doing another PR with the other comparison functions.
thank you for all the guidance youve provided on this!
--
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] liukun4515 commented on a change in pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#discussion_r775446288
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -898,6 +900,305 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
compare_op_scalar!(left, right, |a, b| a >= b)
}
+macro_rules! dyn_compare_scalar {
+ ($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
+ let right: i128 = $RIGHT.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from("Can not convert scalar to i128"))
+ })?;
+ match $LEFT.data_type() {
+ DataType::Int8 => {
+ let right: i8 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
Review comment:
`Can not convert scalar to i128` -> `Can not convert scalar to i8`
--
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] matthewmturner commented on a change in pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on a change in pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#discussion_r774044776
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -898,6 +898,126 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
compare_op_scalar!(left, right, |a, b| a >= b)
}
+macro_rules! dyn_cmp_scalar {
+ ($LEFT: expr, $RIGHT: expr, $T: ident, $OP: ident, $TT: tt) => {{
+ let left = $LEFT.as_any().downcast_ref::<$T>().ok_or_else(|| {
+ ArrowError::CastError(format!(
+ "Left array cannot be cast to {}",
+ type_name::<$T>()
+ ))
+ })?;
+ let right = $RIGHT.as_any().downcast_ref::<$T>().ok_or_else(|| {
+ ArrowError::CastError(format!(
+ "Right array cannot be cast to {}",
+ type_name::<$T>(),
+ ))
+ })?;
+ $OP::<$TT>(left, right)
+ }};
+}
+
+macro_rules! dyn_compare_scalar {
+ ($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
+ let right = $RIGHT.try_into().map_err(|_| {
Review comment:
I just hadn't yet updated where $RIGHT is used in all the match arms to use right instead
--
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] matthewmturner edited a comment on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner edited a comment on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1000074976
also as a note, i added the `IntoArrowNumericType` just for testing purposes here, of course it wouldnt actually be added there or in this PR. I also wasnt sure how to go about accessing the associated type `Arrow` of that trait (i.e. in a match) so added a method to get the relevant arrow type.
--
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] matthewmturner commented on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1003404937
@alamb odd, i had made updates for those suggestions. must have been missed with my rebase.
ive made the fixes.
i can make an issue for it - and im hoping to work on implementing it this weekend.
do you have a preference for how small (i.e how many kernels) youd like each pr / issue to be?
--
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] matthewmturner commented on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1002149093
@alamb updates made!
--
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] liukun4515 commented on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1003480717
> > i can make an issue for it - and im hoping to work on implementing it this weekend.
>
> Thanks @matthewmturner ❤️
>
> > do you have a preference for how small (i.e how many kernels) youd like each pr / issue to be?
>
> My personal bias is one PR per kernel (as that makes it easiest to review) but I am also happy to review a single PR too
I think one pr per kernel is better.
The pr including all updates may be large, and it is not friendly to reviewers.
@matthewmturner
--
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 #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1002031511
> Sure sounds good. Should I just add those on this PR?
I recommend doing the code in multiple PRs to keep the reviews smaller.
> actually - are bool values currently supported with DictionaryArray?
I think they would be valid per the arrow spec, but I don't think they would be very useful -- A dictionary array of bools will be (much) less efficient both in term of space and CPU than a BooleanArray. This is probably why there is no array builder for them https://docs.rs/arrow/6.4.0/arrow/array/struct.DictionaryArray.html?search=dictionarybuilder
I think it is fine to dictionary of bools unimplemented and people can implement them if they want
--
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] matthewmturner commented on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1002149679
seems im getting hit with that `from_str_unchecked` issue
--
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] matthewmturner commented on pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-999087609
@alamb yes agree. That's what I'm going to work on next, the first commit was basically just what you said (copy/paste) with minimal cleaning to show the high level api. Now going to try making macros that can be used for all dyn_scalar kernels.
Just to confirm, we're still expecting type annotation on the scalar right? I.e 123u8
--
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] matthewmturner commented on a change in pull request #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
matthewmturner commented on a change in pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#discussion_r774611115
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -898,6 +899,224 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
compare_op_scalar!(left, right, |a, b| a >= b)
}
+macro_rules! dyn_compare_scalar {
+ ($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
+ let right: i128 = $RIGHT.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from("Can not convert scalar to i128"))
+ })?;
+ match ($LEFT.data_type(), $RIGHT.get_arrow_type()) {
+ // (DataType::Boolean, DataType::Boolean) => {
+ // typed_cmp_scalar!($LEFT, $RIGHT, BooleanArray, $OP_BOOL)
+ // }
+ (DataType::Int8, DataType::Int8) => {
+ let right: i8 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left =
+ $LEFT.as_any().downcast_ref::<Int8Array>().ok_or_else(|| {
+ ArrowError::CastError(String::from("Left array cannot be cast"))
+ })?;
+ $OP::<Int8Type>(left, right)
+ }
+ (DataType::Int16, DataType::Int16) => {
+ let right: i16 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left =
+ $LEFT.as_any().downcast_ref::<Int16Array>().ok_or_else(|| {
+ ArrowError::CastError(String::from("Left array cannot be cast"))
+ })?;
+ $OP::<Int16Type>(left, right)
+ }
+ (DataType::Int32, DataType::Int32) => {
+ let right: i32 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left =
+ $LEFT.as_any().downcast_ref::<Int32Array>().ok_or_else(|| {
+ ArrowError::CastError(String::from("Left array cannot be cast"))
+ })?;
+ $OP::<Int32Type>(left, right)
+ }
+ (DataType::Int64, DataType::Int64) => {
+ let right: i64 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left =
+ $LEFT.as_any().downcast_ref::<Int64Array>().ok_or_else(|| {
+ ArrowError::CastError(String::from("Left array cannot be cast"))
+ })?;
+ $OP::<Int64Type>(left, right)
+ }
+ // (DataType::UInt8, DataType::UInt8) => {
+ // dyn_cmp_scalar!($LEFT, right, UInt8Array, $OP, UInt8Type)
+ // }
+ // (DataType::UInt16, DataType::UInt16) => {
+ // dyn_cmp_scalar!($LEFT, right, UInt16Array, $OP, UInt16Type)
+ // }
+ // (DataType::UInt32, DataType::UInt32) => {
+ // dyn_cmp_scalar!($LEFT, right, UInt32Array, $OP, UInt32Type)
+ // }
+ // (DataType::UInt64, DataType::UInt64) => {
+ // dyn_cmp_scalar!($LEFT, right, UInt64Array, $OP, UInt64Type)
+ // }
+ // (DataType::Float32, DataType::Float32) => {
+ // dyn_cmp_scalar!($LEFT, right, Float32Array, $OP, Float32Type)
+ // }
+ // (DataType::Float64, DataType::Float64) => {
+ // dyn_cmp_scalar!($LEFT, right, Float64Array, $OP, Float64Type)
+ // }
+ // (DataType::Utf8, DataType::Utf8) => {
+ // let right: i32 = right.try_into().map_err(|_| {
+ // ArrowError::ComputeError(String::from(
+ // "Can not convert scalar to i128",
+ // ))
+ // })?;
+ // let left =
+ // $LEFT
+ // .as_any()
+ // .downcast_ref::<StringArray>()
+ // .ok_or_else(|| {
+ // ArrowError::CastError(String::from(
+ // "Left array cannot be cast",
+ // ))
+ // })?;
+ // $OP::<Int32Type>(left, right)
+ // }
+ // (DataType::LargeUtf8, DataType::LargeUtf8) => {
+ // dyn_cmp_scalar!($LEFT, $RIGHT, LargeStringArray, $OP, i64)
+ // }
+ (t1, t2) if t1 == t2 => Err(ArrowError::NotYetImplemented(format!(
+ "Comparing arrays of type {} is not yet implemented",
+ t1
+ ))),
+ (t1, t2) => Err(ArrowError::CastError(format!(
+ "Cannot compare an array with a scalar of different type ({} and {})",
+ t1, t2
+ ))),
+ }
+ }};
+}
+
+pub trait IntoArrowNumericType {
+ // type Arrow: ArrowNumericType<Native = Self>;
+ fn get_arrow_type(&self) -> &DataType;
+}
+
+impl IntoArrowNumericType for i8 {
+ // type Arrow = Int8Type;
+ fn get_arrow_type(&self) -> &DataType {
+ &DataType::Int8
+ }
+}
+
+impl IntoArrowNumericType for i16 {
+ // type Arrow = Int8Type;
+ fn get_arrow_type(&self) -> &DataType {
+ &DataType::Int16
+ }
+}
+
+impl IntoArrowNumericType for i32 {
+ // type Arrow = Int8Type;
+ fn get_arrow_type(&self) -> &DataType {
+ &DataType::Int32
+ }
+}
+
+/// Perform `left == right` operation on an array and a numeric scalar
+/// value. Supports PrimitiveArrays, and DictionaryArrays that have primitive values
+pub fn eq_dyn_scalar<T>(left: &dyn Array, right: T) -> Result<BooleanArray>
+where
+ T: IntoArrowNumericType + TryInto<i128> + Copy + std::fmt::Debug,
Review comment:
Yes agree, I was starting to think the same. Will try it out.
--
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 #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#discussion_r775243362
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -898,6 +900,261 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
compare_op_scalar!(left, right, |a, b| a >= b)
}
+macro_rules! dyn_compare_scalar {
+ ($LEFT: expr, $RIGHT: expr, $OP: ident) => {{
+ let right: i128 = $RIGHT.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from("Can not convert scalar to i128"))
+ })?;
+ match $LEFT.data_type() {
+ DataType::Int8 => {
+ let right: i8 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left = as_primitive_array::<Int8Type>($LEFT);
+ $OP::<Int8Type>(left, right)
+ }
+ DataType::Int16 => {
+ let right: i16 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left = as_primitive_array::<Int16Type>($LEFT);
+ $OP::<Int16Type>(left, right)
+ }
+ DataType::Int32 => {
+ let right: i32 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left = as_primitive_array::<Int32Type>($LEFT);
+ $OP::<Int32Type>(left, right)
+ }
+ DataType::Int64 => {
+ let right: i64 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left = as_primitive_array::<Int64Type>($LEFT);
+ $OP::<Int64Type>(left, right)
+ }
+ DataType::UInt8 => {
+ let right: u8 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left = as_primitive_array::<UInt8Type>($LEFT);
+ $OP::<UInt8Type>(left, right)
+ }
+ DataType::UInt16 => {
+ let right: u16 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left = as_primitive_array::<UInt16Type>($LEFT);
+ $OP::<UInt16Type>(left, right)
+ }
+ DataType::UInt32 => {
+ let right: u32 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left = as_primitive_array::<UInt32Type>($LEFT);
+ $OP::<UInt32Type>(left, right)
+ }
+ DataType::UInt64 => {
+ let right: u64 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from(
+ "Can not convert scalar to i128",
+ ))
+ })?;
+ let left = as_primitive_array::<UInt64Type>($LEFT);
+ $OP::<UInt64Type>(left, right)
+ }
+ _ => Err(ArrowError::ComputeError(String::from(
+ "Unsupported data type",
+ ))),
+ }
+ }};
+ ($LEFT: expr, $RIGHT: expr, $KT: ident, $OP: ident) => {{
+ let right: i128 = $RIGHT.try_into().map_err(|_| {
+ ArrowError::ComputeError(String::from("Can not convert scalar to i128"))
+ })?;
+ match $KT.as_ref() {
+ DataType::UInt8 => {
+ let left = as_dictionary_array::<UInt8Type>($LEFT);
+ unpack_dict_comparison(
+ left,
+ dyn_compare_scalar!(left.values(), right, $OP)?,
+ )
+ }
+ DataType::UInt16 => {
+ let left = as_dictionary_array::<UInt16Type>($LEFT);
+ unpack_dict_comparison(
+ left,
+ dyn_compare_scalar!(left.values(), right, $OP)?,
+ )
+ }
+ DataType::UInt32 => {
+ let left = as_dictionary_array::<UInt32Type>($LEFT);
+ unpack_dict_comparison(
+ left,
+ dyn_compare_scalar!(left.values(), right, $OP)?,
+ )
+ }
+ DataType::UInt64 => {
+ let left = as_dictionary_array::<UInt64Type>($LEFT);
+ unpack_dict_comparison(
+ left,
+ dyn_compare_scalar!(left.values(), right, $OP)?,
+ )
+ }
+ DataType::Int8 => {
+ let left = as_dictionary_array::<Int8Type>($LEFT);
+ unpack_dict_comparison(
+ left,
+ dyn_compare_scalar!(left.values(), right, $OP)?,
+ )
+ }
+ DataType::Int16 => {
+ let left = as_dictionary_array::<Int16Type>($LEFT);
+ unpack_dict_comparison(
+ left,
+ dyn_compare_scalar!(left.values(), right, $OP)?,
+ )
+ }
+ DataType::Int32 => {
+ let left = as_dictionary_array::<Int32Type>($LEFT);
+ unpack_dict_comparison(
+ left,
+ dyn_compare_scalar!(left.values(), right, $OP)?,
+ )
+ }
+ DataType::Int64 => {
+ let left = as_dictionary_array::<Int64Type>($LEFT);
+ unpack_dict_comparison(
+ left,
+ dyn_compare_scalar!(left.values(), right, $OP)?,
+ )
+ }
+ _ => Err(ArrowError::ComputeError(String::from("Unknown key type"))),
+ }
+ }};
+}
+
+macro_rules! dyn_compare_utf8_scalar {
+ ($LEFT: expr, $RIGHT: expr, $KT: ident, $OP: ident) => {{
+ match $KT.as_ref() {
+ DataType::UInt8 => {
+ let left = as_dictionary_array::<UInt8Type>($LEFT);
+ let values = as_string_array(left.values());
+ unpack_dict_comparison(left, $OP(values, $RIGHT)?)
+ }
+ DataType::UInt16 => {
+ let left = as_dictionary_array::<UInt16Type>($LEFT);
+ let values = as_string_array(left.values());
+ unpack_dict_comparison(left, $OP(values, $RIGHT)?)
+ }
+ DataType::UInt32 => {
+ let left = as_dictionary_array::<UInt32Type>($LEFT);
+ let values = as_string_array(left.values());
+ unpack_dict_comparison(left, $OP(values, $RIGHT)?)
+ }
+ DataType::UInt64 => {
+ let left = as_dictionary_array::<UInt64Type>($LEFT);
+ let values = as_string_array(left.values());
+ unpack_dict_comparison(left, $OP(values, $RIGHT)?)
+ }
+ DataType::Int8 => {
+ let left = as_dictionary_array::<Int8Type>($LEFT);
+ let values = as_string_array(left.values());
+ unpack_dict_comparison(left, $OP(values, $RIGHT)?)
+ }
+ DataType::Int16 => {
+ let left = as_dictionary_array::<Int16Type>($LEFT);
+ let values = as_string_array(left.values());
+ unpack_dict_comparison(left, $OP(values, $RIGHT)?)
+ }
+ DataType::Int32 => {
+ let left = as_dictionary_array::<Int32Type>($LEFT);
+ let values = as_string_array(left.values());
+ unpack_dict_comparison(left, $OP(values, $RIGHT)?)
+ }
+ DataType::Int64 => {
+ let left = as_dictionary_array::<Int64Type>($LEFT);
+ let values = as_string_array(left.values());
+ unpack_dict_comparison(left, $OP(values, $RIGHT)?)
+ }
+ _ => Err(ArrowError::ComputeError(String::from("Unknown key type"))),
+ }
+ }};
+}
+
+/// Perform `left == right` operation on an array and a numeric scalar
+/// value. Supports PrimitiveArrays, and DictionaryArrays that have primitive values
+pub fn eq_dyn_scalar<T>(left: Arc<dyn Array>, right: T) -> Result<BooleanArray>
+where
+ T: TryInto<i128> + Copy + std::fmt::Debug,
+{
+ match left.data_type() {
+ DataType::Dictionary(key_type, _) => {
+ return dyn_compare_scalar!(&left, right, key_type, eq_scalar);
+ }
+ _ => {
+ return dyn_compare_scalar!(&left, right, eq_scalar);
+ }
+ };
+}
+
+/// Perform `left == right` operation on an array and a numeric scalar
+/// value. Supports StringArrays, and DictionaryArrays that have string values
+pub fn eq_dyn_utf8_scalar(left: Arc<dyn Array>, right: &str) -> Result<BooleanArray> {
+ match left.data_type() {
+ DataType::Dictionary(key_type, _) => {
+ return dyn_compare_utf8_scalar!(&left, right, key_type, eq_utf8_scalar);
+ }
+ _ => {
Review comment:
this probably wants to match on the `DataType::Utf8` and `DataType::LargeUtf8` but otherwise looks good to me
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2522,4 +2779,60 @@ mod tests {
regexp_is_match_utf8_scalar,
vec![true, true, false, false]
);
+ #[test]
+ fn test_eq_dyn_scalar() {
+ let array = Int32Array::from(vec![6, 7, 8, 8, 10]);
+ let array = Arc::new(array);
+ let a_eq = eq_dyn_scalar(array, 8).unwrap();
Review comment:
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2522,4 +2779,60 @@ mod tests {
regexp_is_match_utf8_scalar,
vec![true, true, false, false]
);
+ #[test]
+ fn test_eq_dyn_scalar() {
+ let array = Int32Array::from(vec![6, 7, 8, 8, 10]);
+ let array = Arc::new(array);
+ let a_eq = eq_dyn_scalar(array, 8).unwrap();
+ assert_eq!(
+ a_eq,
+ BooleanArray::from(
+ vec![Some(false), Some(false), Some(true), Some(true), Some(false)]
+ )
+ );
+ }
+ #[test]
+ fn test_eq_dyn_scalar_with_dict() {
+ let key_builder = PrimitiveBuilder::<Int8Type>::new(3);
+ let value_builder = PrimitiveBuilder::<Int32Type>::new(2);
+ let mut builder = PrimitiveDictionaryBuilder::new(key_builder, value_builder);
+ builder.append(123).unwrap();
+ builder.append_null().unwrap();
+ builder.append(23).unwrap();
+ let array = Arc::new(builder.finish());
+ let a_eq = eq_dyn_scalar(array, 123).unwrap();
+ assert_eq!(
+ a_eq,
+ BooleanArray::from(vec![Some(true), None, Some(false)])
+ );
+ }
+ #[test]
+ fn test_eq_dyn_utf8_scalar() {
+ let array = StringArray::from(vec!["abc", "def", "xyz"]);
+ let array = Arc::new(array);
+ let a_eq = eq_dyn_utf8_scalar(array, "xyz").unwrap();
Review comment:
🎉
--
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 #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#issuecomment-1001183559
How shall we proceed -- get this one polished up (maybe also add `eq_dyn_scalar_bool`?) and then start hammering on the other kernels (e.g. `neq`, `lt`, `gt`, etc?)
--
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 #1074: Define eq_dyn_scalar API
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1074:
URL: https://github.com/apache/arrow-rs/pull/1074#discussion_r773228604
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2522,4 +2603,19 @@ mod tests {
regexp_is_match_utf8_scalar,
vec![true, true, false, false]
);
+ #[test]
+ fn test_eq_dyn_scalar_with_dict() {
+ let key_builder = PrimitiveBuilder::<UInt8Type>::new(3);
+ let value_builder = PrimitiveBuilder::<UInt8Type>::new(2);
+ let mut builder = PrimitiveDictionaryBuilder::new(key_builder, value_builder);
+ builder.append(123).unwrap();
+ builder.append_null().unwrap();
+ builder.append(223).unwrap();
+ let array = builder.finish();
+ let a_eq = eq_dyn_scalar(&array, 123).unwrap();
Review comment:
looking good to me 👍
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -898,6 +898,87 @@ pub fn gt_eq_utf8_scalar<OffsetSize: StringOffsetSizeTrait>(
compare_op_scalar!(left, right, |a, b| a >= b)
}
+pub fn eq_dict_scalar<K, T>(left: &DictionaryArray<K>, right: T) -> Result<BooleanArray>
+where
+ K: ArrowNumericType,
+ T: IntoArrowNumericType,
+{
+ unpack_dict_comparison(left, eq_dyn_scalar(left.values().as_ref(), right)?)
+}
+/// Perform `left == right` operation on an array and a numeric scalar
+/// value. Supports PrimtiveArrays, and DictionaryArrays that have primitive values
+pub fn eq_dyn_scalar<T>(left: &dyn Array, right: T) -> Result<BooleanArray>
+where
+ T: IntoArrowNumericType,
+{
+ match left.data_type() {
+ DataType::UInt8 => {
+ let right: u8 = right.try_into().map_err(|_| {
+ ArrowError::ComputeError(format!(
+ "Can not convert {:?} to u8 for comparison with UInt8Array",
+ right
+ ))
+ })?;
+ eq_scalar::<UInt8Type>(as_primitive_array::<UInt8Type>(left), right)
Review comment:
👍
--
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