alamb commented on code in PR #10318:
URL: https://github.com/apache/datafusion/pull/10318#discussion_r1585360693
##########
datafusion/optimizer/src/decorrelate_predicate_subquery.rs:
##########
@@ -103,62 +106,85 @@ impl DecorrelatePredicateSubquery {
impl OptimizerRule for DecorrelatePredicateSubquery {
fn try_optimize(
&self,
- plan: &LogicalPlan,
- config: &dyn OptimizerConfig,
+ _plan: &LogicalPlan,
+ _config: &dyn OptimizerConfig,
) -> Result<Option<LogicalPlan>> {
- match plan {
- LogicalPlan::Filter(filter) => {
- let (subqueries, mut other_exprs) =
- self.extract_subquery_exprs(&filter.predicate, config)?;
- if subqueries.is_empty() {
- // regular filter, no subquery exists clause here
- return Ok(None);
- }
+ internal_err!("Should have called
DecorrelatePredicateSubquery::rewrite")
+ }
- // iterate through all exists clauses in predicate, turning
each into a join
- let mut cur_input = filter.input.as_ref().clone();
- for subquery in subqueries {
- if let Some(plan) =
- build_join(&subquery, &cur_input,
config.alias_generator())?
- {
- cur_input = plan;
- } else {
- // If the subquery can not be converted to a Join,
reconstruct the subquery expression and add it to the Filter
- let sub_query_expr = match subquery {
- SubqueryInfo {
- query,
- where_in_expr: Some(expr),
- negated: false,
- } => in_subquery(expr, query.subquery.clone()),
- SubqueryInfo {
- query,
- where_in_expr: Some(expr),
- negated: true,
- } => not_in_subquery(expr, query.subquery.clone()),
- SubqueryInfo {
- query,
- where_in_expr: None,
- negated: false,
- } => exists(query.subquery.clone()),
- SubqueryInfo {
- query,
- where_in_expr: None,
- negated: true,
- } => not_exists(query.subquery.clone()),
- };
- other_exprs.push(sub_query_expr);
- }
- }
+ fn supports_rewrite(&self) -> bool {
+ true
+ }
- let expr = conjunction(other_exprs);
- if let Some(expr) = expr {
- let new_filter = Filter::try_new(expr,
Arc::new(cur_input))?;
- cur_input = LogicalPlan::Filter(new_filter);
- }
- Ok(Some(cur_input))
+ fn rewrite(
+ &self,
+ plan: LogicalPlan,
+ config: &dyn OptimizerConfig,
+ ) -> Result<Transformed<LogicalPlan>> {
+ let LogicalPlan::Filter(filter) = plan else {
+ return Ok(Transformed::no(plan));
+ };
+
+ // if there are no subqueries in the predicate, return the original
plan
+ let has_subqueries = split_conjunction(&filter.predicate)
+ .iter()
+ .any(|expr| matches!(expr, Expr::InSubquery(_) | Expr::Exists(_)));
+ if !has_subqueries {
+ return Ok(Transformed::no(LogicalPlan::Filter(filter)));
Review Comment:
in the common case where there are no subqueries there will be no rewriting
/ cloning (which is the same as main
##########
datafusion/optimizer/src/decorrelate_predicate_subquery.rs:
##########
@@ -103,62 +106,85 @@ impl DecorrelatePredicateSubquery {
impl OptimizerRule for DecorrelatePredicateSubquery {
fn try_optimize(
&self,
- plan: &LogicalPlan,
- config: &dyn OptimizerConfig,
+ _plan: &LogicalPlan,
+ _config: &dyn OptimizerConfig,
) -> Result<Option<LogicalPlan>> {
- match plan {
- LogicalPlan::Filter(filter) => {
- let (subqueries, mut other_exprs) =
- self.extract_subquery_exprs(&filter.predicate, config)?;
- if subqueries.is_empty() {
- // regular filter, no subquery exists clause here
- return Ok(None);
- }
+ internal_err!("Should have called
DecorrelatePredicateSubquery::rewrite")
+ }
- // iterate through all exists clauses in predicate, turning
each into a join
- let mut cur_input = filter.input.as_ref().clone();
- for subquery in subqueries {
- if let Some(plan) =
- build_join(&subquery, &cur_input,
config.alias_generator())?
- {
- cur_input = plan;
- } else {
- // If the subquery can not be converted to a Join,
reconstruct the subquery expression and add it to the Filter
- let sub_query_expr = match subquery {
- SubqueryInfo {
- query,
- where_in_expr: Some(expr),
- negated: false,
- } => in_subquery(expr, query.subquery.clone()),
Review Comment:
these are clones of `Arc`s so while this PR removes them and it is less
cloning realistically it doesn't matter
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -37,7 +37,7 @@ use std::sync::Arc;
use arrow::datatypes::{DataType, Schema};
use arrow::record_batch::RecordBatch;
-use datafusion_common::Result;
+use datafusion_common::{internal_err, Result};
Review Comment:
This is the change from https://github.com/apache/datafusion/pull/10317 to
get the CI to pass
##########
datafusion/optimizer/src/decorrelate_predicate_subquery.rs:
##########
@@ -103,62 +106,85 @@ impl DecorrelatePredicateSubquery {
impl OptimizerRule for DecorrelatePredicateSubquery {
fn try_optimize(
&self,
- plan: &LogicalPlan,
- config: &dyn OptimizerConfig,
+ _plan: &LogicalPlan,
+ _config: &dyn OptimizerConfig,
) -> Result<Option<LogicalPlan>> {
- match plan {
- LogicalPlan::Filter(filter) => {
- let (subqueries, mut other_exprs) =
- self.extract_subquery_exprs(&filter.predicate, config)?;
- if subqueries.is_empty() {
- // regular filter, no subquery exists clause here
- return Ok(None);
- }
+ internal_err!("Should have called
DecorrelatePredicateSubquery::rewrite")
+ }
- // iterate through all exists clauses in predicate, turning
each into a join
- let mut cur_input = filter.input.as_ref().clone();
Review Comment:
this is one avoided clone of the input plan
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
