Xinran Tinney 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 14: (16 comments) http://gerrit.cloudera.org:8080/#/c/9065/13/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/13/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@509 PS13, Line 509: public void testCardinalityOverflow() throws ImpalaException { > Please clean up / condense the SQL to make it more readable. I pointing out Done http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@510 PS13, Line 510: String tblName = "tpch.cardinality_overflow"; > Would be great to have individual clauses as building blocks to make the CR Done http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@517 PS13, Line 517: + "l_comment STRING" > clause not needed Done http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@518 PS13, Line 518: + ")"; > clause not needed Done http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@519 PS13, Line 519: String tblLocation = "LOCATION " > clause not needed Done http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@521 PS13, Line 521: String tblPropsTemplate = "TBLPROPERTIES('numRows'='%s')"; > For TBLPROPERTIES, we only care about the 'numRows'. All other keys can be Done http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@524 PS13, Line 524: addTestTable(String.format("CREATE EXTERNAL TABLE %s %s %s %s;", > numFiles property not needed Done http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@530 PS13, Line 530: + "tpch.cardinality_overflow b, tpch.cardinality_overflow c"; > extra space at the end Done http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@541 PS13, Line 541: query = "select l_shipmode from tpch.cardinality_overflow " > use unqualified column references where possible: Done http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@550 PS13, Line 550: > fits in previous line? Done http://gerrit.cloudera.org:8080/#/c/9065/13/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/13/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@783 PS13, Line 783: ueryFileParser.parseFile(); : StringBuilder errorLog = new StringBuilder(); : for (TestCase testCase : queryFileParser.getTestCases()) { : actualOutput.append(testCas > easier: Plans 'query' and fails if estimated cardinalities are not within t Done http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@788 PS13, Line 788: try { > Would be great to run this for all existing planner tests list like checkLi Done http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@799 PS13, Line 799: try { > style: indent 2 Done http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@805 PS13, Line 805: errorLog.append("Unable to create output file: " + e.getMessage()); > style: space after if Done http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@805 PS13, Line 805: rLog.append("Unable to create output file: " + e.getMessage()); : } > nit: spacing Done http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@809 PS13, Line 809: if (errorLog.length() != 0) { > * in general a cardinality of -1 means "unknown" and is also valid; I think Done -- 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: 14 Gerrit-Owner: Xinran Tinney <xyutin...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Jim Apple <jbapple-imp...@apache.org> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Reviewer: Xinran Tinney <xyutin...@cloudera.com> Gerrit-Comment-Date: Mon, 12 Feb 2018 17:20:09 +0000 Gerrit-HasComments: Yes
