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 54: (8 comments) Looks great! Thanks a lot for the changes, some of them are significant. http://gerrit.cloudera.org:8080/#/c/19033/54//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19033/54//COMMIT_MSG@295 PS54, Line 295: . Need to mention the default value. Maybe set it to MT_DOP? Should we also mention PROCESSING_COST_MAX_THREADS which can be default to max # of cores? http://gerrit.cloudera.org:8080/#/c/19033/54//COMMIT_MSG@328 PS54, Line 328: Q3 : CoreCount={total=12 trace=F00:12} : : Q12 : CoreCount={total=12 trace=F00:12} : : Q15 : CoreCount={total=15 trace=N07:3+F00:12} indentation http://gerrit.cloudera.org:8080/#/c/19033/54/fe/src/main/java/org/apache/impala/planner/BaseProcessingCost.java File fe/src/main/java/org/apache/impala/planner/BaseProcessingCost.java: http://gerrit.cloudera.org:8080/#/c/19033/54/fe/src/main/java/org/apache/impala/planner/BaseProcessingCost.java@30 PS54, Line 30: long cardinality, float exprsCost, float materializationCost) { I think we should factor in the row width in computing PC, as row width can vary a lot. Without considering the width, the computed PC may not be right. PC = cardinality * width * (expr cost + materialization cost). http://gerrit.cloudera.org:8080/#/c/19033/54/fe/src/main/java/org/apache/impala/planner/CoreCount.java File fe/src/main/java/org/apache/impala/planner/CoreCount.java: http://gerrit.cloudera.org:8080/#/c/19033/54/fe/src/main/java/org/apache/impala/planner/CoreCount.java@31 PS54, Line 31: requirement CPU cores, computed from the CPU cost, http://gerrit.cloudera.org:8080/#/c/19033/52/fe/src/main/java/org/apache/impala/planner/ProcessingCost.java File fe/src/main/java/org/apache/impala/planner/ProcessingCost.java: http://gerrit.cloudera.org:8080/#/c/19033/52/fe/src/main/java/org/apache/impala/planner/ProcessingCost.java@270 PS52, Line 270: > I think '>" is the correct sign here? Not sure I follow. Maybe an example? http://gerrit.cloudera.org:8080/#/c/19033/52/fe/src/main/java/org/apache/impala/planner/SegmentCost.java File fe/src/main/java/org/apache/impala/planner/SegmentCost.java: http://gerrit.cloudera.org:8080/#/c/19033/52/fe/src/main/java/org/apache/impala/planner/SegmentCost.java@78 PS52, Line 78: > When I first work on this, I intentionally made ProcessingCost to not accep Yeah a revisit will help. It sounds like we need to deal with the unknown cost (-1 in cardinality) as well. Maybe we do not adjust in such situations? http://gerrit.cloudera.org:8080/#/c/19033/52/fe/src/main/java/org/apache/impala/planner/SegmentCost.java@102 PS52, Line 102: > I decide to remove traverseBlockingAwareCost() method. I see. Just wonder if the algorithm can be modified to not check the state of the children. In other words, each child can supply an expected core count which is available when the parent node is being processed. http://gerrit.cloudera.org:8080/#/c/19033/52/fe/src/main/java/org/apache/impala/planner/SegmentCost.java@118 PS52, Line 118: > In traverseBlockingAwareCores(), I changed the loop to iterate all children Sounds like a conservative strategy. Done. -- 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: 54 Gerrit-Owner: Qifan Chen <qfc...@hotmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qfc...@hotmail.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Fri, 03 Mar 2023 14:54:41 +0000 Gerrit-HasComments: Yes
