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 12: (10 comments) couple of proposals for simplifying code/comments. otherwise, looks fine. http://gerrit.cloudera.org:8080/#/c/9065/12/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/12/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@527 PS12, Line 527: two nit: you have three tables in the query. http://gerrit.cloudera.org:8080/#/c/9065/12/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@527 PS12, Line 527: when the two tables both have Long.MAX_VALUE as : // the row number, the CROSS JOIN should have Long.MAX_VALUE*Long.MAX_VALUE rows : // and this tests for the cardinality estimate overflow perhaps the following template is easier? if you think its better, pls use it for the others. "tests that multiplying the input cardinalities does not overflow the cross-join's estimated cardinality." you can replace "cross-join " and "multiply" with add where applicable. http://gerrit.cloudera.org:8080/#/c/9065/12/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@550 PS12, Line 550: // row number, the JOIN (especially unequal JOIN) should have >Long.MAX_VALUE rows nit: space http://gerrit.cloudera.org:8080/#/c/9065/12/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@564 PS12, Line 564: rows nit: row count http://gerrit.cloudera.org:8080/#/c/9065/12/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/12/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@783 PS12, Line 783: the query replace with: 'query' (to refer to the parameter) http://gerrit.cloudera.org:8080/#/c/9065/12/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@784 PS12, Line 784: * fails if the cardinality is not within the specified bounds. add: ['min', 'max'] http://gerrit.cloudera.org:8080/#/c/9065/12/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@785 PS12, Line 785: There are three parameters: query is the request that estimated : * cardinality is calculated based on and min/max indicates the bounds : * of the cardinality. remove this and inline it in the decscription above. http://gerrit.cloudera.org:8080/#/c/9065/12/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@793 PS12, Line 793: TExecRequest execRequest move the declaration and assignment to L796 (no need to split this over two lines). http://gerrit.cloudera.org:8080/#/c/9065/12/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@798 PS12, Line 798: execRequest == null || from my reading of createExecRequest, this will never be null, so remove this check. http://gerrit.cloudera.org:8080/#/c/9065/12/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@825 PS12, Line 825: protected void checkCardinalityBounds( after the simplifications, I think it makes sense to inline this function at L807. at this point, its complicating this more than needed (extra repeated comment, awkward parameters) for a basic bounds check and error message. -- 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: 12 Gerrit-Owner: 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: xyutin...@cloudera.com Gerrit-Comment-Date: Fri, 09 Feb 2018 17:22:16 +0000 Gerrit-HasComments: Yes
