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/13 08:25:31 UTC

[GitHub] [arrow] jorgecarleitao opened a new pull request #8903: ARROW-9771: [Rust] [DataFusion] treat predicates separated by AND separately in predicate pushdown

jorgecarleitao opened a new pull request #8903:
URL: https://github.com/apache/arrow/pull/8903


   


----------------------------------------------------------------
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 #8903: ARROW-9771: [Rust] [DataFusion] treat predicates separated by AND separately in predicate pushdown

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



##########
File path: rust/datafusion/src/optimizer/filter_push_down.rs
##########
@@ -215,13 +215,38 @@ fn issue_filters(
     push_down(&state, &plan)
 }
 
+/// converts "A AND B AND C" => [A, B, C]
+fn split_members(predicate: &Expr) -> Vec<&Expr> {
+    match predicate {
+        Expr::BinaryExpr {
+            right,
+            op: Operator::And,
+            left,
+        } => {
+            let mut a = split_members(&left);
+            a.extend(split_members(&right));

Review comment:
       I think this could be O(n*n) in complexity as expression trees are often nested like `1+(1+(1+(...` and extend just iterates over the right side.
   Probably requires a pretty big expression to become a problem, but still maybe something to think about?




----------------------------------------------------------------
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 #8903: ARROW-9771: [Rust] [DataFusion] treat predicates separated by AND separately in predicate pushdown

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


   


----------------------------------------------------------------
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 #8903: ARROW-9771: [Rust] [DataFusion] treat predicates separated by AND separately in predicate pushdown

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



##########
File path: rust/datafusion/src/optimizer/filter_push_down.rs
##########
@@ -215,13 +215,38 @@ fn issue_filters(
     push_down(&state, &plan)
 }
 
+/// converts "A AND B AND C" => [A, B, C]
+fn split_members(predicate: &Expr) -> Vec<&Expr> {

Review comment:
       Eventually (as part of some other PR) this code might more naturally belong in the utils.rs module -- splitting expressions into an AND list is a common thing to want to do, I think. It would make the code easier to find if people need it for other optimizations. I feel like I have seen this code before in DataFusion but I can't find it now...




----------------------------------------------------------------
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 #8903: ARROW-9771: [Rust] [DataFusion] treat predicates separated by AND separately in predicate pushdown

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



##########
File path: rust/datafusion/src/optimizer/filter_push_down.rs
##########
@@ -215,13 +215,38 @@ fn issue_filters(
     push_down(&state, &plan)
 }
 
+/// converts "A AND B AND C" => [A, B, C]
+fn split_members(predicate: &Expr) -> Vec<&Expr> {
+    match predicate {
+        Expr::BinaryExpr {
+            right,
+            op: Operator::And,
+            left,
+        } => {
+            let mut a = split_members(&left);
+            a.extend(split_members(&right));

Review comment:
       Thanks @Dandandan . This is likely a small performance issue given that it does not affect execution, but I addressed it anyways.




----------------------------------------------------------------
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 #8903: ARROW-9771: [Rust] [DataFusion] treat predicates separated by AND separately in predicate pushdown

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



##########
File path: rust/datafusion/src/optimizer/filter_push_down.rs
##########
@@ -215,13 +215,38 @@ fn issue_filters(
     push_down(&state, &plan)
 }
 
+/// converts "A AND B AND C" => [A, B, C]
+fn split_members(predicate: &Expr) -> Vec<&Expr> {
+    match predicate {
+        Expr::BinaryExpr {
+            right,
+            op: Operator::And,
+            left,
+        } => {
+            let mut a = split_members(&left);
+            a.extend(split_members(&right));

Review comment:
       I think this could be O(n*n) in complexity as expression trees are often nested like `x and (y and (z and(...` and extend just iterates over the right side.
   Probably requires a pretty big expression to become a problem, but still maybe something to think about?




----------------------------------------------------------------
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 #8903: ARROW-9771: [Rust] [DataFusion] treat predicates separated by AND separately in predicate pushdown

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



##########
File path: rust/datafusion/src/optimizer/filter_push_down.rs
##########
@@ -215,13 +215,38 @@ fn issue_filters(
     push_down(&state, &plan)
 }
 
+/// converts "A AND B AND C" => [A, B, C]
+fn split_members(predicate: &Expr) -> Vec<&Expr> {
+    match predicate {
+        Expr::BinaryExpr {
+            right,
+            op: Operator::And,
+            left,
+        } => {
+            let mut a = split_members(&left);
+            a.extend(split_members(&right));

Review comment:
       Simple solution would be passing /inserting to a mutable vec 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.

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

[GitHub] [arrow] github-actions[bot] commented on pull request #8903: ARROW-9771: [Rust] [DataFusion] treat predicates separated by AND separately in predicate pushdown

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


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


----------------------------------------------------------------
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 #8903: ARROW-9771: [Rust] [DataFusion] treat predicates separated by AND separately in predicate pushdown

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


   # [Codecov](https://codecov.io/gh/apache/arrow/pull/8903?src=pr&el=h1) Report
   > Merging [#8903](https://codecov.io/gh/apache/arrow/pull/8903?src=pr&el=desc) (9e6ac5f) into [master](https://codecov.io/gh/apache/arrow/commit/0c8b9903602e1cde0a20b825abf92d361af3c315?el=desc) (0c8b990) will **not change** coverage.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow/pull/8903/graphs/tree.svg?width=650&height=150&src=pr&token=LpTCFbqVT1)](https://codecov.io/gh/apache/arrow/pull/8903?src=pr&el=tree)
   
   ```diff
   @@           Coverage Diff           @@
   ##           master    #8903   +/-   ##
   =======================================
     Coverage   76.77%   76.77%           
   =======================================
     Files         181      181           
     Lines       41009    41009           
   =======================================
     Hits        31485    31485           
     Misses       9524     9524           
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow/pull/8903?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [rust/datafusion/src/optimizer/filter\_push\_down.rs](https://codecov.io/gh/apache/arrow/pull/8903/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9vcHRpbWl6ZXIvZmlsdGVyX3B1c2hfZG93bi5ycw==) | `0.00% <ø> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/8903?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/8903?src=pr&el=footer). Last update [0c8b990...9e6ac5f](https://codecov.io/gh/apache/arrow/pull/8903?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