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 2020/12/16 22:11:11 UTC

[GitHub] [arrow] alamb opened a new pull request #8946: ARROW-10944: [Rust] Implement min/max aggregate kernels for BooleanArray

alamb opened a new pull request #8946:
URL: https://github.com/apache/arrow/pull/8946


   Rationale: While Min/Max is a somewhat silly / meaningless operation,  typically database systems support it for completeness. 
   
   This PR adds a basic implementation and tests. 
   
   More detail on usecase is [here](https://github.com/influxdata/influxdb_iox/issues/568)
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] alamb closed pull request #8946: ARROW-10944: [Rust] Implement min/max aggregate kernels for BooleanArray

Posted by GitBox <gi...@apache.org>.
alamb closed pull request #8946:
URL: https://github.com/apache/arrow/pull/8946


   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] codecov-io edited a comment on pull request #8946: ARROW-10944: [Rust] Implement min/max aggregate kernels for BooleanArray

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #8946:
URL: https://github.com/apache/arrow/pull/8946#issuecomment-747076330


   # [Codecov](https://codecov.io/gh/apache/arrow/pull/8946?src=pr&el=h1) Report
   > Merging [#8946](https://codecov.io/gh/apache/arrow/pull/8946?src=pr&el=desc) (d24c6f1) into [master](https://codecov.io/gh/apache/arrow/commit/3e89a9c6327b5267cdde00cc7b6bd4bfe677e6b6?el=desc) (3e89a9c) will **increase** coverage by `0.01%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow/pull/8946/graphs/tree.svg?width=650&height=150&src=pr&token=LpTCFbqVT1)](https://codecov.io/gh/apache/arrow/pull/8946?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #8946      +/-   ##
   ==========================================
   + Coverage   83.17%   83.18%   +0.01%     
   ==========================================
     Files         197      197              
     Lines       48526    48573      +47     
   ==========================================
   + Hits        40360    40407      +47     
     Misses       8166     8166              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow/pull/8946?src=pr&el=tree) | Coverage Ξ” | |
   |---|---|---|
   | [rust/arrow/src/compute/kernels/aggregate.rs](https://codecov.io/gh/apache/arrow/pull/8946/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvY29tcHV0ZS9rZXJuZWxzL2FnZ3JlZ2F0ZS5ycw==) | `75.00% <100.00%> (+3.57%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/8946?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Ξ” = absolute <relative> (impact)`, `ΓΈ = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/8946?src=pr&el=footer). Last update [3e89a9c...d24c6f1](https://codecov.io/gh/apache/arrow/pull/8946?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] nevi-me commented on pull request #8946: ARROW-10944: [Rust] Implement min/max aggregate kernels for BooleanArray

Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #8946:
URL: https://github.com/apache/arrow/pull/8946#issuecomment-748266025


   > This PR introduces only private methods and are never used internally. Isn't this an issue?
   
   Great catch! They should be public


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] alamb commented on a change in pull request #8946: ARROW-10944: [Rust] Implement min/max aggregate kernels for BooleanArray

Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #8946:
URL: https://github.com/apache/arrow/pull/8946#discussion_r545051950



##########
File path: rust/arrow/src/compute/kernels/aggregate.rs
##########
@@ -135,6 +137,40 @@ where
     Some(n)
 }
 
+/// Helper function to perform min/max lambda function on values from a `BooleanArray`
+fn min_max_boolean<F>(array: &BooleanArray, cmp: F) -> Option<bool>
+where
+    F: Fn(bool, bool) -> bool,
+{
+    let null_count = array.null_count();
+
+    // Includes case array.len() == 0
+    if null_count == array.len() {
+        return None;
+    }
+
+    let m0: Option<bool> = array.iter().next().unwrap();
+
+    array.iter().fold(m0, |max, item| match (max, item) {

Review comment:
       Addressed in https://github.com/apache/arrow/pull/8946/commits/075277c7ae64c8d1aacf7a07c7cbf501c945570b -- I am actually quite happy with how the code looks now given this suggestion. πŸ‘ 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] alamb commented on pull request #8946: ARROW-10944: [Rust] Implement min/max aggregate kernels for BooleanArray

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #8946:
URL: https://github.com/apache/arrow/pull/8946#issuecomment-748362790


   🀦  yes that is an excellent catch @jorgecarleitao  -- I plan to polish this up tomorrow. I apologize for all the noise. Thank you all for the through reviews @nevi-me and @Dandandan 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] codecov-io commented on pull request #8946: ARROW-10944: [Rust] Implement min/max aggregate kernels for BooleanArray

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #8946:
URL: https://github.com/apache/arrow/pull/8946#issuecomment-747076330


   # [Codecov](https://codecov.io/gh/apache/arrow/pull/8946?src=pr&el=h1) Report
   > Merging [#8946](https://codecov.io/gh/apache/arrow/pull/8946?src=pr&el=desc) (4197b1d) into [master](https://codecov.io/gh/apache/arrow/commit/d65ba4ec5daeb93ca5031f883d08d559b68753b2?el=desc) (d65ba4e) will **increase** coverage by `0.01%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow/pull/8946/graphs/tree.svg?width=650&height=150&src=pr&token=LpTCFbqVT1)](https://codecov.io/gh/apache/arrow/pull/8946?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #8946      +/-   ##
   ==========================================
   + Coverage   83.25%   83.26%   +0.01%     
   ==========================================
     Files         196      196              
     Lines       48116    48152      +36     
   ==========================================
   + Hits        40059    40095      +36     
     Misses       8057     8057              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow/pull/8946?src=pr&el=tree) | Coverage Ξ” | |
   |---|---|---|
   | [rust/arrow/src/compute/kernels/aggregate.rs](https://codecov.io/gh/apache/arrow/pull/8946/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvY29tcHV0ZS9rZXJuZWxzL2FnZ3JlZ2F0ZS5ycw==) | `74.24% <100.00%> (+2.81%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/8946?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Ξ” = absolute <relative> (impact)`, `ΓΈ = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/8946?src=pr&el=footer). Last update [d65ba4e...4197b1d](https://codecov.io/gh/apache/arrow/pull/8946?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] jorgecarleitao commented on pull request #8946: ARROW-10944: [Rust] Implement min/max aggregate kernels for BooleanArray

Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #8946:
URL: https://github.com/apache/arrow/pull/8946#issuecomment-748132321


   This PR introduces only private methods and are never used internally. Isn't this an 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] Dandandan commented on pull request #8946: ARROW-10944: [Rust] Implement min/max aggregate kernels for BooleanArray

Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #8946:
URL: https://github.com/apache/arrow/pull/8946#issuecomment-747597413


   I think correct would be
   
   `array.iter().find(|&b| b == Some(false)).flatten().or(Some(true))` and the other way around?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] Dandandan commented on pull request #8946: ARROW-10944: [Rust] Implement min/max aggregate kernels for BooleanArray

Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #8946:
URL: https://github.com/apache/arrow/pull/8946#issuecomment-747559686


   On closer inspection, is it actually correc now. What would max(null, false) 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] alamb commented on pull request #8946: ARROW-10944: [Rust] Implement min/max aggregate kernels for BooleanArray

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #8946:
URL: https://github.com/apache/arrow/pull/8946#issuecomment-747772748


   > What would the output of max(null, false) be in the current implementation?
   
   A: Incorrect (the code in https://github.com/apache/arrow/pull/8946/commits/075277c7ae64c8d1aacf7a07c7cbf501c945570b produces `None` when the answer should be `false`).
   
   > I think correct would be
   πŸ‘ 
   
   I implemented this suggestion, as well as additional tests in https://github.com/apache/arrow/pull/8946/commits/b5a14d9d857cdec41f45008f8d1b69fadaece045. Thank you @Dandandan 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] github-actions[bot] commented on pull request #8946: ARROW-10944: [Rust] Implement min/max aggregate kernels for BooleanArray

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8946:
URL: https://github.com/apache/arrow/pull/8946#issuecomment-747081556


   https://issues.apache.org/jira/browse/ARROW-10944


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] Dandandan edited a comment on pull request #8946: ARROW-10944: [Rust] Implement min/max aggregate kernels for BooleanArray

Posted by GitBox <gi...@apache.org>.
Dandandan edited a comment on pull request #8946:
URL: https://github.com/apache/arrow/pull/8946#issuecomment-747559686


   On closer inspection, is it actually correct now? What would max(null, false) 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] Dandandan edited a comment on pull request #8946: ARROW-10944: [Rust] Implement min/max aggregate kernels for BooleanArray

Posted by GitBox <gi...@apache.org>.
Dandandan edited a comment on pull request #8946:
URL: https://github.com/apache/arrow/pull/8946#issuecomment-747559686


   On closer inspection, is it actually correct now? What would max(null, false) be in the current implementation?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] Dandandan edited a comment on pull request #8946: ARROW-10944: [Rust] Implement min/max aggregate kernels for BooleanArray

Posted by GitBox <gi...@apache.org>.
Dandandan edited a comment on pull request #8946:
URL: https://github.com/apache/arrow/pull/8946#issuecomment-747559686


   On closer inspection, is it actually correct now? What would the output of max(null, false) be in the current implementation?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] alamb commented on a change in pull request #8946: ARROW-10944: [Rust] Implement min/max aggregate kernels for BooleanArray

Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #8946:
URL: https://github.com/apache/arrow/pull/8946#discussion_r545051950



##########
File path: rust/arrow/src/compute/kernels/aggregate.rs
##########
@@ -135,6 +137,40 @@ where
     Some(n)
 }
 
+/// Helper function to perform min/max lambda function on values from a `BooleanArray`
+fn min_max_boolean<F>(array: &BooleanArray, cmp: F) -> Option<bool>
+where
+    F: Fn(bool, bool) -> bool,
+{
+    let null_count = array.null_count();
+
+    // Includes case array.len() == 0
+    if null_count == array.len() {
+        return None;
+    }
+
+    let m0: Option<bool> = array.iter().next().unwrap();
+
+    array.iter().fold(m0, |max, item| match (max, item) {

Review comment:
       Addressed in https://github.com/apache/arrow/pull/8946/commits/075277c7ae64c8d1aacf7a07c7cbf501c945570b -- I am actually quite happy with how the code looks now given this suggestion. πŸ‘ 
   
   Thank you both.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] Dandandan commented on a change in pull request #8946: ARROW-10944: [Rust] Implement min/max aggregate kernels for BooleanArray

Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #8946:
URL: https://github.com/apache/arrow/pull/8946#discussion_r544728199



##########
File path: rust/arrow/src/compute/kernels/aggregate.rs
##########
@@ -135,6 +137,40 @@ where
     Some(n)
 }
 
+/// Helper function to perform min/max lambda function on values from a `BooleanArray`
+fn min_max_boolean<F>(array: &BooleanArray, cmp: F) -> Option<bool>
+where
+    F: Fn(bool, bool) -> bool,
+{
+    let null_count = array.null_count();
+
+    // Includes case array.len() == 0
+    if null_count == array.len() {
+        return None;
+    }
+
+    let m0: Option<bool> = array.iter().next().unwrap();
+
+    array.iter().fold(m0, |max, item| match (max, item) {

Review comment:
       Would it make sense to return early here when seeing the highest or lowest value? 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] jorgecarleitao commented on a change in pull request #8946: ARROW-10944: [Rust] Implement min/max aggregate kernels for BooleanArray

Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #8946:
URL: https://github.com/apache/arrow/pull/8946#discussion_r544830660



##########
File path: rust/arrow/src/compute/kernels/aggregate.rs
##########
@@ -135,6 +137,40 @@ where
     Some(n)
 }
 
+/// Helper function to perform min/max lambda function on values from a `BooleanArray`
+fn min_max_boolean<F>(array: &BooleanArray, cmp: F) -> Option<bool>
+where
+    F: Fn(bool, bool) -> bool,
+{
+    let null_count = array.null_count();
+
+    // Includes case array.len() == 0
+    if null_count == array.len() {
+        return None;
+    }
+
+    let m0: Option<bool> = array.iter().next().unwrap();
+
+    array.iter().fold(m0, |max, item| match (max, item) {

Review comment:
       Good point. This is indeed simpler given the finite number of choices. Something like `.any(item)` would short-circuit, I think.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org