You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/02/17 06:47:57 UTC
[GitHub] [arrow-rs] viirya opened a new pull request #1326: Implement DictionaryArray support in neq_dyn, lt_dyn, lt_eq_dyn, gt_dyn, gt_eq_dyn
viirya opened a new pull request #1326:
URL: https://github.com/apache/arrow-rs/pull/1326
# 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.
-->
Closes #1201.
# 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 #1326: Implement DictionaryArray support in neq_dyn, lt_dyn, lt_eq_dyn, gt_dyn, gt_eq_dyn
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1326:
URL: https://github.com/apache/arrow-rs/pull/1326#issuecomment-1042635166
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1326?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 [#1326](https://codecov.io/gh/apache/arrow-rs/pull/1326?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a9e8b67) into [master](https://codecov.io/gh/apache/arrow-rs/commit/827cc3e9e153d6cf4e6a4f8a71c760043a9f25a8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (827cc3e) will **increase** coverage by `0.00%`.
> The diff coverage is `92.30%`.
> :exclamation: Current head a9e8b67 differs from pull request most recent head 066f2b1. Consider uploading reports for the commit 066f2b1 to get more accurate results
[](https://codecov.io/gh/apache/arrow-rs/pull/1326?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 #1326 +/- ##
=======================================
Coverage 83.00% 83.00%
=======================================
Files 180 180
Lines 52919 52958 +39
=======================================
+ Hits 43924 43960 +36
- Misses 8995 8998 +3
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1326?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/1326/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) | `92.12% <92.30%> (-0.06%)` | :arrow_down: |
| [arrow/src/datatypes/field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1326/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9maWVsZC5ycw==) | `53.79% <0.00%> (-0.31%)` | :arrow_down: |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1326/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.52% <0.00%> (+0.13%)` | :arrow_up: |
| [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/1326/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9kYXRhdHlwZS5ycw==) | `66.80% <0.00%> (+0.39%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1326?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/1326?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 [827cc3e...066f2b1](https://codecov.io/gh/apache/arrow-rs/pull/1326?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a change in pull request #1326: Implement DictionaryArray support in neq_dyn, lt_dyn, lt_eq_dyn, gt_dyn, gt_eq_dyn
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1326:
URL: https://github.com/apache/arrow-rs/pull/1326#discussion_r809283121
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2318,7 +2318,7 @@ where
pub fn eq_dyn(left: &dyn Array, right: &dyn Array) -> Result<BooleanArray> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_compares!(left, right, |a, b| a == b)
+ typed_dict_compares!(left, right, |a, b| a == b, |a, b| !(a ^ b))
Review comment:
Oh, okay, I wrote it like you suggest at first, but changed it basically to make clippy happy. 😄
If we can ignore that, then I can change back.
--
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 #1326: Implement DictionaryArray support in neq_dyn, lt_dyn, lt_eq_dyn, gt_dyn, gt_eq_dyn
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1326:
URL: https://github.com/apache/arrow-rs/pull/1326#discussion_r809354161
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2318,7 +2318,7 @@ where
pub fn eq_dyn(left: &dyn Array, right: &dyn Array) -> Result<BooleanArray> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_compares!(left, right, |a, b| a == b)
+ typed_dict_compares!(left, right, |a, b| a == b, |a, b| !(a ^ b))
Review comment:
I think we can ignore it. I think clippy is somewhat confused probably when the parameters are boolea
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2318,7 +2318,7 @@ where
pub fn eq_dyn(left: &dyn Array, right: &dyn Array) -> Result<BooleanArray> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_compares!(left, right, |a, b| a == b)
+ typed_dict_compares!(left, right, |a, b| a == b, |a, b| !(a ^ b))
Review comment:
I think we can ignore it. I think clippy is somewhat confused probably when the parameters are boolean
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a change in pull request #1326: Implement DictionaryArray support in neq_dyn, lt_dyn, lt_eq_dyn, gt_dyn, gt_eq_dyn
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1326:
URL: https://github.com/apache/arrow-rs/pull/1326#discussion_r808721121
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -4790,5 +4848,12 @@ mod tests {
result.unwrap(),
BooleanArray::from(vec![false, true, false])
);
+
+ let result = neq_dyn(&dict_array1, &dict_array2);
+ assert!(result.is_ok());
+ assert_eq!(
+ result.unwrap(),
+ BooleanArray::from(vec![true, false, true])
+ );
Review comment:
I will add test for `lt_dyn`, `lt_eq_dyn`, `gt_dyn`, `gt_eq_dyn` later.
--
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 #1326: Implement DictionaryArray support in neq_dyn, lt_dyn, lt_eq_dyn, gt_dyn, gt_eq_dyn
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1326:
URL: https://github.com/apache/arrow-rs/pull/1326#issuecomment-1054677800
Hi @viirya -- I hope you don't mind but i merged this PR from master and added https://github.com/apache/arrow-rs/pull/1326/commits/219c131bcc7998807ec3e668563e8658747d8117 to silence clippy -- it was claiming
```
error: order comparisons between booleans can be simplified
--> arrow/src/compute/kernels/comparison.rs:2370:68
|
2370 | typed_dict_compares!(left, right, |a, b| a < b, |a, b| a < b)
| ^^^^^ help: try simplifying it as shown: `!a & b`
|
= note: `-D clippy::bool-comparison` implied by `-D warnings`
= help: for further information visit [https://rust-lang.gi](https://rust-lang.github.io/rust-clippy/master/index.html#bool_comparison)
```
Which is nonsense in my opinion (!a & b) is much less readable than `a < b` and I would expect the code generator to do that transformation anyways if it helps performance.
--
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 #1326: Implement DictionaryArray support in neq_dyn, lt_dyn, lt_eq_dyn, gt_dyn, gt_eq_dyn
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1326:
URL: https://github.com/apache/arrow-rs/pull/1326#issuecomment-1042635166
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1326?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 [#1326](https://codecov.io/gh/apache/arrow-rs/pull/1326?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0a33401) into [master](https://codecov.io/gh/apache/arrow-rs/commit/827cc3e9e153d6cf4e6a4f8a71c760043a9f25a8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (827cc3e) will **increase** coverage by `0.00%`.
> The diff coverage is `92.15%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1326?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 #1326 +/- ##
=======================================
Coverage 83.00% 83.00%
=======================================
Files 180 180
Lines 52919 52958 +39
=======================================
+ Hits 43924 43960 +36
- Misses 8995 8998 +3
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1326?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/1326/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) | `92.12% <92.15%> (-0.06%)` | :arrow_down: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1326/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/1326/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.65% <0.00%> (+0.26%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1326?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/1326?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 [827cc3e...0a33401](https://codecov.io/gh/apache/arrow-rs/pull/1326?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 #1326: Implement DictionaryArray support in neq_dyn, lt_dyn, lt_eq_dyn, gt_dyn, gt_eq_dyn
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1326:
URL: https://github.com/apache/arrow-rs/pull/1326#issuecomment-1042635166
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1326?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 [#1326](https://codecov.io/gh/apache/arrow-rs/pull/1326?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1060821) into [master](https://codecov.io/gh/apache/arrow-rs/commit/827cc3e9e153d6cf4e6a4f8a71c760043a9f25a8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (827cc3e) will **increase** coverage by `0.02%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head 1060821 differs from pull request most recent head 1bf0384. Consider uploading reports for the commit 1bf0384 to get more accurate results
[](https://codecov.io/gh/apache/arrow-rs/pull/1326?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 #1326 +/- ##
==========================================
+ Coverage 83.00% 83.03% +0.02%
==========================================
Files 180 180
Lines 52919 53002 +83
==========================================
+ Hits 43924 44008 +84
+ Misses 8995 8994 -1
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1326?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/1326/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) | `92.57% <100.00%> (+0.39%)` | :arrow_up: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1326/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldF9kZXJpdmUvc3JjL3BhcnF1ZXRfZmllbGQucnM=) | `66.21% <0.00%> (ø)` | |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1326/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.52% <0.00%> (+0.13%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1326?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/1326?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 [827cc3e...1bf0384](https://codecov.io/gh/apache/arrow-rs/pull/1326?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 #1326: Implement DictionaryArray support in neq_dyn, lt_dyn, lt_eq_dyn, gt_dyn, gt_eq_dyn
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1326:
URL: https://github.com/apache/arrow-rs/pull/1326#discussion_r809061485
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2032,10 +2032,10 @@ macro_rules! typed_compares {
/// Applies $OP to $LEFT and $RIGHT which are two dictionaries which have (the same) key type $KT
macro_rules! typed_dict_cmp {
- ($LEFT: expr, $RIGHT: expr, $OP: expr, $KT: tt) => {{
+ ($LEFT: expr, $RIGHT: expr, $OP: expr, $OP_BOOL: expr, $KT: tt) => {{
Review comment:
👍 nice readability improvement
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2360,7 +2365,12 @@ pub fn neq_dyn(left: &dyn Array, right: &dyn Array) -> Result<BooleanArray> {
/// assert_eq!(BooleanArray::from(vec![Some(true), Some(false), None]), result);
/// ```
pub fn lt_dyn(left: &dyn Array, right: &dyn Array) -> Result<BooleanArray> {
- typed_compares!(left, right, lt_bool, lt, lt_utf8, lt_binary)
+ match left.data_type() {
+ DataType::Dictionary(_, _) => {
+ typed_dict_compares!(left, right, |a, b| a < b, |a, b| (!a) & b)
Review comment:
```suggestion
typed_dict_compares!(left, right, |a, b| a < b, |a, b| a < b)
```
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2318,7 +2318,7 @@ where
pub fn eq_dyn(left: &dyn Array, right: &dyn Array) -> Result<BooleanArray> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_compares!(left, right, |a, b| a == b)
+ typed_dict_compares!(left, right, |a, b| a == b, |a, b| !(a ^ b))
Review comment:
I don't understand this change -- I think the `a == b` is easier to understand and I would expect that llvm would create optimized code for whatever was being compared.
If this is clippy being silly about comparing booleans perhaps we can just ignore the lint
```suggestion
typed_dict_compares!(left, right, |a, b| a == b, |a, b| a == b)
```
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2415,7 +2435,12 @@ pub fn gt_dyn(left: &dyn Array, right: &dyn Array) -> Result<BooleanArray> {
/// assert_eq!(BooleanArray::from(vec![Some(false), Some(true), None]), result);
/// ```
pub fn gt_eq_dyn(left: &dyn Array, right: &dyn Array) -> Result<BooleanArray> {
- typed_compares!(left, right, gt_eq_bool, gt_eq, gt_eq_utf8, gt_eq_binary)
+ match left.data_type() {
+ DataType::Dictionary(_, _) => {
+ typed_dict_compares!(left, right, |a, b| a >= b, |a, b| !((!a) & b))
Review comment:
```suggestion
typed_dict_compares!(left, right, |a, b| a >= b, |a, b| a >= b)
```
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2379,7 +2389,12 @@ pub fn lt_dyn(left: &dyn Array, right: &dyn Array) -> Result<BooleanArray> {
/// assert_eq!(BooleanArray::from(vec![Some(false), Some(true), Some(true), None]), result);
/// ```
pub fn lt_eq_dyn(left: &dyn Array, right: &dyn Array) -> Result<BooleanArray> {
- typed_compares!(left, right, lt_eq_bool, lt_eq, lt_eq_utf8, lt_eq_binary)
+ match left.data_type() {
+ DataType::Dictionary(_, _) => {
+ typed_dict_compares!(left, right, |a, b| a <= b, |a, b| !(a & (!b)))
Review comment:
```suggestion
typed_dict_compares!(left, right, |a, b| a <= b, |a, b| a <= b)
```
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2341,7 +2341,12 @@ pub fn eq_dyn(left: &dyn Array, right: &dyn Array) -> Result<BooleanArray> {
/// assert_eq!(BooleanArray::from(vec![Some(false), None, Some(true)]), result);
/// ```
pub fn neq_dyn(left: &dyn Array, right: &dyn Array) -> Result<BooleanArray> {
- typed_compares!(left, right, neq_bool, neq, neq_utf8, neq_binary)
+ match left.data_type() {
+ DataType::Dictionary(_, _) => {
+ typed_dict_compares!(left, right, |a, b| a != b, |a, b| (a ^ b))
Review comment:
```suggestion
typed_dict_compares!(left, right, |a, b| a != b, |a, b| a != b)
```
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -4790,5 +4851,76 @@ mod tests {
result.unwrap(),
BooleanArray::from(vec![false, true, false])
);
+
+ let result = neq_dyn(&dict_array1, &dict_array2);
+ assert!(result.is_ok());
Review comment:
As a style thing, I think it is ok to just `.unwrap()` the result -- if there is a problem it will panic one line later, but I think the source of the problem would still be quite clear
```suggestion
```
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2397,7 +2412,12 @@ pub fn lt_eq_dyn(left: &dyn Array, right: &dyn Array) -> Result<BooleanArray> {
/// assert_eq!(BooleanArray::from(vec![Some(true), Some(false), None]), result);
/// ```
pub fn gt_dyn(left: &dyn Array, right: &dyn Array) -> Result<BooleanArray> {
- typed_compares!(left, right, gt_bool, gt, gt_utf8, gt_binary)
+ match left.data_type() {
+ DataType::Dictionary(_, _) => {
+ typed_dict_compares!(left, right, |a, b| a > b, |a, b| a & (!b))
Review comment:
```suggestion
typed_dict_compares!(left, right, |a, b| a > b, |a, b| a > b)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on pull request #1326: Implement DictionaryArray support in neq_dyn, lt_dyn, lt_eq_dyn, gt_dyn, gt_eq_dyn
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1326:
URL: https://github.com/apache/arrow-rs/pull/1326#issuecomment-1043199710
Oh, I used `a == b` at first, but when I looked at `eq_bool`, it uses `!(a ^ b)`, so I modified to follow it. I can change it back to `a == b` if it looks better.
--
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 #1326: Implement DictionaryArray support in neq_dyn, lt_dyn, lt_eq_dyn, gt_dyn, gt_eq_dyn
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1326:
URL: https://github.com/apache/arrow-rs/pull/1326#issuecomment-1042635166
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1326?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 [#1326](https://codecov.io/gh/apache/arrow-rs/pull/1326?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (635e90d) into [master](https://codecov.io/gh/apache/arrow-rs/commit/827cc3e9e153d6cf4e6a4f8a71c760043a9f25a8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (827cc3e) will **increase** coverage by `0.04%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1326?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 #1326 +/- ##
==========================================
+ Coverage 83.00% 83.04% +0.04%
==========================================
Files 180 180
Lines 52919 52980 +61
==========================================
+ Hits 43924 43998 +74
+ Misses 8995 8982 -13
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1326?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/1326/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) | `92.47% <100.00%> (+0.29%)` | :arrow_up: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/1326/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: |
| [arrow/src/ipc/writer.rs](https://codecov.io/gh/apache/arrow-rs/pull/1326/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-YXJyb3cvc3JjL2lwYy93cml0ZXIucnM=) | `83.45% <0.00%> (-0.04%)` | :arrow_down: |
| [arrow/src/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/1326/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-YXJyb3cvc3JjL2ZmaS5ycw==) | `84.53% <0.00%> (ø)` | |
| [arrow/src/array/data.rs](https://codecov.io/gh/apache/arrow-rs/pull/1326/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-YXJyb3cvc3JjL2FycmF5L2RhdGEucnM=) | `83.30% <0.00%> (ø)` | |
| [arrow/src/csv/reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/1326/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-YXJyb3cvc3JjL2Nzdi9yZWFkZXIucnM=) | `88.12% <0.00%> (ø)` | |
| [arrow/src/csv/writer.rs](https://codecov.io/gh/apache/arrow-rs/pull/1326/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-YXJyb3cvc3JjL2Nzdi93cml0ZXIucnM=) | `72.13% <0.00%> (ø)` | |
| [arrow/src/compute/util.rs](https://codecov.io/gh/apache/arrow-rs/pull/1326/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-YXJyb3cvc3JjL2NvbXB1dGUvdXRpbC5ycw==) | `98.90% <0.00%> (ø)` | |
| [arrow/src/array/builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/1326/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-YXJyb3cvc3JjL2FycmF5L2J1aWxkZXIucnM=) | `86.73% <0.00%> (ø)` | |
| [arrow/src/array/array\_primitive.rs](https://codecov.io/gh/apache/arrow-rs/pull/1326/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2FycmF5X3ByaW1pdGl2ZS5ycw==) | `94.69% <0.00%> (ø)` | |
| ... and [3 more](https://codecov.io/gh/apache/arrow-rs/pull/1326/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1326?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/1326?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 [827cc3e...635e90d](https://codecov.io/gh/apache/arrow-rs/pull/1326?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on pull request #1326: Implement DictionaryArray support in neq_dyn, lt_dyn, lt_eq_dyn, gt_dyn, gt_eq_dyn
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1326:
URL: https://github.com/apache/arrow-rs/pull/1326#issuecomment-1042755943
cc @alamb
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on pull request #1326: Implement DictionaryArray support in neq_dyn, lt_dyn, lt_eq_dyn, gt_dyn, gt_eq_dyn
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1326:
URL: https://github.com/apache/arrow-rs/pull/1326#issuecomment-1043230600
Thanks @alamb ! Changed the bool ops back and removed `is_ok` check.
--
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 #1326: Implement DictionaryArray support in neq_dyn, lt_dyn, lt_eq_dyn, gt_dyn, gt_eq_dyn
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1326:
URL: https://github.com/apache/arrow-rs/pull/1326
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on pull request #1326: Implement DictionaryArray support in neq_dyn, lt_dyn, lt_eq_dyn, gt_dyn, gt_eq_dyn
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1326:
URL: https://github.com/apache/arrow-rs/pull/1326#issuecomment-1054887278
Yea, no problem at all! Thanks @alamb !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org