Skip to content

Commit eda6c4b

Browse files
JoshRosenMaxGekk
authored andcommitted
[SPARK-39259][SQL][FOLLOWUP] Fix source and binary incompatibilities in transformDownWithSubqueries
### What changes were proposed in this pull request? This is a followup to #36654. That PR modified the existing `QueryPlan.transformDownWithSubqueries` to add additional arguments for tree pattern pruning. In this PR, I roll back the change to that method's signature and instead add a new `transformDownWithSubqueriesAndPruning` method. ### Why are the changes needed? The original change breaks binary and source compatibility in Catalyst. Technically speaking, Catalyst APIs are considered internal to Spark and are subject to change between minor releases (see [source](https://github.com/apache/spark/blob/bb51add5c79558df863d37965603387d40cc4387/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/package.scala#L20-L24)), but I think it's nice to try to avoid API breakage when possible. While trying to compile some custom Catalyst code, I ran into issues when trying to call the `transformDownWithSubqueries` method without supplying a tree pattern filter condition. If I do `transformDownWithSubqueries() { f} ` then I get a compilation error. I think this is due to the first parameter group containing all default parameters. My PR's solution of adding a new `transformDownWithSubqueriesAndPruning` method solves this problem. It's also more consistent with the naming convention used for other pruning-enabled tree transformation methods. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing tests. Closes #36765 from JoshRosen/SPARK-39259-binary-compatibility-followup. Authored-by: Josh Rosen <[email protected]> Signed-off-by: Max Gekk <[email protected]>
1 parent bb51add commit eda6c4b

File tree

2 files changed

+17
-7
lines changed

2 files changed

+17
-7
lines changed

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/finishAnalysis.scala

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ object ComputeCurrentTime extends Rule[LogicalPlan] {
8484
treePatternbits.containsPattern(CURRENT_LIKE)
8585
}
8686

87-
plan.transformDownWithSubqueries(transformCondition) {
87+
plan.transformDownWithSubqueriesAndPruning(transformCondition) {
8888
case subQuery =>
8989
subQuery.transformAllExpressionsWithPruning(transformCondition) {
9090
case cd: CurrentDate =>

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -454,7 +454,7 @@ abstract class QueryPlan[PlanType <: QueryPlan[PlanType]]
454454
* to rewrite the whole plan, include its subqueries, in one go.
455455
*/
456456
def transformWithSubqueries(f: PartialFunction[PlanType, PlanType]): PlanType =
457-
transformDownWithSubqueries(AlwaysProcess.fn, UnknownRuleId)(f)
457+
transformDownWithSubqueries(f)
458458

459459
/**
460460
* Returns a copy of this node where the given partial function has been recursively applied
@@ -479,18 +479,28 @@ abstract class QueryPlan[PlanType <: QueryPlan[PlanType]]
479479
* first to this node, then this node's subqueries and finally this node's children.
480480
* When the partial function does not apply to a given node, it is left unchanged.
481481
*/
482-
def transformDownWithSubqueries(
483-
cond: TreePatternBits => Boolean = AlwaysProcess.fn, ruleId: RuleId = UnknownRuleId)
484-
(f: PartialFunction[PlanType, PlanType])
485-
: PlanType = {
482+
def transformDownWithSubqueries(f: PartialFunction[PlanType, PlanType]): PlanType = {
483+
transformDownWithSubqueriesAndPruning(AlwaysProcess.fn, UnknownRuleId)(f)
484+
}
485+
486+
/**
487+
* This method is the top-down (pre-order) counterpart of transformUpWithSubqueries.
488+
* Returns a copy of this node where the given partial function has been recursively applied
489+
* first to this node, then this node's subqueries and finally this node's children.
490+
* When the partial function does not apply to a given node, it is left unchanged.
491+
*/
492+
def transformDownWithSubqueriesAndPruning(
493+
cond: TreePatternBits => Boolean,
494+
ruleId: RuleId = UnknownRuleId)
495+
(f: PartialFunction[PlanType, PlanType]): PlanType = {
486496
val g: PartialFunction[PlanType, PlanType] = new PartialFunction[PlanType, PlanType] {
487497
override def isDefinedAt(x: PlanType): Boolean = true
488498

489499
override def apply(plan: PlanType): PlanType = {
490500
val transformed = f.applyOrElse[PlanType, PlanType](plan, identity)
491501
transformed transformExpressionsDown {
492502
case planExpression: PlanExpression[PlanType] =>
493-
val newPlan = planExpression.plan.transformDownWithSubqueries(cond, ruleId)(f)
503+
val newPlan = planExpression.plan.transformDownWithSubqueriesAndPruning(cond, ruleId)(f)
494504
planExpression.withNewPlan(newPlan)
495505
}
496506
}

0 commit comments

Comments
 (0)