liuyao has posted comments on this change. ( http://gerrit.cloudera.org:8080/16842 )
Change subject: IMPALA-10377: Improve the accuracy of resource estimation ...................................................................... Patch Set 17: (8 comments) http://gerrit.cloudera.org:8080/#/c/16842/16//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16842/16//COMMIT_MSG@8 PS16, Line 8: > nit: Please put a blank line after the title Done http://gerrit.cloudera.org:8080/#/c/16842/16//COMMIT_MSG@23 PS16, Line 23: > nit: just 'data', or 'data rows'? Done http://gerrit.cloudera.org:8080/#/c/16842/16/fe/src/main/java/org/apache/impala/planner/HashJoinNode.java File fe/src/main/java/org/apache/impala/planner/HashJoinNode.java: http://gerrit.cloudera.org:8080/#/c/16842/16/fe/src/main/java/org/apache/impala/planner/HashJoinNode.java@220 PS16, Line 220: rements for the join that > Could you please add a comment to describe the overal calculation? Done http://gerrit.cloudera.org:8080/#/c/16842/16/fe/src/main/java/org/apache/impala/planner/HashJoinNode.java@230 PS16, Line 230: if (getChild(1).getCardinality() == -1 || getChild(1).getAvgRowSize() == -1 : || numInstances <= 0) { : perBuildInstanceMemEstimate = DEFAULT_PER_INSTANCE_MEM; : perBuildInstanceDataBytes = -1; : } else { : long rhsCard = getChild(1).getCardinality(); : long rhsNdv = 1; : // Calculate the ndv of the right child, which is the multiplication of : / > Could you please add some comments for the computation of rhsNdv? Done http://gerrit.cloudera.org:8080/#/c/16842/16/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java File fe/src/test/java/org/apache/impala/planner/CardinalityTest.java: http://gerrit.cloudera.org:8080/#/c/16842/16/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java@838 PS16, Line 838: > nit: probably we should just use MIN_HASH_TBL_MEM here Done http://gerrit.cloudera.org:8080/#/c/16842/16/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java@921 PS16, Line 921: pathToJoinNode, HashJoinNode.class); : // create a single node plan. : verifyApproxMemoryEstimate("SELECT A.ID FROM (SELECT A1.ID,B1.DATE_STRING_COL " + : "IDB FROM FUNCTIONAL.ALLTYPES A1 , FUNCTIONAL.ALLTYPES B1 GROUP BY A1.ID," + : > nit: too many blank lines Done http://gerrit.cloudera.org:8080/#/c/16842/16/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java@1101 PS16, Line 1101: > nit: doc comments start with /** Done http://gerrit.cloudera.org:8080/#/c/16842/16/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java@1143 PS16, Line 1143: de of interest. > Seems like all tests use distributed plans. It'd be good to add a few tests Done -- To view, visit http://gerrit.cloudera.org:8080/16842 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic01db168ff2c6d6de33ee553a8175599f035d7a1 Gerrit-Change-Number: 16842 Gerrit-PatchSet: 17 Gerrit-Owner: liuyao <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: liuyao <[email protected]> Gerrit-Comment-Date: Wed, 24 Feb 2021 08:22:34 +0000 Gerrit-HasComments: Yes
