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:
[email protected]
