Alex Behm 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 13: (14 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 specific improvements below. http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@510 PS13, Line 510: String tblStr = "l_orderkey BIGINT, " Would be great to have individual clauses as building blocks to make the CREATE TABLE more readable. Example: String colDefs = "( l_orderkey BIGINT, l_partkey BIGINT, ...)"; String tblPropsTempate = "TBLPROPERTIES('numRows'='%s')"; String tblProps = String.format(tblPropsTemplate, Long.toString(Long.MAX_VALUE)); // Stitch them together addTestTable(String.format("CREATE TABLE ... %s %s", colDefs, tblProps); http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@517 PS13, Line 517: + "ROW FORMAT DELIMITED FIELDS TERMINATED BY ' ' " clause not needed http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@518 PS13, Line 518: + "WITH SERDEPROPERTIES ('field.delim'=' ', 'serialization.format'='|') " clause not needed http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@519 PS13, Line 519: + "STORED AS TEXTFILE " clause not needed http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@521 PS13, Line 521: + "TBLPROPERTIES ('COLUMN_STATS_ACCURATE'='false', " For TBLPROPERTIES, we only care about the 'numRows'. All other keys can be removed. http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@524 PS13, Line 524: + tblStr + "'numFiles'='1', 'numRows'= '" numFiles property not needed 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 http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@541 PS13, Line 541: query = "select tpch.cardinality_overflow.l_shipmode from tpch.cardinality_overflow " use unqualified column references where possible: tpch.cardinality_overflow.l_shipmode -> l_shipmode http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@550 PS13, Line 550: + "< b.l_orderkey"; fits in previous line? 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@788 PS13, Line 788: protected void checkCardinality(String query, long min, long max) Would be great to run this for all existing planner tests list like checkLimitCardinality(). Please move this function under checkLimitCardinality() so the refactoring/combination opportunity is visible (for future changes). http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@799 PS13, Line 799: return; style: indent 2 http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@805 PS13, Line 805: if(node.estimated_stats == null) { style: space after if http://gerrit.cloudera.org:8080/#/c/9065/13/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@809 PS13, Line 809: if(cardinality < min || cardinality > max){ * in general a cardinality of -1 means "unknown" and is also valid; I think it makes most sense to deal with that -1 specially; we should document in the function comment cardinalities between min/max and -1 are valid * style: space after if -- 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: 13 Gerrit-Owner: Xinran Tinney <[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: Xinran Tinney <[email protected]> Gerrit-Comment-Date: Sat, 10 Feb 2018 01:57:49 +0000 Gerrit-HasComments: Yes
