Qifan Chen 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 22:
(45 comments)
Addressed all review comments and fixed the following.
P25:
select count(distinct int_col) from functional.alltypes group by year;
Error Stack:
java.lang.NullPointerException
at org.apache.impala.analysis.Expr.removeDuplicates(Expr.java:1252)
<== resolved by copy substGroupingExprs_ in
MultiAggregateInfo’s copy cstr
at
org.apache.impala.analysis.Analyzer.setsHaveValueTransfer(Analyzer.java:2705)
at
org.apache.impala.planner.DistributedPlanner.createPhase2DistinctAggregationFragment(DistributedPlanner.java:996)
at
org.apache.impala.planner.DistributedPlanner.createAggregationFragment(DistributedPlanner.java:889)
at
org.apache.impala.planner.DistributedPlanner.createPlanFragments(DistributedPlanner.java:132)
at
org.apache.impala.planner.DistributedPlanner.createPlanFragments(DistributedPlanner.java:84)
at org.apache.impala.planner.Planner.createPlanFragments(Planner.java:165)
at org.apache.impala.planner.Planner.createPlans(Planner.java:303)
at org.apache.impala.service.Frontend.createExecRequest(Frontend.java:1562)
at
org.apache.impala.service.Frontend.getPlannedExecRequest(Frontend.java:1939)
at org.apache.impala.service.Frontend.doCreateExecRequest(Frontend.java:1779)
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 replan.
> typo relan
Done
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
Done
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:
> Caller should deal with src==null
Since this is a static function, we can use it to check the null as oppose to
in all the calling places.
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: public AnalysisResult(AnalysisResult src, CloneContext c) {
> Is deep copy needed for stmt? Explain why in comment if it is.
Can't shadow copy and document the reason.
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: public AnalyticWindow(AnalyticWindow other, CloneContext c) {
> Caller should deal with null src
Same reason: being a static method, it is possible to handle
null in a single location here.
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
Same reason: being a static method, it is possible to handle
null in a single location here.
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
Same reason: being a static method, it is possible to handle
null in a single location here.
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
Same reason: being a static method, it is possible to handle
null in a single location here.
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:
> Caller should deal with null src
Same reason: being a static method, it is possible to handle
null in a single location here.
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: CloneContext.AssertAutoScaling(c);
> Caller should deal with null src
Same reason: being a static method, it is possible to handle
null in a single location here.
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: OrderByElement reverseElement =
> can you use ArrayList.clone() here?
ArrayList.clone() does not clone the content, which is needed here. Also follow
the same pattern for Expr.clone() where each element is cloned.
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:
> Move this login into clone() instead of adding a copy constructor.
Decide to keep the new copy constructor that takes an extra argument
CloneContext and keep the old copy constructor (if any) the same. In this way,
it is easy to call among copy constructors.
The clone() method also takes the CloneContext argument.
http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java@111
PS11, Line 111: public static SlotDescriptor clone(SlotDescriptor src,
CloneContext c) {
> Caller should deal with null src
Same reason: being a static method, it is possible to handle
null in a single location here.
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 abstract IdType getMaxId();
> Better to have constructors for this if possible
Removed setNextVal() and keep getNextVal() which provides a way to get the
current nextId_ value without calling getNextId(). If getNextId() were called,
we need another method to set it back by 1 in the source generator.
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:
> Duplicate signature?
The method names are different.
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: super(src, c);
> Move to clone body
Decide to keep the new copy constructor that takes an extra argument
CloneContext and keep the old copy constructor (if any) the same. In this way,
it is easy to call among copy constructors.
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: // Deep copy
> move to clone body
Decide to keep the new copy constructor that takes an extra argument
CloneContext and keep the old copy constructor (if any) the same. In this way,
it is easy to call among copy constructors.
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: // deep copy
> move to clone body
Decide to keep the new copy constructor that takes an extra argument
CloneContext and keep the old copy constructor (if any) the same. In this way,
it is easy to call among copy constructors.
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: // Always non-null.
> unused?
Done
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: // Deep copy
> move to clone body
Decide to keep the new copy constructor that takes an extra argument
CloneContext and keep the old copy constructor (if any) the same. In this way,
it is easy to call among copy constructors.
http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/DataSourceScanNode.java@118
PS11, Line 118: for (List<TBinaryPredicate> l : ls) {
> why is there nulls in this list?
Fixed by removing the IF block.
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: public EmptySetNode(EmptySetNode src, CloneContext c) {
> move to clone body
Decide to keep the new copy constructor that takes an extra argument
CloneContext and keep the old copy constructor (if any) the same. In this way,
it is easy to call among copy constructors.
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: public class ExchangeNode extends PlanNode {
> unused?
Done
http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java@99
PS11, Line 99: mergeInfo_ = SortInfo.clone(src.mergeInfo_, c);
> move to clone body
Decide to keep the new copy constructor that takes an extra argument
CloneContext and keep the old copy constructor (if any) the same. In this way,
it is easy to call among copy constructors.
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: // Deep copy
> move to clone body
Decide to keep the new copy constructor that takes an extra argument
CloneContext and keep the old copy constructor (if any) the same. In this way,
it is easy to call among copy constructors.
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: public HashJoinNode cloneNode(CloneContext c) {
> move to clone body
Decide to keep the new copy constructor that takes an extra argument
CloneContext and keep the old copy constructor (if any) the same. In this way,
it is easy to call among copy constructors.
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: // Deep copy
> move to clone body
Decide to keep the new copy constructor that takes an extra argument
CloneContext and keep the old copy constructor (if any) the same. In this way,
it is easy to call among copy constructors.
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: // Deep copy
> move to clone body
Decide to keep the new copy constructor that takes an extra argument
CloneContext and keep the old copy constructor (if any) the same. In this way,
it is easy to call among copy constructors.
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: // Deep copy
> move to clone body
Decide to keep the new copy constructor that takes an extra argument
CloneContext and keep the old copy constructor (if any) the same. In this way,
it is easy to call among copy constructors.
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: // Deep copy
> move to clone body
Decide to keep the new copy constructor that takes an extra argument
CloneContext and keep the old copy constructor (if any) the same. In this way,
it is easy to call among copy constructors.
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: public NestedLoopJoinNode(NestedLoopJoinNode src, CloneContext
c) {
> move to clone body
Decide to keep the new copy constructor that takes an extra argument
CloneContext and keep the old copy constructor (if any) the same. In this way,
it is easy to call among copy constructors.
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 PlanFragmentId fragmentId_;
> unused
Done
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: // Construction only allowed via an IdGenerator.
> unused
Done
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: cardinality_ = -1;
> move to clone body
Decide to keep the new copy constructor that takes an extra argument
CloneContext and keep the old copy constructor (if any) the same. In this way,
it is easy to call among copy constructors.
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: if (ctx_.getQueryOptions().isEnable_replan()) {
> Use the original on the last iteration
Done
http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/Planner.java@162
PS11, Line 162:
> Use the orignial on the last iteration
Need to revisit as making use of the original one in last iteration does not
work well.
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: public class PlannerContext {
> unused
Done
http://gerrit.cloudera.org:8080/#/c/18050/11/fe/src/main/java/org/apache/impala/planner/PlannerContext.java@108
PS11, Line 108: queryStmt_ = (src.queryStmt_ == null) ? null :
src.queryStmt_.clone(c);
> move to clone method
Decide to keep the new copy constructor that takes an extra argument
CloneContext and keep the old copy constructor (if any) the same. In this way,
it is easy to call among copy constructors.
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: // Deep copy
> move to clone method
Decide to keep the new copy constructor that takes an extra argument
CloneContext and keep the old copy constructor (if any) the same. In this way,
it is easy to call among copy constructors.
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: // Deep copy
> move to clone body
Decide to keep the new copy constructor that takes an extra argument
CloneContext and keep the old copy constructor (if any) the same. In this way,
it is easy to call among copy constructors.
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: // Deep copy
> move to clone body
Decide to keep the new copy constructor that takes an extra argument
CloneContext and keep the old copy constructor (if any) the same. In this way,
it is easy to call among copy constructors.
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: // Deep copy
> move to clone body
Decide to keep the new copy constructor that takes an extra argument
CloneContext and keep the old copy constructor (if any) the same. In this way,
it is easy to call among copy constructors.
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: // Deep copy
> move to clone body
Decide to keep the new copy constructor that takes an extra argument
CloneContext and keep the old copy constructor (if any) the same. In this way,
it is easy to call among copy constructors.
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: // Deep copy
> move to clone body
Decide to keep the new copy constructor that takes an extra argument
CloneContext and keep the old copy constructor (if any) the same. In this way,
it is easy to call among copy constructors.
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: // Deep copy
> move to clone body
Decide to keep the new copy constructor that takes an extra argument
CloneContext and keep the old copy constructor (if any) the same. In this way,
it is easy to call among copy constructors.
--
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: 22
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-Reviewer: Wenzhe Zhou <[email protected]>
Gerrit-Comment-Date: Fri, 17 Dec 2021 18:03:34 +0000
Gerrit-HasComments: Yes
