[GitHub] [doris] morrySnow commented on a diff in pull request #10240: [Refactor](nereids) Abstract interface of statistics framework for new optimizer reuse

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

morrySnow commented on code in PR #10240:
URL: https://github.com/apache/doris/pull/10240#discussion_r900261396


##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/PlaceHolderPlan.java:
##########
@@ -81,4 +85,34 @@ public PlaceHolderPlan withChildren(List<Plan> children) {
         Preconditions.checkArgument(children.size() == 0);
         return new PlaceHolderPlan(logicalProperties);
     }
+
+    @Override
+    public List<? extends PlanStats> getChildrenStats() {
+        return null;

Review Comment:
   I haven't thought about it very carefully, but IMO, throw exception in these interface in PlaceHolderPlan may be better



##########
fe/fe-core/src/main/java/org/apache/doris/planner/PlanNode.java:
##########
@@ -353,6 +327,11 @@ public List<Expr> getConjuncts() {
         return conjuncts;
     }
 
+    @Override
+    public List<? extends PlanStats> getChildrenStats() {
+        return children;

Review Comment:
   why return children here? i think this interface should be used to return children's StatsDeriveResult



##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/ExprId.java:
##########
@@ -17,13 +17,15 @@
 
 package org.apache.doris.nereids.trees.expressions;
 
+import org.apache.doris.common.Id;
+
 import java.util.Objects;
 import java.util.UUID;
 
 /**
  * UUID for Expression in Nereids.
  */
-public class ExprId {
+public class ExprId extends Id<ExprId> {
     private final long id;
     private final UUID jvmId;

Review Comment:
   remove these lines instead with parent class's attribute: id



##########
fe/fe-core/src/main/java/org/apache/doris/statistics/StatsRecursiveDerive.java:
##########
@@ -38,11 +36,11 @@ private static class Inner {
      * This parameter is an input and output parameter,
      * which will store the derivation result of statistical information in the corresponding node
      */
-    public void statsRecursiveDerive(PlanNode node) throws UserException {

Review Comment:
   statsRecursiveDerive's parameter should not change to PlanStats, since Nereids will never use this to do stats Derive



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org