alamb commented on a change in pull request #1176:
URL: https://github.com/apache/arrow-datafusion/pull/1176#discussion_r735915407
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -838,17 +796,16 @@ mod tests {
// now() < cast(to_timestamp(...) as int) + 5000000000
let plan = LogicalPlanBuilder::from(table_scan)
.filter(
- now_expr()
+ cast_to_int64_expr(now_expr())
Review comment:
It will be pretty awesome when we get
https://github.com/apache/arrow-datafusion/issues/194 so casting to do
timestamp arithmetic is no longer needed
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -753,27 +732,6 @@ mod tests {
}
}
- #[test]
Review comment:
covered in the tests for `ConstEvaluator`
##########
File path: datafusion/src/optimizer/utils.rs
##########
@@ -604,9 +612,8 @@ impl ConstEvaluator {
fn volatility_ok(volatility: Volatility) -> bool {
match volatility {
Volatility::Immutable => true,
- // To evaluate stable functions, need ExecutionProps, see
- // Simplifier for code that does that.
- Volatility::Stable => false,
+ // Values for functions such as now() are taken from ExecutionProps
Review comment:
Here is the actual change that allows the const evaluator to replace
`now()` with a constant.
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -838,17 +796,16 @@ mod tests {
// now() < cast(to_timestamp(...) as int) + 5000000000
let plan = LogicalPlanBuilder::from(table_scan)
.filter(
- now_expr()
+ cast_to_int64_expr(now_expr())
.lt(cast_to_int64_expr(to_timestamp_expr(ts_string)) +
lit(50000)),
)
.unwrap()
.build()
.unwrap();
- // Note that constant folder should be able to run again and fold
- // this whole expression down to a single constant;
- // https://github.com/apache/arrow-datafusion/issues/1160
- let expected = "Filter: TimestampNanosecond(1599566400000000000) <
CAST(totimestamp(Utf8(\"2020-09-08T12:05:00+00:00\")) AS Int64) + Int32(50000)\
+ // Note that constant folder runs and folds the entire
+ // expression down to a single constant (true)
+ let expected = "Filter: Boolean(true)\
Review comment:
The filter expression has been totally simplified 🎉
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -228,15 +220,6 @@ impl<'a> ExprRewriter for Simplifier<'a> {
Expr::Not(inner)
}
}
- // convert now() --> the time in `ExecutionProps`
Review comment:
The point of this PR is to remove this code (it is now handled by
ConstEvaluator)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
