[GitHub] [spark] tanelk commented on a change in pull request #30093: [SPARK-33183][SQL] Fix EliminateSorts bug when removing global sorts

[GitBox <gi...@apache.org>]

tanelk commented on a change in pull request #30093:
URL: https://github.com/apache/spark/pull/30093#discussion_r510383756



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala
##########
@@ -1052,11 +1052,11 @@ object CombineFilters extends Rule[LogicalPlan] with PredicateHelper {
  *    function is order irrelevant
  */
 object EliminateSorts extends Rule[LogicalPlan] {
-  def apply(plan: LogicalPlan): LogicalPlan = plan transform {
+  def apply(plan: LogicalPlan): LogicalPlan = plan transformUp {

Review comment:
       Would something like this work?
   ```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 e41e380539..e17013f7e0 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
   @@ -1054,13 +1054,17 @@ object CombineFilters extends Rule[LogicalPlan] with PredicateHelper {
    object EliminateSorts extends Rule[LogicalPlan] {
      // transformUp is needed here to ensure idempotency of this rule when removing consecutive
      // local sorts.
   -  def apply(plan: LogicalPlan): LogicalPlan = plan transformUp {
   +  def apply(plan: LogicalPlan): LogicalPlan = plan transformDown {
        case s @ Sort(orders, _, child) if orders.isEmpty || orders.exists(_.child.foldable) =>
          val newOrders = orders.filterNot(_.child.foldable)
          if (newOrders.isEmpty) child else s.copy(order = newOrders)
   -    case Sort(orders, false, child) if SortOrder.orderingSatisfies(child.outputOrdering, orders) =>
   -      child
   -    case s @ Sort(_, _, child) => s.copy(child = recursiveRemoveSort(child))
   +    case s @ Sort(orders, _, child) =>
   +      val sortsRemoved = recursiveRemoveSort(child)
   +      if (!s.global && SortOrder.orderingSatisfies(sortsRemoved.outputOrdering, orders)) {
   +        sortsRemoved
   +      } else {
   +        s.copy(child = sortsRemoved)
   +      }
        case j @ Join(originLeft, originRight, _, cond, _) if cond.forall(_.deterministic) =>
          j.copy(left = recursiveRemoveSort(originLeft), right = recursiveRemoveSort(originRight))
        case g @ Aggregate(_, aggs, originChild) if isOrderIrrelevantAggs(aggs) =>
   ```
   
   Not as elegant, but should avoid the extra iterations from `transformUp`




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org