From 3ce9e7cfca4a03543d62fc468f4cce92c198ffb5 Mon Sep 17 00:00:00 2001
From: jakevin <30525741+jackwener@users.noreply.github.com>
Date: Thu, 7 Jul 2022 22:32:07 +0800
Subject: [PATCH] [enhance](planner): remove redundant field in sort (#10624)

SortInfo is in SortNode. But there are some replicated field in SortNode

Issue Number: close #10616

Remove the redundant field in `TSortNode` which exist in `TSortInfo`.

[API-BREAK] This has changed `Thrift` file.
---
 .../org/apache/doris/analysis/SortInfo.java   | 16 ++++++++++++++
 .../translator/PhysicalPlanTranslator.java    |  1 -
 .../org/apache/doris/planner/SortNode.java    | 22 +++++--------------
 gensrc/thrift/PlanNodes.thrift                |  9 --------
 4 files changed, 21 insertions(+), 27 deletions(-)

diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/SortInfo.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/SortInfo.java
index 3e0703414b..05763e8d07 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/analysis/SortInfo.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/SortInfo.java
@@ -21,6 +21,7 @@
 package org.apache.doris.analysis;
 
 import org.apache.doris.common.TreeNode;
+import org.apache.doris.thrift.TSortInfo;
 
 import com.google.common.base.Preconditions;
 import com.google.common.base.Predicates;
@@ -136,6 +137,10 @@ public class SortInfo {
         return sortTupleSlotExprs;
     }
 
+    public void setSortTupleSlotExprs(List<Expr> sortTupleSlotExprs) {
+        this.sortTupleSlotExprs = sortTupleSlotExprs;
+    }
+
     public TupleDescriptor getSortTupleDescriptor() {
         return sortTupleDesc;
     }
@@ -273,4 +278,15 @@ public class SortInfo {
         }
         return substOrderBy;
     }
+
+    /**
+     * Convert the sort info to TSortInfo.
+     */
+    public TSortInfo toThrift() {
+        TSortInfo sortInfo = new TSortInfo(
+                Expr.treesToThrift(orderingExprs),
+                isAscOrder,
+                nullsFirstParams);
+        return sortInfo;
+    }
 }
diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java
index da69c993fa..98a697db74 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java
@@ -229,7 +229,6 @@ public class PhysicalPlanTranslator extends PlanOperatorVisitor<PlanFragment, Pl
         // TODO: notice topN
         SortNode sortNode = new SortNode(context.nextNodeId(), childNode, sortInfo, true,
                 physicalHeapSort.hasLimit(), physicalHeapSort.getOffset());
-        exec(sortNode::init);
         childFragment.addPlanRoot(sortNode);
         if (!childFragment.isPartitioned()) {
             return childFragment;
diff --git a/fe/fe-core/src/main/java/org/apache/doris/planner/SortNode.java b/fe/fe-core/src/main/java/org/apache/doris/planner/SortNode.java
index d55fa5002f..db08f4eb48 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/planner/SortNode.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/planner/SortNode.java
@@ -179,10 +179,6 @@ public class SortNode extends PlanNode {
         LOG.debug("stats Sort: cardinality=" + Long.toString(cardinality));
     }
 
-    public void init() throws UserException {
-        this.resolvedTupleExprs = Lists.newArrayList();
-    }
-
     public void init(Analyzer analyzer) throws UserException {
         // Compute the memory layout for the generated tuple.
         computeStats(analyzer);
@@ -234,24 +230,16 @@ public class SortNode extends PlanNode {
     @Override
     protected void toThrift(TPlanNode msg) {
         msg.node_type = TPlanNodeType.SORT_NODE;
-        TSortInfo sortInfo = new TSortInfo(
-                Expr.treesToThrift(info.getOrderingExprs()),
-                info.getIsAscOrder(),
-                info.getNullsFirst());
+
+        TSortInfo sortInfo = info.toThrift();
         Preconditions.checkState(tupleIds.size() == 1, "Incorrect size for tupleIds in SortNode");
-        sortInfo.setSortTupleSlotExprs(Expr.treesToThrift(resolvedTupleExprs));
+        if (resolvedTupleExprs != null) {
+            sortInfo.setSortTupleSlotExprs(Expr.treesToThrift(resolvedTupleExprs));
+        }
         TSortNode sortNode = new TSortNode(sortInfo, useTopN);
 
         msg.sort_node = sortNode;
         msg.sort_node.setOffset(offset);
-
-        // TODO(lingbin): remove blew codes, because it is duplicate with TSortInfo
-        msg.sort_node.setOrderingExprs(Expr.treesToThrift(info.getOrderingExprs()));
-        msg.sort_node.setIsAscOrder(info.getIsAscOrder());
-        msg.sort_node.setNullsFirst(info.getNullsFirst());
-        if (info.getSortTupleSlotExprs() != null) {
-            msg.sort_node.setSortTupleSlotExprs(Expr.treesToThrift(info.getSortTupleSlotExprs()));
-        }
     }
 
     @Override
diff --git a/gensrc/thrift/PlanNodes.thrift b/gensrc/thrift/PlanNodes.thrift
index 1b627f4879..dec3ca2f64 100644
--- a/gensrc/thrift/PlanNodes.thrift
+++ b/gensrc/thrift/PlanNodes.thrift
@@ -573,17 +573,8 @@ struct TSortNode {
   // This is the number of rows to skip before returning results
   3: optional i64 offset
 
-  // TODO(lingbin): remove blew, because duplaicate with TSortInfo
-  4: optional list<Exprs.TExpr> ordering_exprs                                   
-  5: optional list<bool> is_asc_order                                            
   // Indicates whether the imposed limit comes DEFAULT_ORDER_BY_LIMIT.           
   6: optional bool is_default_limit                                              
-  // Indicates, for each expr, if nulls should be listed first or last. This is  
-  // independent of is_asc_order.                                                
-  7: optional list<bool> nulls_first                                             
-  // Expressions evaluated over the input row that materialize the tuple to be so
-  // Contains one expr per slot in the materialized tuple.                       
-  8: optional list<Exprs.TExpr> sort_tuple_slot_exprs                            
 }
 
 enum TAnalyticWindowType {