Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/18050 )
Change subject: [WIP] IMPALA-10992 Planner changes for estimate peak memory - v2 ...................................................................... Patch Set 11: (48 comments) Regarding moving all of the copy constructor logic into the clone functions, the concern is 2-fold. First if there is existing use of a copy constructor and the semantics changed. Second, the cloning semantics make assumptions about what needs to be deep copied and what does not which may not be meaningful in a different context. http://gerrit.cloudera.org:8080/#/c/18050/11/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/18050/11/common/thrift/ImpalaService.thrift@710 PS11, Line 710: // Indicates whether to relan. typo relan http://gerrit.cloudera.org:8080/#/c/18050/11/common/thrift/Query.thrift File common/thrift/Query.thrift: http://gerrit.cloudera.org:8080/#/c/18050/11/common/thrift/Query.thrift@563 PS11, Line 563: // Indicates whether to enable replan which is a process to generate a suitable clarify: replan is used to generate a new distributed plan for a different size executor group. http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java File fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java@118 PS11, Line 118: public static AggregateInfo clone(AggregateInfo src) { Caller should deal with src==null http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@99 PS11, Line 99: // Deep copy Is deep copy needed for stmt? Explain why in comment if it is. http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/AnalyticWindow.java File fe/src/main/java/org/apache/impala/analysis/AnalyticWindow.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/AnalyticWindow.java@227 PS11, Line 227: return (src == null) ? null : new AnalyticWindow(src); Caller should deal with null src http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/Expr.java@1188 PS11, Line 1188: return (src == null) ? null : src.clone(); Caller should deal with null src http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/Expr.java@1196 PS11, Line 1196: if (l == null) return null; Caller should deal with null src http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/Expr.java@1209 PS11, Line 1209: if (ls == null) return null; Caller should deal with null src http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java File fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@59 PS11, Line 59: return (src == null) ? null : new ExprSubstitutionMap(src); Caller should deal with null src http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java File fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@214 PS11, Line 214: return (src == null) ? null : new MultiAggregateInfo(src); Caller should deal with null src http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/OrderByElement.java File fe/src/main/java/org/apache/impala/analysis/OrderByElement.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/OrderByElement.java@163 PS11, Line 163: List<OrderByElement> result = Lists.newArrayListWithCapacity(l.size()); can you use ArrayList.clone() here? http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java File fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java@106 PS11, Line 106: public SlotDescriptor(SlotDescriptor src) { Move this login into clone() instead of adding a copy constructor. http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java@111 PS11, Line 111: return (src == null) ? null : new SlotDescriptor(src); Caller should deal with null src http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/SortInfo.java File fe/src/main/java/org/apache/impala/analysis/SortInfo.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@176 PS11, Line 176: return (src == null) ? null : new SortInfo(src); Caller should deal with null src http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java File fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java@125 PS11, Line 125: id_ = src.id_; Move this login into clone() instead of adding a copy constructor. http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java@145 PS11, Line 145: return (src == null) ? null : new TupleDescriptor(src); Caller should handle null src http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/common/IdGenerator.java File fe/src/main/java/org/apache/impala/common/IdGenerator.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/common/IdGenerator.java@29 PS11, Line 29: public void setNextVal(int id) { nextId_ = id; } Better to have constructors for this if possible http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/common/TreeNode.java File fe/src/main/java/org/apache/impala/common/TreeNode.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/common/TreeNode.java@54 PS11, Line 54: public <C extends TreeNode<NodeType>> C cloneNode() { return null; } Duplicate signature? http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/AggregationNode.java File fe/src/main/java/org/apache/impala/planner/AggregationNode.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@119 PS11, Line 119: // Deep copy Move to clone body http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java File fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java@111 PS11, Line 111: analyticFnCalls_ = Expr.cloneList(src.analyticFnCalls_); move to clone body http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/CardinalityCheckNode.java File fe/src/main/java/org/apache/impala/planner/CardinalityCheckNode.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/CardinalityCheckNode.java@57 PS11, Line 57: cardinality_ = src.cardinality_; move to clone body http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/DataPartition.java File fe/src/main/java/org/apache/impala/planner/DataPartition.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/DataPartition.java@44 PS11, Line 44: private final static Logger LOG = LoggerFactory.getLogger(DataPartition.class); unused? http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/DataSourceScanNode.java File fe/src/main/java/org/apache/impala/planner/DataSourceScanNode.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/DataSourceScanNode.java@101 PS11, Line 101: desc_ = TupleDescriptor.clone(src.desc_); move to clone body http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/DataSourceScanNode.java@118 PS11, Line 118: if (l == null) { why is there nulls in this list? http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/EmptySetNode.java File fe/src/main/java/org/apache/impala/planner/EmptySetNode.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/EmptySetNode.java@44 PS11, Line 44: super(src); move to clone body http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java File fe/src/main/java/org/apache/impala/planner/ExchangeNode.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java@50 PS11, Line 50: private final static Logger LOG = LoggerFactory.getLogger(ExchangeNode.class); unused? http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java@99 PS11, Line 99: offset_ = src.offset_; move to clone body http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java File fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java@133 PS11, Line 133: keyConjuncts_ = Expr.cloneList(src.keyConjuncts_); move to clone body http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/HashJoinNode.java File fe/src/main/java/org/apache/impala/planner/HashJoinNode.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/HashJoinNode.java@65 PS11, Line 65: return new HashJoinNode(this); move to clone body http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@361 PS11, Line 361: randomReplica_ = src.randomReplica_; move to clone body http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java File fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java@88 PS11, Line 88: icebergPredicates_.addAll(src.icebergPredicates_); move to clone body http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/JoinNode.java File fe/src/main/java/org/apache/impala/planner/JoinNode.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/JoinNode.java@147 PS11, Line 147: joinOp_ = src.joinOp_; move to clone body http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java File fe/src/main/java/org/apache/impala/planner/KuduScanNode.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java@142 PS11, Line 142: useMtScanNode_ = src.useMtScanNode_; move to clone body http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/NestedLoopJoinNode.java File fe/src/main/java/org/apache/impala/planner/NestedLoopJoinNode.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/NestedLoopJoinNode.java@60 PS11, Line 60: super(src); move to clone body http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/PlanFragment.java File fe/src/main/java/org/apache/impala/planner/PlanFragment.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@82 PS11, Line 82: private final static Logger LOG = LoggerFactory.getLogger(PlanFragment.class); unused http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/PlanId.java File fe/src/main/java/org/apache/impala/planner/PlanId.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/PlanId.java@26 PS11, Line 26: private final static Logger LOG = LoggerFactory.getLogger(PlanId.class); unused http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/PlanNode.java File fe/src/main/java/org/apache/impala/planner/PlanNode.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/PlanNode.java@207 PS11, Line 207: this(src.id_, src, src.displayName_); move to clone body http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/Planner.java@157 PS11, Line 157: ctx_ = new PlannerContext(ctxCopy); Use the original on the last iteration http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/Planner.java@162 PS11, Line 162: PlanNode singleNodePlanCopy = singleNodePlan.cloneTree(); Use the orignial on the last iteration http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/PlannerContext.java File fe/src/main/java/org/apache/impala/planner/PlannerContext.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/PlannerContext.java@41 PS11, Line 41: private final static Logger LOG = LoggerFactory.getLogger(PlannerContext.class); unused http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/PlannerContext.java@108 PS11, Line 108: analysisResult_ = AnalysisResult.clone(src.analysisResult_); move to clone method http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/ScanNode.java File fe/src/main/java/org/apache/impala/planner/ScanNode.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/ScanNode.java@90 PS11, Line 90: desc_ = TupleDescriptor.clone(src.desc_); move to clone method http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/SelectNode.java File fe/src/main/java/org/apache/impala/planner/SelectNode.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/SelectNode.java@52 PS11, Line 52: selectivity_ = src.selectivity_; move to clone body http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/SingularRowSrcNode.java File fe/src/main/java/org/apache/impala/planner/SingularRowSrcNode.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/SingularRowSrcNode.java@48 PS11, Line 48: containingSubplanNode_ = (src.containingSubplanNode_ == null) ? move to clone body http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/SortNode.java File fe/src/main/java/org/apache/impala/planner/SortNode.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/SortNode.java@186 PS11, Line 186: info_ = SortInfo.clone(src.info_); move to clone body http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/SubplanNode.java File fe/src/main/java/org/apache/impala/planner/SubplanNode.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/SubplanNode.java@55 PS11, Line 55: subplan_ = (src.subplan_ == null) ? null : src.subplan_.cloneNode(); move to clone body http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/UnionNode.java File fe/src/main/java/org/apache/impala/planner/UnionNode.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/UnionNode.java@99 PS11, Line 99: unionResultExprs_ = Expr.cloneList(src.unionResultExprs_); move to clone body http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/UnnestNode.java File fe/src/main/java/org/apache/impala/planner/UnnestNode.java: http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/UnnestNode.java@69 PS11, Line 69: containingSubplanNode_ = (src.containingSubplanNode_ == null) ? move to clone body -- To view, visit http://gerrit.cloudera.org:8080/18050 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If8a31a574b364f39b049a4bae33a8b98c5fc20bd Gerrit-Change-Number: 18050 Gerrit-PatchSet: 11 Gerrit-Owner: Qifan Chen <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Comment-Date: Wed, 08 Dec 2021 22:02:50 +0000 Gerrit-HasComments: Yes
