Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/19033 )
Change subject: IMPALA-11604 Planner changes for CPU usage ...................................................................... Patch Set 44: (11 comments) http://gerrit.cloudera.org:8080/#/c/19033/44//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19033/44//COMMIT_MSG@16 PS44, Line 16: blocking-operator nature May need to explain why blocking operators are considered in the context of DoP adjustment. Conceptually, blocking operators can be divided into multiple tiers with those in the 1st are close to the leaf nodes and can run right away. Those in the 2nd tier can run when all their dependent children (including the blocking operators in 1st tier) can provide data. I did not see a linkage to increase DoP for blocking operators. http://gerrit.cloudera.org:8080/#/c/19033/44//COMMIT_MSG@19 PS44, Line 19: explained nit found http://gerrit.cloudera.org:8080/#/c/19033/37/be/src/util/backend-gflag-util.cc File be/src/util/backend-gflag-util.cc: http://gerrit.cloudera.org:8080/#/c/19033/37/be/src/util/backend-gflag-util.cc@201 PS37, Line 201: 0.5 > My intention is to associate the scaling flag to CPU requirement of the que The scaling factor as defined is less intuitive, since one has to inverse it to understand its semantics. I think you can define the true scaling factor to be <n> = 1 / <current query_cpu_requirement_scale>. http://gerrit.cloudera.org:8080/#/c/19033/37/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/19033/37/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@147 PS37, Line 147: have an adjusted number of instance based on : // Processin nit. A positive value implies the instance count has been adjusted. It is also nice to provide an example here. http://gerrit.cloudera.org:8080/#/c/19033/37/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@864 PS37, Line 864: elism, numNode Better renamed as getMaxParallelismByTotalWorkSize(). http://gerrit.cloudera.org:8080/#/c/19033/37/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@883 PS37, Line 883: : protected boolean hasAdjustedInstanceCount() { return adjustedInstanceCount_ > 0; } : : protected void setFixedInstanceCount(int count) { : isFixedParallelism_ = true; : setAdjustedInstanceCount(count); : } Repeated use from line 869. Can be refactored. http://gerrit.cloudera.org:8080/#/c/19033/37/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@957 PS37, Line 957: processingCosts_.get(index).getNumInstanceExpected()); Should add a comment. http://gerrit.cloudera.org:8080/#/c/19033/37/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@972 PS37, Line 972: : // Compute exchanging child parallelism first. I wonder if the computation can be improved here e.g. by the size of the work. Making it to the max # of nodes can overuse the system resource. In general, I wonder if this logic tries to fix some bugs in DoP computation. Adjusting DoP specifically for plans with blocking operators seems odd. See my comment to the commit message. http://gerrit.cloudera.org:8080/#/c/19033/37/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@1098 PS37, Line 1098: if (hasBlockingNode()) { add a comment should be helpful. http://gerrit.cloudera.org:8080/#/c/19033/37/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@1113 PS37, Line 1113: does not sound right. http://gerrit.cloudera.org:8080/#/c/19033/44/testdata/workloads/functional-planner/queries/PlannerTest/tpcds-processing-cost.test File testdata/workloads/functional-planner/queries/PlannerTest/tpcds-processing-cost.test: http://gerrit.cloudera.org:8080/#/c/19033/44/testdata/workloads/functional-planner/queries/PlannerTest/tpcds-processing-cost.test@75 PS44, Line 75: cardinality=3.04K Is it possible to show the new processing cost here too? It will be wonderful. -- To view, visit http://gerrit.cloudera.org:8080/19033 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If32dc770dfffcdd0be2b5555a789a7720952c68a Gerrit-Change-Number: 19033 Gerrit-PatchSet: 44 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: Riza Suminto <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Wed, 08 Feb 2023 15:54:25 +0000 Gerrit-HasComments: Yes
