tanelk commented on pull request #31907: URL: https://github.com/apache/spark/pull/31907#issuecomment-803521361
@wangyum, I'll answer your question (https://github.com/apache/spark/pull/31677#pullrequestreview-616975279) here. Just to clarify - the #31677 does not fix the issue, you are trying to fix here. But they are very similar. I improved the `CollapseWindow`, the `TransposeWindow` can be improved in a similar way to fix your issue. I tried a quick change: ```diff diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala index 3e3550d5da..e629ccc268 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala @@ -991,13 +991,24 @@ object TransposeWindow extends Rule[LogicalPlan] { }) } + private def windowsCompatible(w1: Window, w2: Window): Boolean = { + w1.references.intersect(w2.windowOutputSet).isEmpty && + w1.expressions.forall(_.deterministic) && + w2.expressions.forall(_.deterministic) && + compatiblePartitions(w1.partitionSpec, w2.partitionSpec) + } + def apply(plan: LogicalPlan): LogicalPlan = plan transformUp { - case w1 @ Window(we1, ps1, os1, w2 @ Window(we2, ps2, os2, grandChild)) - if w1.references.intersect(w2.windowOutputSet).isEmpty && - w1.expressions.forall(_.deterministic) && - w2.expressions.forall(_.deterministic) && - compatiblePartitions(ps1, ps2) => - Project(w1.output, Window(we2, ps2, os2, Window(we1, ps1, os1, grandChild))) + case w1 @ Window(_, _, _, w2 @ Window(_, _, _, grandChild)) + if windowsCompatible(w1, w2) => + Project(w1.output, w2.copy(child = w1.copy(child = grandChild))) + + case w1 @ Window(_, _, _, Project(pl, w2 @ Window(_, _, _, grandChild))) + if windowsCompatible(w1, w2) && w1.references.subsetOf(grandChild.outputSet) => + Project( + pl ++ w1.windowOutputSet, + w2.copy(child = w1.copy(child = grandChild)) + ) } } ``` And it changes the TPC-DS q47 and TPC-DS q57 in the same way your PR does, but I find this change to be more robust. -- 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]
