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/02/18 15:10:36 UTC
[GitHub] [arrow] alamb opened a new pull request #9527: ARROW-11690: [Rust][DataFusion] Avoid expr copies while using builder methods
alamb opened a new pull request #9527:
URL: https://github.com/apache/arrow/pull/9527
This is part of a larger body of work I would like to do to DataFusion to make it more efficient and idomatic Rust. See https://issues.apache.org/jira/browse/ARROW-11689 for more context.
The theme is to make the plan and expression rewriting phases of DataFusion more efficient by avoiding copies
This particular PR avoids deep cloning `Expr`s when building up new exprs. While this is technically a backwards compatible change, given there was only a single place in the datafusion codebase that needs to be updated, I think the impact was minimal.
----------------------------------------------------------------
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 #9527: ARROW-11690: [Rust][DataFusion] Avoid expr copies while using builder methods
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #9527:
URL: https://github.com/apache/arrow/pull/9527#issuecomment-781688802
# [Codecov](https://codecov.io/gh/apache/arrow/pull/9527?src=pr&el=h1) Report
> Merging [#9527](https://codecov.io/gh/apache/arrow/pull/9527?src=pr&el=desc) (5fa7da8) into [master](https://codecov.io/gh/apache/arrow/commit/bca7d2fe84ccd8fc1129cb4d85448eb0779c52c3?el=desc) (bca7d2f) will **increase** coverage by `0.03%`.
> The diff coverage is `91.01%`.
[](https://codecov.io/gh/apache/arrow/pull/9527?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9527 +/- ##
==========================================
+ Coverage 82.24% 82.27% +0.03%
==========================================
Files 244 244
Lines 55289 55371 +82
==========================================
+ Hits 45470 45556 +86
+ Misses 9819 9815 -4
```
| [Impacted Files](https://codecov.io/gh/apache/arrow/pull/9527?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [rust/arrow/src/compute/kernels/comparison.rs](https://codecov.io/gh/apache/arrow/pull/9527/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvY29tcHV0ZS9rZXJuZWxzL2NvbXBhcmlzb24ucnM=) | `95.84% <ø> (ø)` | |
| [rust/datafusion/src/logical\_plan/expr.rs](https://codecov.io/gh/apache/arrow/pull/9527/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vZXhwci5ycw==) | `80.75% <84.84%> (ø)` | |
| [rust/arrow/src/json/reader.rs](https://codecov.io/gh/apache/arrow/pull/9527/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvanNvbi9yZWFkZXIucnM=) | `83.63% <89.89%> (+0.81%)` | :arrow_up: |
| [rust/arrow/src/compute/kernels/cast.rs](https://codecov.io/gh/apache/arrow/pull/9527/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvY29tcHV0ZS9rZXJuZWxzL2Nhc3QucnM=) | `97.28% <100.00%> (+0.04%)` | :arrow_up: |
| [rust/datafusion/src/physical\_plan/limit.rs](https://codecov.io/gh/apache/arrow/pull/9527/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL2xpbWl0LnJz) | `57.47% <100.00%> (-0.49%)` | :arrow_down: |
| [rust/datafusion/src/physical\_plan/parquet.rs](https://codecov.io/gh/apache/arrow/pull/9527/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3BhcnF1ZXQucnM=) | `88.05% <100.00%> (ø)` | |
| [rust/datafusion/src/physical\_plan/planner.rs](https://codecov.io/gh/apache/arrow/pull/9527/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3BsYW5uZXIucnM=) | `79.58% <100.00%> (-0.05%)` | :arrow_down: |
| [rust/datafusion/src/physical\_plan/sort.rs](https://codecov.io/gh/apache/arrow/pull/9527/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3NvcnQucnM=) | `90.17% <100.00%> (+0.90%)` | :arrow_up: |
| [rust/parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow/pull/9527/diff?src=pr&el=tree#diff-cnVzdC9wYXJxdWV0L3NyYy9lbmNvZGluZ3MvZW5jb2RpbmcucnM=) | `95.05% <0.00%> (+0.19%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/9527?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/9527?src=pr&el=footer). Last update [a951c6c...5fa7da8](https://codecov.io/gh/apache/arrow/pull/9527?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] alamb commented on a change in pull request #9527: ARROW-11690: [Rust][DataFusion] Avoid expr copies while using builder methods
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9527:
URL: https://github.com/apache/arrow/pull/9527#discussion_r578500826
##########
File path: rust/datafusion/src/physical_plan/parquet.rs
##########
@@ -611,7 +611,7 @@ fn build_predicate_expression(
let max_column_expr = expr_builder.max_column_expr()?;
min_column_expr
.lt_eq(expr_builder.scalar_expr().clone())
- .and(expr_builder.scalar_expr().lt_eq(max_column_expr))
+ .and(expr_builder.scalar_expr().clone().lt_eq(max_column_expr))
Review comment:
this is the only change where the caller had to do an extra clone (and in this case it was actually needed as `scalar_expr()` is used twice. In other locations the expr was ignored and this can be consumed without 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] github-actions[bot] commented on pull request #9527: ARROW-11690: [Rust][DataFusion] Avoid expr copies while using builder methods
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9527:
URL: https://github.com/apache/arrow/pull/9527#issuecomment-781411083
https://issues.apache.org/jira/browse/ARROW-11690
----------------------------------------------------------------
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 #9527: ARROW-11690: [Rust][DataFusion] Avoid expr copies while using builder methods
Posted by GitBox <gi...@apache.org>.
alamb closed pull request #9527:
URL: https://github.com/apache/arrow/pull/9527
----------------------------------------------------------------
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] seddonm1 commented on pull request #9527: ARROW-11690: [Rust][DataFusion] Avoid expr copies while using builder methods
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #9527:
URL: https://github.com/apache/arrow/pull/9527#issuecomment-782817280
LGTM
----------------------------------------------------------------
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 #9527: ARROW-11690: [Rust][DataFusion] Avoid expr copies while using builder methods
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9527:
URL: https://github.com/apache/arrow/pull/9527#issuecomment-782620706
FYI @seddonm1 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] alamb commented on a change in pull request #9527: ARROW-11690: [Rust][DataFusion] Avoid expr copies while using builder methods
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9527:
URL: https://github.com/apache/arrow/pull/9527#discussion_r578499671
##########
File path: rust/datafusion/src/logical_plan/expr.rs
##########
@@ -335,75 +335,77 @@ impl Expr {
}
}
- /// Equal
- pub fn eq(&self, other: Expr) -> Expr {
- binary_expr(self.clone(), Operator::Eq, other)
+ /// Return `self == other`
+ pub fn eq(self, other: Expr) -> Expr {
Review comment:
The change is to have all these functions take `self` rather than `&self`.
Given they almost always are used like `let expr = lit("foo").eq(lit("bar"))` self is not reused.
Furthermore, the `clone()` is actually doing a deep clone, so in the above example `lit("foo")` gets copied twice.
----------------------------------------------------------------
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