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/07/02 10:37:53 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #2828: Improve ergonomics of physical expr `lit`
alamb opened a new pull request, #2828:
URL: https://github.com/apache/arrow-datafusion/pull/2828
# Which issue does this PR close?
Closes https://github.com/apache/arrow-datafusion/issues/2827
# Rationale for this change
See https://github.com/apache/arrow-datafusion/issues/2827
# What changes are included in this PR?
Make `lit` in physical expr easier to use
# Are there any user-facing changes?
Yes, but they are backwards compatible
--
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-datafusion] andygrove commented on a diff in pull request #2828: Improve ergonomics of physical expr `lit`
Posted by GitBox <gi...@apache.org>.
andygrove commented on code in PR #2828:
URL: https://github.com/apache/arrow-datafusion/pull/2828#discussion_r912398460
##########
datafusion/physical-expr/src/expressions/literal.rs:
##########
@@ -74,8 +74,13 @@ impl PhysicalExpr for Literal {
}
/// Create a literal expression
-pub fn lit(value: ScalarValue) -> Arc<dyn PhysicalExpr> {
- Arc::new(Literal::new(value))
+pub fn lit<T: datafusion_expr::Literal>(value: T) -> Arc<dyn PhysicalExpr> {
Review Comment:
This could also be written slightly more concisely as a match:
```
let scalar_value = match value.lit() {
Expr::Literal(v) => v,
_ => unreachable!()
};
```
--
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-datafusion] alamb commented on a diff in pull request #2828: Improve ergonomics of physical expr `lit`
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2828:
URL: https://github.com/apache/arrow-datafusion/pull/2828#discussion_r914005761
##########
datafusion/physical-expr/src/expressions/literal.rs:
##########
@@ -74,8 +74,13 @@ impl PhysicalExpr for Literal {
}
/// Create a literal expression
-pub fn lit(value: ScalarValue) -> Arc<dyn PhysicalExpr> {
- Arc::new(Literal::new(value))
+pub fn lit<T: datafusion_expr::Literal>(value: T) -> Arc<dyn PhysicalExpr> {
Review Comment:
I implemented this suggestion in https://github.com/apache/arrow-datafusion/pull/2838
--
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-datafusion] alamb commented on a diff in pull request #2828: Improve ergonomics of physical expr `lit`
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2828:
URL: https://github.com/apache/arrow-datafusion/pull/2828#discussion_r912349280
##########
datafusion/core/src/physical_optimizer/aggregate_statistics.rs:
##########
@@ -525,7 +525,7 @@ mod tests {
expressions::binary(
expressions::col("a", &schema)?,
Operator::Gt,
- expressions::lit(ScalarValue::from(1u32)),
+ expressions::lit(1u32),
Review Comment:
This change shows the point of this PR pretty well
##########
datafusion/physical-expr/src/expressions/literal.rs:
##########
@@ -74,8 +74,13 @@ impl PhysicalExpr for Literal {
}
/// Create a literal expression
-pub fn lit(value: ScalarValue) -> Arc<dyn PhysicalExpr> {
- Arc::new(Literal::new(value))
+pub fn lit<T: datafusion_expr::Literal>(value: T) -> Arc<dyn PhysicalExpr> {
Review Comment:
Here is the actual change. It isn't the most beautiful code but I think it is reasonable
--
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-datafusion] codecov-commenter commented on pull request #2828: Improve ergonomics of physical expr `lit`
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #2828:
URL: https://github.com/apache/arrow-datafusion/pull/2828#issuecomment-1172884604
# [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/2828?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 [#2828](https://codecov.io/gh/apache/arrow-datafusion/pull/2828?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (353bbc1) into [master](https://codecov.io/gh/apache/arrow-datafusion/commit/88b88d4360054a85982987aa07b3f3afd2db7d70?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (88b88d4) will **decrease** coverage by `0.08%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #2828 +/- ##
==========================================
- Coverage 85.26% 85.17% -0.09%
==========================================
Files 275 275
Lines 48830 48564 -266
==========================================
- Hits 41633 41366 -267
- Misses 7197 7198 +1
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-datafusion/pull/2828?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ore/src/physical\_optimizer/aggregate\_statistics.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/2828/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-ZGF0YWZ1c2lvbi9jb3JlL3NyYy9waHlzaWNhbF9vcHRpbWl6ZXIvYWdncmVnYXRlX3N0YXRpc3RpY3MucnM=) | `100.00% <100.00%> (ø)` | |
| [...atafusion/core/src/physical\_plan/aggregates/mod.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/2828/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-ZGF0YWZ1c2lvbi9jb3JlL3NyYy9waHlzaWNhbF9wbGFuL2FnZ3JlZ2F0ZXMvbW9kLnJz) | `91.16% <100.00%> (ø)` | |
| [datafusion/core/src/physical\_plan/filter.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/2828/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-ZGF0YWZ1c2lvbi9jb3JlL3NyYy9waHlzaWNhbF9wbGFuL2ZpbHRlci5ycw==) | `87.64% <100.00%> (-1.36%)` | :arrow_down: |
| [datafusion/physical-expr/src/expressions/case.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/2828/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-ZGF0YWZ1c2lvbi9waHlzaWNhbC1leHByL3NyYy9leHByZXNzaW9ucy9jYXNlLnJz) | `92.00% <100.00%> (-0.09%)` | :arrow_down: |
| [...physical-expr/src/expressions/get\_indexed\_field.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/2828/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-ZGF0YWZ1c2lvbi9waHlzaWNhbC1leHByL3NyYy9leHByZXNzaW9ucy9nZXRfaW5kZXhlZF9maWVsZC5ycw==) | `92.93% <100.00%> (ø)` | |
| [...atafusion/physical-expr/src/expressions/in\_list.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/2828/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-ZGF0YWZ1c2lvbi9waHlzaWNhbC1leHByL3NyYy9leHByZXNzaW9ucy9pbl9saXN0LnJz) | `68.20% <100.00%> (-3.33%)` | :arrow_down: |
| [...atafusion/physical-expr/src/expressions/literal.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/2828/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-ZGF0YWZ1c2lvbi9waHlzaWNhbC1leHByL3NyYy9leHByZXNzaW9ucy9saXRlcmFsLnJz) | `100.00% <100.00%> (ø)` | |
| [datafusion/physical-expr/src/functions.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/2828/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-ZGF0YWZ1c2lvbi9waHlzaWNhbC1leHByL3NyYy9mdW5jdGlvbnMucnM=) | `92.88% <100.00%> (-1.32%)` | :arrow_down: |
| [datafusion/expr/src/window\_frame.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/2828/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-ZGF0YWZ1c2lvbi9leHByL3NyYy93aW5kb3dfZnJhbWUucnM=) | `92.43% <0.00%> (-0.85%)` | :arrow_down: |
| [datafusion/core/src/physical\_plan/metrics/value.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/2828/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-ZGF0YWZ1c2lvbi9jb3JlL3NyYy9waHlzaWNhbF9wbGFuL21ldHJpY3MvdmFsdWUucnM=) | `86.93% <0.00%> (-0.51%)` | :arrow_down: |
| ... and [1 more](https://codecov.io/gh/apache/arrow-datafusion/pull/2828/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-datafusion/pull/2828?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-datafusion/pull/2828?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 [88b88d4...353bbc1](https://codecov.io/gh/apache/arrow-datafusion/pull/2828?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-datafusion] liukun4515 merged pull request #2828: Improve ergonomics of physical expr `lit`
Posted by GitBox <gi...@apache.org>.
liukun4515 merged PR #2828:
URL: https://github.com/apache/arrow-datafusion/pull/2828
--
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