sririshindra commented on a change in pull request #28885: URL: https://github.com/apache/spark/pull/28885#discussion_r445859500
########## File path: sql/core/src/main/scala/org/apache/spark/sql/execution/reuse/Reuse.scala ########## @@ -0,0 +1,95 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql.execution.reuse + +import scala.collection.mutable.Map + +import org.apache.spark.sql.catalyst.rules.Rule +import org.apache.spark.sql.execution.{BaseSubqueryExec, ExecSubqueryExpression, ReusedSubqueryExec, SparkPlan} +import org.apache.spark.sql.execution.exchange.{Exchange, ReusedExchangeExec} +import org.apache.spark.sql.internal.SQLConf +import org.apache.spark.sql.types.StructType + +/** + * Find out duplicated exchanges and subqueries in the whole spark plan including subqueries, then + * use the same exhange or subquery for all the references. + */ +case class WholePlanReuse(conf: SQLConf) extends Rule[SparkPlan] { + + def apply(plan: SparkPlan): SparkPlan = { + if (conf.exchangeReuseEnabled || conf.subqueryReuseEnabled) { Review comment: Shouldn't this be `if (conf.wholePlanReuseEnabled) { `instead of `if (conf.exchangeReuseEnabled || conf.subqueryReuseEnabled) {`. We could take the position that if wholePlanReuse is enabled then it means both exchangeReuseEnabled and subqueryReuseEnabled are set to true. So setting wholePlanReuseEnabled to true will override whatever user specified value for exchangeReuseEnabled and subqueryReuseEnabled is. if that is the case then in the current commit even `if (conf.wholePlanReuseEnabled) { ` will be redundant because this piece of code will only be triggered when wholePlanReuseEnabled is set to true. But if we want to say that the flags exchangeReuseEnabled and subqueryReuseEnabled are independent of the flag wholePlanReuseEnabled, then `if (conf.exchangeReuseEnabled || conf.subqueryReuseEnabled) {` is saying that if one flag is true then the other is true as well i.e if one flag is true then both exchange and subquery are being reused irrespective of the value set by the user for the other. This could be confusing to the user. Here we are basically enabling both exchangeReuse and subqueryReuse as along as wholePlanReuseEnabled is set to true. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
