Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/11306 )
Change subject: IMPALA-7351: Improve memory estimates for Hbase Scan Nodes ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/11306/2/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java File fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java: http://gerrit.cloudera.org:8080/#/c/11306/2/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java@534 PS2, Line 534: public > can this be package-private? Let's document that it's only exposed for test Done http://gerrit.cloudera.org:8080/#/c/11306/2/fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java@548 PS2, Line 548: isStatsMissing > This variable name sounds a bit ungrammatical to me. Maybe "statsAreMissing whoops, I was only trying to follow the convention that all boolean vars have names starting with "is..". How about "isMissingStats"? http://gerrit.cloudera.org:8080/#/c/11306/2/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/11306/2/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@630 PS2, Line 630: assertTrue > assertEquals() here and below. We should also consider doing a static impor Done http://gerrit.cloudera.org:8080/#/c/11306/2/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@631 PS2, Line 631: BitUtil.roundUpToPowerOf2(Type.INT.getSlotSize()) * 2 > I think I'd prefer just hardcoding the expected value instead of having sli Done http://gerrit.cloudera.org:8080/#/c/11306/2/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@636 PS2, Line 636: columnList.clear(); > Consider just constructing a new column list with https://google.github.io/ Done http://gerrit.cloudera.org:8080/#/c/11306/2/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@678 PS2, Line 678: } > Also consider adding a test case with many columns, just for completeness. Done -- To view, visit http://gerrit.cloudera.org:8080/11306 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I583545c3f5e454854f111871c5fbc4f108ae4bff Gerrit-Change-Number: 11306 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 17 Sep 2018 23:07:08 +0000 Gerrit-HasComments: Yes
