Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9065 )
Change subject: IMPALA-5440 Add planner tests with extreme statistics values ...................................................................... Patch Set 4: (11 comments) http://gerrit.cloudera.org:8080/#/c/9065/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9065/4//COMMIT_MSG@8 PS4, Line 8: the JIRA references multiple issues-- this change addresses some of the issues (not all). pls state what part of the JIRA is addressed. http://gerrit.cloudera.org:8080/#/c/9065/4/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/9065/4/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@38 PS4, Line 38: ecInfo; : import org.apache.impala.thrift.TPlanFragment; : import org.apache.impala.thrift.TPlanNode; pls check if these additional imports are needed. might've been needed before moving helper methods to the other file. http://gerrit.cloudera.org:8080/#/c/9065/4/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@522 PS4, Line 522: /C add a space. also, for these comments, pls explain why you expect an overflow. http://gerrit.cloudera.org:8080/#/c/9065/4/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@523 PS4, Line 523: rowNum = Lo perhaps just inline this in the create table string. http://gerrit.cloudera.org:8080/#/c/9065/4/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@556 PS4, Line 556: er_ca wrap long lines. http://gerrit.cloudera.org:8080/#/c/9065/4/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@560 PS4, Line 560: _addr here and several more places below. http://gerrit.cloudera.org:8080/#/c/9065/4/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@568 PS4, Line 568: create what are you testing for? http://gerrit.cloudera.org:8080/#/c/9065/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java File fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java: http://gerrit.cloudera.org:8080/#/c/9065/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@791 PS4, Line 791: @param query : * the testing query : * @param tablename : * the table's name that the query is queried on : * @throws ImpalaException the code base does not include javadocs for params, return, etc. pls remove for consistency. http://gerrit.cloudera.org:8080/#/c/9065/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@828 PS4, Line 828: @param query : * the testing query : * @param tablename : * the table's name that the query is queried on : * @return : * returns the request in TExecRequest format : * @throws ImpalaException same here, pls remove. the content is fine, just not using javadoc style. http://gerrit.cloudera.org:8080/#/c/9065/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@850 PS4, Line 850: @par and here http://gerrit.cloudera.org:8080/#/c/9065/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@853 PS4, Line 853: setupStats perhaps just inline this in the test. it has little to do with stats at this point. -- To view, visit http://gerrit.cloudera.org:8080/9065 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I86dec47cf1438882cafaec53e97864ccfcdff3cb Gerrit-Change-Number: 9065 Gerrit-PatchSet: 4 Gerrit-Owner: [email protected] Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Comment-Date: Mon, 29 Jan 2018 23:19:14 +0000 Gerrit-HasComments: Yes
