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/05/22 17:57:33 UTC
[GitHub] [arrow-datafusion] msathis opened a new pull request #387: optimize to_timestamp
msathis opened a new pull request #387:
URL: https://github.com/apache/arrow-datafusion/pull/387
# Which issue does this PR close?
Fixes #383
# Rationale for this change
Optimize to_timestamp function
# What changes are included in this PR?
Optimize to_timestamp function
# Are there any user-facing changes?
N/A
--
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-datafusion] msathis commented on pull request #387: Constant fold / optimize `to_timestamp` function during planning
Posted by GitBox <gi...@apache.org>.
msathis commented on pull request #387:
URL: https://github.com/apache/arrow-datafusion/pull/387#issuecomment-847623401
> Oh no -- we have some clippy errors
>
> https://github.com/apache/arrow-datafusion/pull/387/checks?check_run_id=2658215529
>
> ```
>
> error: useless use of `format!`
> --> datafusion/src/optimizer/constant_folding.rs:680:24
> |
> 680 | let expected = format!(
> | ________________________^
> 681 | | "Projection: TimestampNanosecond(1599566400000000000)\
> 682 | | \n TableScan: test projection=None",
> 683 | | );
> | |_________^
> |
> = note: `-D clippy::useless-format` implied by `-D warnings`
> = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_format
> help: consider using `.to_string()`
> |
> 680 | let expected = "Projection: TimestampNanosecond(1599566400000000000)\
> 681 | \n TableScan: test projection=None".to_string();
> |
>
> error: useless use of `format!`
> --> datafusion/src/optimizer/constant_folding.rs:703:24
> |
> 703 | let expected = format!(
> | ________________________^
> 704 | | "Projection: totimestamp(Utf8(\"I\'M NOT A TIMESTAMP\"))\
> 705 | | \n TableScan: test projection=None",
> 706 | | );
> | |_________^
> |
> = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_format
> help: consider using `.to_string()`
> ```
For some strange reason `cargo clippy` on my local doesn't throw these errors.
--
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-datafusion] alamb commented on pull request #387: Constant fold / optimize `to_timestamp` function during planning
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #387:
URL: https://github.com/apache/arrow-datafusion/pull/387#issuecomment-848113787
Thanks again @msathis !
--
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-datafusion] alamb edited a comment on pull request #387: Constant fold / optimize `to_timestamp` function during planning
Posted by GitBox <gi...@apache.org>.
alamb edited a comment on pull request #387:
URL: https://github.com/apache/arrow-datafusion/pull/387#issuecomment-847988995
> For some strange reason cargo clippy on my local doesn't throw these errors.
It may be related to what version of rust is used. I also have some troubles with clippy sometimes not finding errors. I run the following locally (I am somewhat embarrassed but it does work for me 🤷 )
```
find . -name '*.rs' | grep -v target | xargs touch && cargo clippy --all --all-targets -- -D warnings
```
--
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-datafusion] alamb commented on a change in pull request #387: Optimize to_timestamp function
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #387:
URL: https://github.com/apache/arrow-datafusion/pull/387#discussion_r637523360
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -217,6 +218,29 @@ impl<'a> ExprRewriter for ConstantRewriter<'a> {
.query_execution_start_time
.timestamp_nanos(),
))),
+ Expr::ScalarFunction {
Review comment:
Matching on the literal argument is good as the `rewrite` pass happens *after* all children are visited 👍
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -217,6 +218,29 @@ impl<'a> ExprRewriter for ConstantRewriter<'a> {
.query_execution_start_time
.timestamp_nanos(),
))),
+ Expr::ScalarFunction {
+ fun: BuiltinScalarFunction::ToTimestamp,
+ args,
+ } => {
+ if !args.is_empty() {
+ match &args[0] {
+ Expr::Literal(ScalarValue::Utf8(Some(val))) => {
+ Expr::Literal(ScalarValue::TimestampNanosecond(
+ string_to_timestamp_nanos(val).ok(),
Review comment:
I don't think the `ok()` here preserves the same semantics.
I tested what happens when an invalid argument is passed to `to_timestamp` on master (via `cargo run -p datafusion-cli`):
```
> select to_timestamp('foo');
ArrowError(ExternalError(Execution("Error parsing \'foo\' as timestamp")))
```
On this branch
```
> select to_timestamp('foo');
+---------------------------+
| TimestampNanosecond(NULL) |
+---------------------------+
| |
+---------------------------+
```
I suggest that if the `to_timestamp(..)` call would generate a runtime error, then the optimizer pass should just leave the expression as is (aka don't do the rewrite and leave the original expression)
--
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-datafusion] NGA-TRAN commented on a change in pull request #387: Optimize to_timestamp function
Posted by GitBox <gi...@apache.org>.
NGA-TRAN commented on a change in pull request #387:
URL: https://github.com/apache/arrow-datafusion/pull/387#discussion_r638017200
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -632,6 +662,29 @@ mod tests {
return format!("{:?}", optimized_plan);
}
+ #[test]
+ fn to_timestamp_expr() {
+ let table_scan = test_table_scan().unwrap();
+ let proj = vec![Expr::ScalarFunction {
+ args: vec![Expr::Literal(ScalarValue::Utf8(Some(
+ "2020-09-08T12:00:00+00:00".to_string(),
+ )))],
+ fun: BuiltinScalarFunction::ToTimestamp,
+ }];
+ let plan = LogicalPlanBuilder::from(&table_scan)
+ .project(proj)
+ .unwrap()
+ .build()
+ .unwrap();
+
+ let expected = format!(
+ "Projection: TimestampNanosecond(1599566400000000000)\
+ \n TableScan: test projection=None",
+ );
+ let actual = get_optimized_plan_formatted(&plan, &chrono::Utc::now());
+ assert_eq!(expected, actual);
+ }
+
Review comment:
The positive test looks great. How about a negative test with empty argument and another negative test with non-timestamp argument
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -217,6 +218,35 @@ impl<'a> ExprRewriter for ConstantRewriter<'a> {
.query_execution_start_time
.timestamp_nanos(),
))),
+ Expr::ScalarFunction {
+ fun: BuiltinScalarFunction::ToTimestamp,
+ args,
+ } => {
+ if !args.is_empty() {
+ match &args[0] {
+ Expr::Literal(ScalarValue::Utf8(Some(val))) => {
+ match string_to_timestamp_nanos(val) {
+ Ok(timestamp) => Expr::Literal(
+ ScalarValue::TimestampNanosecond(Some(timestamp)),
+ ),
+ _ => Expr::ScalarFunction {
+ fun: BuiltinScalarFunction::ToTimestamp,
+ args,
+ },
+ }
+ }
+ _ => Expr::ScalarFunction {
+ fun: BuiltinScalarFunction::ToTimestamp,
+ args,
+ },
+ }
+ } else {
+ Expr::ScalarFunction {
+ fun: BuiltinScalarFunction::ToTimestamp,
+ args,
+ }
+ }
+ }
Review comment:
What happen if the `args.is_empty()`? Do we use a default value or return syntax error we simply return `false` for this expression?
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -632,6 +662,29 @@ mod tests {
return format!("{:?}", optimized_plan);
}
+ #[test]
+ fn to_timestamp_expr() {
+ let table_scan = test_table_scan().unwrap();
+ let proj = vec![Expr::ScalarFunction {
+ args: vec![Expr::Literal(ScalarValue::Utf8(Some(
+ "2020-09-08T12:00:00+00:00".to_string(),
+ )))],
+ fun: BuiltinScalarFunction::ToTimestamp,
+ }];
+ let plan = LogicalPlanBuilder::from(&table_scan)
+ .project(proj)
+ .unwrap()
+ .build()
+ .unwrap();
+
+ let expected = format!(
+ "Projection: TimestampNanosecond(1599566400000000000)\
+ \n TableScan: test projection=None",
+ );
+ let actual = get_optimized_plan_formatted(&plan, &chrono::Utc::now());
+ assert_eq!(expected, actual);
Review comment:
Nice
--
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-datafusion] codecov-commenter commented on pull request #387: Optimize to_timestamp function
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #387:
URL: https://github.com/apache/arrow-datafusion/pull/387#issuecomment-846444916
# [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/387?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 [#387](https://codecov.io/gh/apache/arrow-datafusion/pull/387?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ff2a20b) into [master](https://codecov.io/gh/apache/arrow-datafusion/commit/db4f098d38993b96ce1134c4bc7bf5c6579509cf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (db4f098) will **decrease** coverage by `0.03%`.
> The diff coverage is `66.66%`.
[](https://codecov.io/gh/apache/arrow-datafusion/pull/387?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 #387 +/- ##
==========================================
- Coverage 74.94% 74.90% -0.04%
==========================================
Files 146 146
Lines 24314 24323 +9
==========================================
- Hits 18221 18220 -1
- Misses 6093 6103 +10
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-datafusion/pull/387?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [datafusion/src/optimizer/constant\_folding.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL2NvbnN0YW50X2ZvbGRpbmcucnM=) | `90.93% <66.66%> (-0.71%)` | :arrow_down: |
| [...tafusion/src/physical\_plan/datetime\_expressions.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9kYXRldGltZV9leHByZXNzaW9ucy5ycw==) | `67.29% <0.00%> (-2.52%)` | :arrow_down: |
| [datafusion/src/scalar.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi9zcmMvc2NhbGFyLnJz) | `55.02% <0.00%> (-0.46%)` | :arrow_down: |
| [datafusion/src/physical\_plan/functions.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9mdW5jdGlvbnMucnM=) | `92.70% <0.00%> (-0.08%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/387?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/387?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 [db4f098...ff2a20b](https://codecov.io/gh/apache/arrow-datafusion/pull/387?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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on pull request #387: Constant fold / optimize `to_timestamp` function during planning
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #387:
URL: https://github.com/apache/arrow-datafusion/pull/387#issuecomment-848067209
> > For some strange reason cargo clippy on my local doesn't throw these errors.
>
> It may be related to what version of rust is used. I also have some troubles with clippy sometimes not finding errors. I run the following locally (I am somewhat embarrassed but it does work for me )
>
> ```
> find . -name '*.rs' | grep -v target | xargs touch && cargo clippy --all --all-targets -- -D warnings
> ```
Touching the files (or cleaning the build) should not be needed anymore since 1.52 https://blog.rust-lang.org/2021/05/06/Rust-1.52.0.html
--
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-datafusion] codecov-commenter edited a comment on pull request #387: Optimize to_timestamp function
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #387:
URL: https://github.com/apache/arrow-datafusion/pull/387#issuecomment-846444916
# [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/387?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 [#387](https://codecov.io/gh/apache/arrow-datafusion/pull/387?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6b1a362) into [master](https://codecov.io/gh/apache/arrow-datafusion/commit/db4f098d38993b96ce1134c4bc7bf5c6579509cf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (db4f098) will **decrease** coverage by `0.10%`.
> The diff coverage is `61.48%`.
> :exclamation: Current head 6b1a362 differs from pull request most recent head 43d01ef. Consider uploading reports for the commit 43d01ef to get more accurate results
[](https://codecov.io/gh/apache/arrow-datafusion/pull/387?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 #387 +/- ##
==========================================
- Coverage 74.94% 74.83% -0.11%
==========================================
Files 146 146
Lines 24314 24517 +203
==========================================
+ Hits 18221 18348 +127
- Misses 6093 6169 +76
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-datafusion/pull/387?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ta/rust/core/src/execution\_plans/shuffle\_reader.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-YmFsbGlzdGEvcnVzdC9jb3JlL3NyYy9leGVjdXRpb25fcGxhbnMvc2h1ZmZsZV9yZWFkZXIucnM=) | `0.00% <0.00%> (ø)` | |
| [datafusion/src/logical\_plan/expr.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi9zcmMvbG9naWNhbF9wbGFuL2V4cHIucnM=) | `84.34% <ø> (ø)` | |
| [datafusion/src/physical\_plan/group\_scalar.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9ncm91cF9zY2FsYXIucnM=) | `65.88% <ø> (ø)` | |
| [benchmarks/src/bin/tpch.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-YmVuY2htYXJrcy9zcmMvYmluL3RwY2gucnM=) | `30.84% <33.33%> (+0.01%)` | :arrow_up: |
| [datafusion/src/physical\_optimizer/pruning.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfb3B0aW1pemVyL3BydW5pbmcucnM=) | `89.73% <38.46%> (-0.88%)` | :arrow_down: |
| [datafusion/src/scalar.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi9zcmMvc2NhbGFyLnJz) | `58.31% <63.26%> (+2.83%)` | :arrow_up: |
| [datafusion/src/physical\_plan/repartition.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9yZXBhcnRpdGlvbi5ycw==) | `82.45% <70.96%> (-1.89%)` | :arrow_down: |
| [datafusion/src/physical\_plan/parquet.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9wYXJxdWV0LnJz) | `81.76% <83.33%> (+0.49%)` | :arrow_up: |
| [datafusion/src/optimizer/constant\_folding.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL2NvbnN0YW50X2ZvbGRpbmcucnM=) | `91.21% <84.21%> (-0.43%)` | :arrow_down: |
| [datafusion/src/physical\_plan/hash\_aggregate.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9oYXNoX2FnZ3JlZ2F0ZS5ycw==) | `85.21% <100.00%> (+0.36%)` | :arrow_up: |
| ... and [4 more](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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/387?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/387?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 [db4f098...43d01ef](https://codecov.io/gh/apache/arrow-datafusion/pull/387?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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan edited a comment on pull request #387: Constant fold / optimize `to_timestamp` function during planning
Posted by GitBox <gi...@apache.org>.
Dandandan edited a comment on pull request #387:
URL: https://github.com/apache/arrow-datafusion/pull/387#issuecomment-848067209
> > For some strange reason cargo clippy on my local doesn't throw these errors.
>
> It may be related to what version of rust is used. I also have some troubles with clippy sometimes not finding errors. I run the following locally (I am somewhat embarrassed but it does work for me )
>
> ```
> find . -name '*.rs' | grep -v target | xargs touch && cargo clippy --all --all-targets -- -D warnings
> ```
Touching the files (or cleaning the build) should not be needed anymore since 1.52 https://blog.rust-lang.org/2021/05/06/Rust-1.52.0.html
> Previously, running cargo check followed by cargo clippy wouldn't actually run Clippy: the build caching in Cargo didn't differentiate between the two. In 1.52, however, this has been fixed, which means that users will get the expected behavior independent of the order in which they run the two commands.
--
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-datafusion] msathis commented on a change in pull request #387: Optimize to_timestamp function
Posted by GitBox <gi...@apache.org>.
msathis commented on a change in pull request #387:
URL: https://github.com/apache/arrow-datafusion/pull/387#discussion_r638137645
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -632,6 +662,29 @@ mod tests {
return format!("{:?}", optimized_plan);
}
+ #[test]
+ fn to_timestamp_expr() {
+ let table_scan = test_table_scan().unwrap();
+ let proj = vec![Expr::ScalarFunction {
+ args: vec![Expr::Literal(ScalarValue::Utf8(Some(
+ "2020-09-08T12:00:00+00:00".to_string(),
+ )))],
+ fun: BuiltinScalarFunction::ToTimestamp,
+ }];
+ let plan = LogicalPlanBuilder::from(&table_scan)
+ .project(proj)
+ .unwrap()
+ .build()
+ .unwrap();
+
+ let expected = format!(
+ "Projection: TimestampNanosecond(1599566400000000000)\
+ \n TableScan: test projection=None",
+ );
+ let actual = get_optimized_plan_formatted(&plan, &chrono::Utc::now());
+ assert_eq!(expected, actual);
+ }
+
Review comment:
Yup. Makes sense. I will add it.
--
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-datafusion] alamb commented on pull request #387: Constant fold / optimize `to_timestamp` function during planning
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #387:
URL: https://github.com/apache/arrow-datafusion/pull/387#issuecomment-847221931
Oh no -- we have some clippy errors
https://github.com/apache/arrow-datafusion/pull/387/checks?check_run_id=2658215529
```
error: useless use of `format!`
--> datafusion/src/optimizer/constant_folding.rs:680:24
|
680 | let expected = format!(
| ________________________^
681 | | "Projection: TimestampNanosecond(1599566400000000000)\
682 | | \n TableScan: test projection=None",
683 | | );
| |_________^
|
= note: `-D clippy::useless-format` implied by `-D warnings`
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_format
help: consider using `.to_string()`
|
680 | let expected = "Projection: TimestampNanosecond(1599566400000000000)\
681 | \n TableScan: test projection=None".to_string();
|
error: useless use of `format!`
--> datafusion/src/optimizer/constant_folding.rs:703:24
|
703 | let expected = format!(
| ________________________^
704 | | "Projection: totimestamp(Utf8(\"I\'M NOT A TIMESTAMP\"))\
705 | | \n TableScan: test projection=None",
706 | | );
| |_________^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_format
help: consider using `.to_string()`
```
--
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-datafusion] msathis commented on a change in pull request #387: Optimize to_timestamp function
Posted by GitBox <gi...@apache.org>.
msathis commented on a change in pull request #387:
URL: https://github.com/apache/arrow-datafusion/pull/387#discussion_r637772382
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -217,6 +218,29 @@ impl<'a> ExprRewriter for ConstantRewriter<'a> {
.query_execution_start_time
.timestamp_nanos(),
))),
+ Expr::ScalarFunction {
+ fun: BuiltinScalarFunction::ToTimestamp,
+ args,
+ } => {
+ if !args.is_empty() {
+ match &args[0] {
+ Expr::Literal(ScalarValue::Utf8(Some(val))) => {
+ Expr::Literal(ScalarValue::TimestampNanosecond(
+ string_to_timestamp_nanos(val).ok(),
Review comment:
Makes sense. I'll get this fixed today 👍
--
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-datafusion] alamb commented on a change in pull request #387: Optimize to_timestamp function
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #387:
URL: https://github.com/apache/arrow-datafusion/pull/387#discussion_r637931538
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -217,6 +218,29 @@ impl<'a> ExprRewriter for ConstantRewriter<'a> {
.query_execution_start_time
.timestamp_nanos(),
))),
+ Expr::ScalarFunction {
+ fun: BuiltinScalarFunction::ToTimestamp,
+ args,
+ } => {
+ if !args.is_empty() {
+ match &args[0] {
+ Expr::Literal(ScalarValue::Utf8(Some(val))) => {
+ Expr::Literal(ScalarValue::TimestampNanosecond(
+ string_to_timestamp_nanos(val).ok(),
Review comment:
Thank you
--
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-datafusion] codecov-commenter edited a comment on pull request #387: Constant fold / optimize `to_timestamp` function during planning
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #387:
URL: https://github.com/apache/arrow-datafusion/pull/387#issuecomment-846444916
# [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/387?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 [#387](https://codecov.io/gh/apache/arrow-datafusion/pull/387?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7961343) into [master](https://codecov.io/gh/apache/arrow-datafusion/commit/db4f098d38993b96ce1134c4bc7bf5c6579509cf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (db4f098) will **decrease** coverage by `0.09%`.
> The diff coverage is `61.70%`.
> :exclamation: Current head 7961343 differs from pull request most recent head 2b9b9b7. Consider uploading reports for the commit 2b9b9b7 to get more accurate results
[](https://codecov.io/gh/apache/arrow-datafusion/pull/387?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 #387 +/- ##
==========================================
- Coverage 74.94% 74.84% -0.10%
==========================================
Files 146 146
Lines 24314 24553 +239
==========================================
+ Hits 18221 18377 +156
- Misses 6093 6176 +83
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-datafusion/pull/387?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ta/rust/core/src/execution\_plans/shuffle\_reader.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-YmFsbGlzdGEvcnVzdC9jb3JlL3NyYy9leGVjdXRpb25fcGxhbnMvc2h1ZmZsZV9yZWFkZXIucnM=) | `0.00% <0.00%> (ø)` | |
| [datafusion-cli/src/main.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi1jbGkvc3JjL21haW4ucnM=) | `0.00% <0.00%> (ø)` | |
| [datafusion/src/logical\_plan/expr.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi9zcmMvbG9naWNhbF9wbGFuL2V4cHIucnM=) | `84.34% <ø> (ø)` | |
| [datafusion/src/physical\_plan/group\_scalar.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9ncm91cF9zY2FsYXIucnM=) | `65.88% <ø> (ø)` | |
| [benchmarks/src/bin/tpch.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-YmVuY2htYXJrcy9zcmMvYmluL3RwY2gucnM=) | `30.84% <11.11%> (+0.01%)` | :arrow_up: |
| [datafusion/src/physical\_optimizer/pruning.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfb3B0aW1pemVyL3BydW5pbmcucnM=) | `89.73% <38.46%> (-0.88%)` | :arrow_down: |
| [datafusion-cli/src/print\_format.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi1jbGkvc3JjL3ByaW50X2Zvcm1hdC5ycw==) | `84.44% <58.82%> (-5.97%)` | :arrow_down: |
| [datafusion/src/scalar.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi9zcmMvc2NhbGFyLnJz) | `58.31% <63.26%> (+2.83%)` | :arrow_up: |
| [datafusion/src/physical\_plan/repartition.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9yZXBhcnRpdGlvbi5ycw==) | `82.45% <70.96%> (-1.89%)` | :arrow_down: |
| [datafusion/src/physical\_plan/parquet.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9wYXJxdWV0LnJz) | `81.76% <83.33%> (+0.49%)` | :arrow_up: |
| ... and [6 more](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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/387?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/387?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 [db4f098...2b9b9b7](https://codecov.io/gh/apache/arrow-datafusion/pull/387?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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #387: Optimize to_timestamp function
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #387:
URL: https://github.com/apache/arrow-datafusion/pull/387#discussion_r638150373
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -632,6 +662,73 @@ mod tests {
return format!("{:?}", optimized_plan);
}
+ #[test]
+ fn to_timestamp_expr() {
+ let table_scan = test_table_scan().unwrap();
+ let proj = vec![Expr::ScalarFunction {
+ args: vec![Expr::Literal(ScalarValue::Utf8(Some(
+ "2020-09-08T12:00:00+00:00".to_string(),
+ )))],
+ fun: BuiltinScalarFunction::ToTimestamp,
+ }];
+ let plan = LogicalPlanBuilder::from(&table_scan)
+ .project(proj)
+ .unwrap()
+ .build()
+ .unwrap();
+
+ let expected = format!(
+ "Projection: TimestampNanosecond(1599566400000000000)\
+ \n TableScan: test projection=None",
+ );
+ let actual = get_optimized_plan_formatted(&plan, &chrono::Utc::now());
+ assert_eq!(expected, actual);
+ }
+
+ #[test]
+ fn to_timestamp_expr_wrong_arg() {
+ let table_scan = test_table_scan().unwrap();
+ let proj = vec![Expr::ScalarFunction {
+ args: vec![Expr::Literal(ScalarValue::Utf8(Some(
+ "I'M NOT A TIMESTAMP".to_string(),
Review comment:
😆
--
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-datafusion] msathis commented on pull request #387: Constant fold / optimize `to_timestamp` function during planning
Posted by GitBox <gi...@apache.org>.
msathis commented on pull request #387:
URL: https://github.com/apache/arrow-datafusion/pull/387#issuecomment-848061575
> > For some strange reason cargo clippy on my local doesn't throw these errors.
>
> It may be related to what version of rust is used. I also have some troubles with clippy sometimes not finding errors. I run the following locally (I am somewhat embarrassed but it does work for me 🤷 )
>
> ```
> find . -name '*.rs' | grep -v target | xargs touch && cargo clippy --all --all-targets -- -D warnings
> ```
Thanks. That's nice. I'll copy & use it 👍
--
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-datafusion] msathis commented on a change in pull request #387: Optimize to_timestamp function
Posted by GitBox <gi...@apache.org>.
msathis commented on a change in pull request #387:
URL: https://github.com/apache/arrow-datafusion/pull/387#discussion_r638142927
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -632,6 +662,29 @@ mod tests {
return format!("{:?}", optimized_plan);
}
+ #[test]
+ fn to_timestamp_expr() {
+ let table_scan = test_table_scan().unwrap();
+ let proj = vec![Expr::ScalarFunction {
+ args: vec![Expr::Literal(ScalarValue::Utf8(Some(
+ "2020-09-08T12:00:00+00:00".to_string(),
+ )))],
+ fun: BuiltinScalarFunction::ToTimestamp,
+ }];
+ let plan = LogicalPlanBuilder::from(&table_scan)
+ .project(proj)
+ .unwrap()
+ .build()
+ .unwrap();
+
+ let expected = format!(
+ "Projection: TimestampNanosecond(1599566400000000000)\
+ \n TableScan: test projection=None",
+ );
+ let actual = get_optimized_plan_formatted(&plan, &chrono::Utc::now());
+ assert_eq!(expected, actual);
+ }
+
Review comment:
Done
--
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-datafusion] alamb merged pull request #387: Constant fold / optimize `to_timestamp` function during planning
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #387:
URL: https://github.com/apache/arrow-datafusion/pull/387
--
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-datafusion] alamb commented on pull request #387: Constant fold / optimize `to_timestamp` function during planning
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #387:
URL: https://github.com/apache/arrow-datafusion/pull/387#issuecomment-847988932
--
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-datafusion] codecov-commenter edited a comment on pull request #387: Optimize to_timestamp function
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #387:
URL: https://github.com/apache/arrow-datafusion/pull/387#issuecomment-846444916
# [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/387?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 [#387](https://codecov.io/gh/apache/arrow-datafusion/pull/387?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (18cd760) into [master](https://codecov.io/gh/apache/arrow-datafusion/commit/db4f098d38993b96ce1134c4bc7bf5c6579509cf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (db4f098) will **decrease** coverage by `0.03%`.
> The diff coverage is `66.66%`.
> :exclamation: Current head 18cd760 differs from pull request most recent head c32d2f5. Consider uploading reports for the commit c32d2f5 to get more accurate results
[](https://codecov.io/gh/apache/arrow-datafusion/pull/387?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 #387 +/- ##
==========================================
- Coverage 74.94% 74.90% -0.04%
==========================================
Files 146 146
Lines 24314 24323 +9
==========================================
- Hits 18221 18220 -1
- Misses 6093 6103 +10
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-datafusion/pull/387?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [datafusion/src/optimizer/constant\_folding.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL2NvbnN0YW50X2ZvbGRpbmcucnM=) | `90.93% <66.66%> (-0.71%)` | :arrow_down: |
| [...tafusion/src/physical\_plan/datetime\_expressions.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9kYXRldGltZV9leHByZXNzaW9ucy5ycw==) | `67.29% <0.00%> (-2.52%)` | :arrow_down: |
| [datafusion/src/scalar.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi9zcmMvc2NhbGFyLnJz) | `55.02% <0.00%> (-0.46%)` | :arrow_down: |
| [datafusion/src/physical\_plan/functions.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/387/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-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9mdW5jdGlvbnMucnM=) | `92.70% <0.00%> (-0.08%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/387?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/387?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 [db4f098...c32d2f5](https://codecov.io/gh/apache/arrow-datafusion/pull/387?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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on a change in pull request #387: Optimize to_timestamp function
Posted by GitBox <gi...@apache.org>.
msathis commented on a change in pull request #387:
URL: https://github.com/apache/arrow-datafusion/pull/387#discussion_r638138544
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -217,6 +218,35 @@ impl<'a> ExprRewriter for ConstantRewriter<'a> {
.query_execution_start_time
.timestamp_nanos(),
))),
+ Expr::ScalarFunction {
+ fun: BuiltinScalarFunction::ToTimestamp,
+ args,
+ } => {
+ if !args.is_empty() {
+ match &args[0] {
+ Expr::Literal(ScalarValue::Utf8(Some(val))) => {
+ match string_to_timestamp_nanos(val) {
+ Ok(timestamp) => Expr::Literal(
+ ScalarValue::TimestampNanosecond(Some(timestamp)),
+ ),
+ _ => Expr::ScalarFunction {
+ fun: BuiltinScalarFunction::ToTimestamp,
+ args,
+ },
+ }
+ }
+ _ => Expr::ScalarFunction {
+ fun: BuiltinScalarFunction::ToTimestamp,
+ args,
+ },
+ }
+ } else {
+ Expr::ScalarFunction {
+ fun: BuiltinScalarFunction::ToTimestamp,
+ args,
+ }
+ }
+ }
Review comment:
In case of all error cases including empty, we don't optimise. So the default non-optimised flow continues.
--
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