alamb commented on a change in pull request #1375:
URL: https://github.com/apache/arrow-datafusion/pull/1375#discussion_r759201211
##########
File path: datafusion/src/optimizer/simplify_expressions.rs
##########
@@ -284,9 +274,62 @@ impl OptimizerRule for SimplifyExpressions {
fn optimize(
&self,
plan: &LogicalPlan,
- _execution_props: &ExecutionProps,
+ execution_props: &ExecutionProps,
) -> Result<LogicalPlan> {
- optimize(plan)
+ // We need to pass down the all schemas within the plan tree to
`optimize_expr` in order to
+ // to evaluate expression types. For example, a projection plan's
schema will only include
+ // projected columns. With just the projected schema, it's not
possible to infer types for
+ // expressions that references non-projected columns within the same
project plan or its
+ // children plans.
+ let mut simplifier =
Review comment:
this logic is moved from ConstantFolding
##########
File path: datafusion/src/optimizer/simplify_expressions.rs
##########
@@ -284,9 +274,62 @@ impl OptimizerRule for SimplifyExpressions {
fn optimize(
&self,
plan: &LogicalPlan,
- _execution_props: &ExecutionProps,
+ execution_props: &ExecutionProps,
) -> Result<LogicalPlan> {
- optimize(plan)
+ // We need to pass down the all schemas within the plan tree to
`optimize_expr` in order to
+ // to evaluate expression types. For example, a projection plan's
schema will only include
+ // projected columns. With just the projected schema, it's not
possible to infer types for
+ // expressions that references non-projected columns within the same
project plan or its
+ // children plans.
+ let mut simplifier =
+ super::simplify_expressions::Simplifier::new(plan.all_schemas());
+
+ let mut const_evaluator =
+ super::simplify_expressions::ConstEvaluator::new(execution_props);
+
+ let new_inputs = plan
+ .inputs()
+ .iter()
+ .map(|input| self.optimize(input, execution_props))
+ .collect::<Result<Vec<_>>>()?;
+
+ let expr = plan
+ .expressions()
+ .into_iter()
+ .map(|e| {
+ // We need to keep original expression name, if any.
+ // Constant folding should not change expression name.
+ let name = &e.name(plan.schema());
+
+ // TODO combine simplify into Simplifier
+ let e = simplify(&e);
+
+ // TODO iterate until no changes are made
+ // during rewrite (evaluating constants can
+ // enable new simplifications and
+ // simplifications can enable new constant
+ // evaluation)
+ let new_e = e
+ // fold constants and then simplify
+ .rewrite(&mut const_evaluator)?
+ .rewrite(&mut simplifier)?;
+
+ let new_name = &new_e.name(plan.schema());
+
+ // TODO simplify this logic
+ if let (Ok(expr_name), Ok(new_expr_name)) = (name, new_name) {
Review comment:
Note that now the fix for alias names (introduced by @viirya in
https://github.com/apache/arrow-datafusion/pull/1319) is applied to the result
of `simplify` as well -- previously it was not
##########
File path: datafusion/src/optimizer/simplify_expressions.rs
##########
@@ -284,9 +274,62 @@ impl OptimizerRule for SimplifyExpressions {
fn optimize(
&self,
plan: &LogicalPlan,
- _execution_props: &ExecutionProps,
+ execution_props: &ExecutionProps,
) -> Result<LogicalPlan> {
- optimize(plan)
+ // We need to pass down the all schemas within the plan tree to
`optimize_expr` in order to
+ // to evaluate expression types. For example, a projection plan's
schema will only include
+ // projected columns. With just the projected schema, it's not
possible to infer types for
+ // expressions that references non-projected columns within the same
project plan or its
+ // children plans.
+ let mut simplifier =
+ super::simplify_expressions::Simplifier::new(plan.all_schemas());
+
+ let mut const_evaluator =
+ super::simplify_expressions::ConstEvaluator::new(execution_props);
+
+ let new_inputs = plan
+ .inputs()
+ .iter()
+ .map(|input| self.optimize(input, execution_props))
+ .collect::<Result<Vec<_>>>()?;
+
+ let expr = plan
+ .expressions()
+ .into_iter()
+ .map(|e| {
+ // We need to keep original expression name, if any.
+ // Constant folding should not change expression name.
+ let name = &e.name(plan.schema());
+
+ // TODO combine simplify into Simplifier
+ let e = simplify(&e);
Review comment:
Here is the call into the existing `simplify` function that is in the
`SimplifyExpressions` pass (originally written by @jgoday)
##########
File path: datafusion/src/optimizer/simplify_expressions.rs
##########
@@ -1562,4 +1556,388 @@ mod tests {
.unwrap(),
)
}
+
Review comment:
these tests are just moved from `constant_folding.rs`
--
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]
