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 <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: Sat, 10 Feb 2018 01:57:49 +0000
Gerrit-HasComments: Yes

Reply via email to