alamb commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r630061415
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -200,6 +209,15 @@ impl<'a> ExprRewriter for ConstantRewriter<'a> {
Expr::Not(inner)
}
}
+ Expr::ScalarFunction {
Review comment:
❤️ looking very nice
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -200,6 +209,15 @@ impl<'a> ExprRewriter for ConstantRewriter<'a> {
Expr::Not(inner)
}
}
+ Expr::ScalarFunction {
+ fun: BuiltinScalarFunction::Now,
+ ..
+ } => Expr::Literal(ScalarValue::TimestampNanosecond(Some(
+ self.execution_props
+ .query_execution_start_time
+ .unwrap()
Review comment:
It may be worth a comment here about why the code assumes that
`query_execution_start_time` is set -- it is always set prior to the optimizer
being run.
While I am typing this, I almost wonder if we could change `ExecutionProps`
so that `query_execution_time` was always set (set it on construction). We
could do that as a follow on PR perhaps
##########
File path: datafusion/src/physical_plan/datetime_expressions.rs
##########
@@ -268,6 +268,13 @@ pub fn to_timestamp(args: &[ColumnarValue]) ->
Result<ColumnarValue> {
)
}
+/// now SQL function
+pub fn now(_: &[ColumnarValue]) -> Result<ColumnarValue> {
+ Ok(ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(
+ Some(chrono::Utc::now().timestamp_nanos()),
Review comment:
It seems to me that this function should never actually be called if the
plan has been optimized (as this function call should have been replaced by a
scalar function during constant folding).
Thus if this code is hit, wouldn't it be a bug? I can think of two possible
behaviors:
1. The code we have now (generate a value, but not the intended one)
2. Return an error.
If we return an error it starts meaning we can't run plans without the
optimizer which may not be ideal (I am not sure).
Thus, in terms of "being nice to users" I would suggest option 1 and add a
`warn!` here saying that the plan hasn't been optimized.
```suggestion
// this function should have been replaced with a constant as part
of optimization
// it this code is being run it likely means 1) there is a bug in
constant folding or
// 2) the plan was not optimized prior to being run.
warn!("now() function has not been optimized");
Some(chrono::Utc::now().timestamp_nanos()),
```
--
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:
us...@infra.apache.org
