jonahgao commented on code in PR #7909:
URL: https://github.com/apache/arrow-datafusion/pull/7909#discussion_r1369772035
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -652,32 +653,60 @@ impl OptimizerRule for PushDownFilter {
child_plan.with_new_inputs(&[new_filter])?
}
LogicalPlan::Projection(projection) => {
- // A projection is filter-commutable, but re-writes all
predicate expressions
+ // A projection is filter-commutable if it do not contain
volatile predicates or contain volatile
+ // predicates that are not used in the filter. However, we
should re-writes all predicate expressions.
// collect projection.
- let replace_map = projection
- .schema
- .fields()
- .iter()
- .enumerate()
- .map(|(i, field)| {
- // strip alias, as they should not be part of filters
- let expr = match &projection.expr[i] {
- Expr::Alias(Alias { expr, .. }) =>
expr.as_ref().clone(),
- expr => expr.clone(),
- };
-
- (field.qualified_name(), expr)
- })
- .collect::<HashMap<_, _>>();
+ let (volatile_map, non_volatile_map): (HashMap<_, _>,
HashMap<_, _>) =
+ projection
+ .schema
+ .fields()
+ .iter()
+ .enumerate()
+ .map(|(i, field)| {
+ // strip alias, as they should not be part of
filters
+ let expr = match &projection.expr[i] {
+ Expr::Alias(Alias { expr, .. }) =>
expr.as_ref().clone(),
+ expr => expr.clone(),
+ };
+
+ (field.qualified_name(), expr)
+ })
+ .partition(|(_, value)|
is_volatile_expression(value.clone()));
Review Comment:
```suggestion
.partition(|(_, value)|
is_volatile_expression(value));
```
f the parameters is passed by reference, the `clone` can be removed.
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -652,32 +653,60 @@ impl OptimizerRule for PushDownFilter {
child_plan.with_new_inputs(&[new_filter])?
}
LogicalPlan::Projection(projection) => {
- // A projection is filter-commutable, but re-writes all
predicate expressions
+ // A projection is filter-commutable if it do not contain
volatile predicates or contain volatile
+ // predicates that are not used in the filter. However, we
should re-writes all predicate expressions.
// collect projection.
- let replace_map = projection
- .schema
- .fields()
- .iter()
- .enumerate()
- .map(|(i, field)| {
- // strip alias, as they should not be part of filters
- let expr = match &projection.expr[i] {
- Expr::Alias(Alias { expr, .. }) =>
expr.as_ref().clone(),
- expr => expr.clone(),
- };
-
- (field.qualified_name(), expr)
- })
- .collect::<HashMap<_, _>>();
+ let (volatile_map, non_volatile_map): (HashMap<_, _>,
HashMap<_, _>) =
+ projection
+ .schema
+ .fields()
+ .iter()
+ .enumerate()
+ .map(|(i, field)| {
+ // strip alias, as they should not be part of
filters
+ let expr = match &projection.expr[i] {
+ Expr::Alias(Alias { expr, .. }) =>
expr.as_ref().clone(),
+ expr => expr.clone(),
+ };
+
+ (field.qualified_name(), expr)
+ })
+ .partition(|(_, value)|
is_volatile_expression(value.clone()));
Review Comment:
```suggestion
.partition(|(_, value)|
is_volatile_expression(value));
```
If the parameters is passed by reference, the `clone` can be removed.
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
