Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/20024 )
Change subject: IMPALA-12192: Fix scaling bug in scan fragment ...................................................................... Patch Set 7: (8 comments) http://gerrit.cloudera.org:8080/#/c/20024/7/fe/src/main/java/org/apache/impala/planner/CostingSegment.java File fe/src/main/java/org/apache/impala/planner/CostingSegment.java: http://gerrit.cloudera.org:8080/#/c/20024/7/fe/src/main/java/org/apache/impala/planner/CostingSegment.java@194 PS7, Line 194: Preconditions.checkState(newParallelism >= minParallelism, This seems always true now with the local max() bounding. http://gerrit.cloudera.org:8080/#/c/20024/6/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/20024/6/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@1085 PS6, Line 1085: Preconditions.checkState(child.hasAdjustedInstanceCount()); rename maxScanRangesPerNode http://gerrit.cloudera.org:8080/#/c/20024/7/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/20024/7/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@150 PS7, Line 150: private int maxParallelismForExplainString_ = -1; Not clear what the ForExplainString is about http://gerrit.cloudera.org:8080/#/c/20024/7/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@1093 PS7, Line 1093: // union fargment, but it should still capped at costBasedMaxParallelism. nit: be capped http://gerrit.cloudera.org:8080/#/c/20024/7/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@1100 PS7, Line 1100: (int) Math.min(costBasedMaxParallelism, largestScanRange); Might be better to round up. http://gerrit.cloudera.org:8080/#/c/20024/7/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@1140 PS7, Line 1140: maxParallelism = costBasedMaxParallelism; Seems strange that maxParallelismForExplainString_does not always follow maxParallelism. http://gerrit.cloudera.org:8080/#/c/20024/7/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/20024/7/fe/src/main/java/org/apache/impala/planner/ScanNode.java@357 PS7, Line 357: int maxScannerThreads = Move this into the else if block below. http://gerrit.cloudera.org:8080/#/c/20024/7/fe/src/main/java/org/apache/impala/planner/ScanNode.java@403 PS7, Line 403: * (float) BackendConfig.INSTANCE.getScanRangeCostFactor() / getInputCardinality() getScanRangeCostFactor returning double whereas these functons multiply and return float -- To view, visit http://gerrit.cloudera.org:8080/20024 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7010f6c3bc48ae3f74e8db98a83f645b6c157226 Gerrit-Change-Number: 20024 Gerrit-PatchSet: 7 Gerrit-Owner: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Wed, 21 Jun 2023 15:08:55 +0000 Gerrit-HasComments: Yes
