alamb commented on code in PR #4733:
URL: https://github.com/apache/arrow-datafusion/pull/4733#discussion_r1057281418
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -143,10 +143,15 @@ impl OptimizerRule for CommonSubexprEliminate {
)?;
if let Some(predicate) = pop_expr(&mut new_expr)?.pop() {
- Ok(Some(LogicalPlan::Filter(Filter::try_new(
+ let filter = LogicalPlan::Filter(Filter::try_new(
Review Comment:
💯 for using `try_new`
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -209,16 +214,21 @@ impl OptimizerRule for CommonSubexprEliminate {
}
LogicalPlan::Sort(Sort { expr, input, fetch }) => {
let input_schema = Arc::clone(input.schema());
- let arrays = to_arrays(expr, input_schema, &mut expr_set)?;
+ let arrays = to_arrays(expr, input_schema.clone(), &mut
expr_set)?;
let (mut new_expr, new_input) =
self.rewrite_expr(&[expr], &[&arrays], input, &mut
expr_set, config)?;
- Ok(Some(LogicalPlan::Sort(Sort {
+ let sort = LogicalPlan::Sort(Sort {
expr: pop_expr(&mut new_expr)?,
input: Arc::new(new_input),
fetch: *fetch,
- })))
+ });
+ if sort.schema() == &input_schema {
+ Ok(Some(sort))
+ } else {
+ Ok(Some(build_recover_project_plan(&input_schema, sort)))
Review Comment:
I wonder if it would be possible to reduce the replication as well as be
slightly more "defensive" coding style if instead of
`build_recover_project_plan` on each path, it could be called once to ensure
the schema was always the same
Something like
```rust
impl OptimizerRule for CommonSubexprEliminate {
fn try_optimize(
&self,
plan: &LogicalPlan,
config: &dyn OptimizerConfig,
) -> Result<Option<LogicalPlan>> {
let mut expr_set = ExprSet::new();
let input_schema = Arc::clone(&plan.schema());
let new_plan = match plan {
...
}?;
if new_plan.schema() == &input_schema {
Ok(Some(new_plan))
} else {
Ok(Some(build_recover_project_plan(&input_schema, new_plan)))
}
}
```
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -858,4 +911,26 @@ mod test {
]"###;
assert_eq!(expected, formatted_fields_with_datatype);
}
+
+ #[test]
+ fn filter_schema_changed() -> Result<()> {
+ let table_scan = test_table_scan()?;
+
+ let plan = LogicalPlanBuilder::from(table_scan)
+ .filter(binary_expr(
+ binary_expr(lit(1), Operator::Gt, col("a")),
+ Operator::And,
+ binary_expr(lit(1), Operator::Gt, col("a")),
Review Comment:
You might be able to write this in a more "fluent" style if you wanted
```suggestion
.filter(lit(1).gt(col("a"))).and(
lit(1).gt(col("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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
