Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/11440 )
Change subject: IMPALA-7351: Improve memory estimates for Kudu Scan Nodes ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/11440/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11440/1//COMMIT_MSG@14 PS1, Line 14: Modified resource requirements planner test. > I don't see any changes to this test (it seems like I didn't include a Kudu Done http://gerrit.cloudera.org:8080/#/c/11440/1/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/11440/1/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java@91 PS1, Line 91: // Factor capturing the worst-case deviation from a uniform distribution of scan ranges > Put this in the ScanNode base class? Done http://gerrit.cloudera.org:8080/#/c/11440/1/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java@284 PS1, Line 284: // The non-MT scan node requires at least one scanner thread. : int maxScannerThreads; : if (queryOptions.getMt_dop() >= 1) { : maxScannerThreads = 1; : } else { : // TODO: what about heterogeneous hardware, i.e., different num of cores? : maxScannerThreads = Math.min(perHostScanRanges, RuntimeEnv.INSTANCE.getNumCores()); : // Account for the max scanner threads query option. : if (queryOptions.isSetNum_scanner_threads() && : queryOptions.getNum_scanner_threads() > 0) { : maxScannerThreads = : Math.min(maxScannerThreads, queryOptions.getNum_scanner_threads()); : } : } > I think this is shared with HdfsScanNode. Can we factor out to a helper fun Done http://gerrit.cloudera.org:8080/#/c/11440/1/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java@298 PS1, Line 298: int num_cols = desc_.getSlots().size(); > Can you leave a comment briefly describing the heuristic and mentioning any Done http://gerrit.cloudera.org:8080/#/c/11440/1/testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test File testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test: PS1: > You might know already, but we don't validate resource requirements for thi Yup, my main intention was to just keep track of how the estimates change over time. If you believe that this might mislead someone to think that we have extra coverage then we can remove these changes or maybe add a comment to the commit message clarifying that estimate changes to this file are not actually checked, but were only made for the purpose of updating the file. -- To view, visit http://gerrit.cloudera.org:8080/11440 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If9bb52530271b0bff91311a67d222a2e9fac1229 Gerrit-Change-Number: 11440 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Fri, 21 Sep 2018 21:40:11 +0000 Gerrit-HasComments: Yes
