Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19033 )
Change subject: IMPALA-11604 Planner changes for CPU usage ...................................................................... Patch Set 52: (15 comments) http://gerrit.cloudera.org:8080/#/c/19033/52/be/src/util/backend-gflag-util.cc File be/src/util/backend-gflag-util.cc: http://gerrit.cloudera.org:8080/#/c/19033/52/be/src/util/backend-gflag-util.cc@200 PS52, Line 200: query_cpu_count_divisor, 1.0 Is it possible to add test cases with divisor as 0.5 and 2.0? http://gerrit.cloudera.org:8080/#/c/19033/52/fe/src/main/java/org/apache/impala/planner/BroadcastProcessingCost.java File fe/src/main/java/org/apache/impala/planner/BroadcastProcessingCost.java: http://gerrit.cloudera.org:8080/#/c/19033/52/fe/src/main/java/org/apache/impala/planner/BroadcastProcessingCost.java@31 PS52, Line 31: Multiple supplier don't see multiplier in this class http://gerrit.cloudera.org:8080/#/c/19033/52/fe/src/main/java/org/apache/impala/planner/BroadcastProcessingCost.java@62 PS52, Line 62: numInstanceSupplier_ call getNumInstanceExpected()? http://gerrit.cloudera.org:8080/#/c/19033/52/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/52/fe/src/main/java/org/apache/impala/planner/CoreCount.java@58 PS52, Line 58: ids_ = ids; Add Preconditions.check to make sure two lists have same length http://gerrit.cloudera.org:8080/#/c/19033/52/fe/src/main/java/org/apache/impala/planner/DataSink.java File fe/src/main/java/org/apache/impala/planner/DataSink.java: http://gerrit.cloudera.org:8080/#/c/19033/52/fe/src/main/java/org/apache/impala/planner/DataSink.java@75 PS52, Line 75: && explainLevel.ordinal() >= TExplainLevel.VERBOSE.ordinal()) { : // Print processing cost. : output.append(processingCost_.getExplainString(detailPrefix, false)); can be moved after line #67 http://gerrit.cloudera.org:8080/#/c/19033/52/fe/src/main/java/org/apache/impala/planner/DataSink.java@147 PS52, Line 147: fragment_.getPlanRoot().getCardinality() can be replaced with getNumRowsProduced()? Numbers of rows consumed and produced are same? http://gerrit.cloudera.org:8080/#/c/19033/52/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/19033/52/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java@245 PS52, Line 245: SerDe too short, hard to read http://gerrit.cloudera.org:8080/#/c/19033/52/fe/src/main/java/org/apache/impala/planner/MultipleProcessingCost.java File fe/src/main/java/org/apache/impala/planner/MultipleProcessingCost.java: http://gerrit.cloudera.org:8080/#/c/19033/52/fe/src/main/java/org/apache/impala/planner/MultipleProcessingCost.java@24 PS52, Line 24: MultipleProcessingCost > nit. Maybe ScaledProcessingCost? MultipleProcessingCost sounds like a list Or MultiplyProcessingCost? http://gerrit.cloudera.org:8080/#/c/19033/52/fe/src/main/java/org/apache/impala/planner/MultipleProcessingCost.java@28 PS52, Line 28: multiple nit: multiplier? 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@39 PS52, Line 39: numInstances add Preconditions.check for numInstance greater than 0 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@72 PS52, Line 72: nodes_.size() if nodes_size() equal 0? http://gerrit.cloudera.org:8080/#/c/19033/52/fe/src/main/java/org/apache/impala/planner/SegmentCost.java@83 PS52, Line 83: appendSinkCost > appendSink() or setSink()? http://gerrit.cloudera.org:8080/#/c/19033/52/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/19033/52/fe/src/main/java/org/apache/impala/planner/UnionNode.java@155 PS52, Line 155: cost nit: costs http://gerrit.cloudera.org:8080/#/c/19033/52/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/19033/52/fe/src/main/java/org/apache/impala/service/Frontend.java@273 PS52, Line 273: Certain queries such as EXPLAIN that do not populate : // TExecRequest.query_exec_request field nit: sentence seems not complete. set value as -1? http://gerrit.cloudera.org:8080/#/c/19033/52/fe/src/test/java/org/apache/impala/planner/PlannerTest.java File fe/src/test/java/org/apache/impala/planner/PlannerTest.java: http://gerrit.cloudera.org:8080/#/c/19033/52/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@1378 PS52, Line 1378: } could you add test case with processing_cost_allow_thread_increment as non default value? -- 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: 52 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: Mon, 27 Feb 2023 23:50:40 +0000 Gerrit-HasComments: Yes
