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 10: (10 comments) http://gerrit.cloudera.org:8080/#/c/9065/10/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/10/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@572 PS10, Line 572: node query http://gerrit.cloudera.org:8080/#/c/9065/10/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@572 PS10, Line 572: Query that can generate remove http://gerrit.cloudera.org:8080/#/c/9065/10/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@573 PS10, Line 573: .java L88 : // checkedMultiply(getChild(0).cardinality_, getChild(1).cardinality_) remove, that'll rot. http://gerrit.cloudera.org:8080/#/c/9065/10/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@575 PS10, Line 575: the nested complex type e.g. int_array ARRAY<INT> can get SUBPLAN node : // two children: NESTED LOOP JOIN and UNNEST couldn't understand that, pls rephrase. http://gerrit.cloudera.org:8080/#/c/9065/10/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/10/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@39 PS10, Line 39: Catalog is this used? http://gerrit.cloudera.org:8080/#/c/9065/10/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@77 PS10, Line 77: BeforeClass is this used? http://gerrit.cloudera.org:8080/#/c/9065/10/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@791 PS10, Line 791: String tablename is this used any longer? http://gerrit.cloudera.org:8080/#/c/9065/10/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@810 PS10, Line 810: .length() you've created an error message and are 1) tossing it (why?) and 2) using it indirectly to test something else. http://gerrit.cloudera.org:8080/#/c/9065/10/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@811 PS10, Line 811: errorLog what's this? http://gerrit.cloudera.org:8080/#/c/9065/10/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@830 PS10, Line 830: The lower/upper bounds of the : * estimated cardinality should be [0, Long.MAX_VALUE], e.g. the test for overflow : * would be [Long.MAX_VALUE, Long.MAX_VALUE], the test for zero cardinality would be : * be [0,0] and the test for a non-negative Long would be [0, Long.MAX_VAL unclear about this part... I was suggesting a change to the method signature so that instead of "zeroCardinality", we have expectedLower, expectedUpper args (or similar such naming). not sure what this part of the comment is trying to convey. if this is not making things easier/cleaner, feel free to ignore. -- 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: 10 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: Tue, 06 Feb 2018 23:07:58 +0000 Gerrit-HasComments: Yes
