[email protected] 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: (26 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: SUBPLAN query: this test > remove Done http://gerrit.cloudera.org:8080/#/c/9065/10/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@572 PS10, Line 572: e es > query Done http://gerrit.cloudera.org:8080/#/c/9065/10/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@573 PS10, Line 573: ARRAY<INT> can get SUBPLAN node : // two children nodes in the planner tree. If the sum of both children > remove, that'll rot. Done http://gerrit.cloudera.org:8080/#/c/9065/10/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@575 PS10, Line 575: cardinality is > Long.MAX_VALUE, then SUBPLAN's estimate cardinality is : // > Long.MAX_VALUE and this tests for the o > couldn't understand that, pls rephrase. Done http://gerrit.cloudera.org:8080/#/c/9065/11/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/11/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@532 PS11, Line 532: 0, Long.MAX_VALUE > I think its clearer what this is doing. Done http://gerrit.cloudera.org:8080/#/c/9065/11/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@572 PS11, Line 572: SUBPLAN query > nit: reverse this to be consistent with the other comments. Done 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@77 PS10, Line 77: Before; > is this used? Yes, it is used at L102 let me put it back as original file shows. http://gerrit.cloudera.org:8080/#/c/9065/10/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@791 PS10, Line 791: ntext(Catalog.DE > is this used any longer? Done http://gerrit.cloudera.org:8080/#/c/9065/10/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@810 PS10, Line 810: > you've created an error message and are 1) tossing it (why?) and 2) using i Done http://gerrit.cloudera.org:8080/#/c/9065/10/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@811 PS10, Line 811: > what's this? Done http://gerrit.cloudera.org:8080/#/c/9065/10/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@830 PS10, Line 830: ong cardinality = node.estimated_stats.cardinality; : if(cardinality < min || cardinality > max){ : StringBuilder errorLog = new StringBuilder(); : errorLog.append("Query: " + query + "\n"); > unclear about this part... I was suggesting a change to the method signatur Done http://gerrit.cloudera.org:8080/#/c/9065/11/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/11/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@783 PS11, Line 783: plans the query, extra > plans the query Done http://gerrit.cloudera.org:8080/#/c/9065/11/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@784 PS11, Line 784: fails if the cardinality is not within the > cardinalities from plan nodes Done http://gerrit.cloudera.org:8080/#/c/9065/11/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@785 PS11, Line 785: are three parameters: query is the request that estimated > fails if the cardinality is not within the specified bounds. Done http://gerrit.cloudera.org:8080/#/c/9065/11/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@786 PS11, Line 786: cardinality is calculated based on and min/max indicates the bounds : * of the cardinality. : */ : pro > stale comment. Done http://gerrit.cloudera.org:8080/#/c/9065/11/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@794 PS11, Line 794: queryCtx.client_request > declare this on L797 Done http://gerrit.cloudera.org:8080/#/c/9065/11/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@799 PS11, Line 799: || execRequest.query_exec_r > fold this into the condition below. Done http://gerrit.cloudera.org:8080/#/c/9065/11/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@815 PS11, Line 815: * For example if two table's number of rows are both > of a plan node Done http://gerrit.cloudera.org:8080/#/c/9065/11/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@818 PS11, Line 818: > . Done http://gerrit.cloudera.org:8080/#/c/9065/11/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@819 PS11, Line 819: * The parameter node is the TPlanNode in the plan tree, and every TPlanNode is > the cardinality should max out at Done http://gerrit.cloudera.org:8080/#/c/9065/11/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@820 PS11, Line 820: d to have estima > we're just testing the cardinality here, not the explain message. pls remov Done http://gerrit.cloudera.org:8080/#/c/9065/11/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@821 PS11, Line 821: The parameter min/max indicates the bounds : * of the cardinality and query is the request that estimated cardinality is : * calculated based on. : */ > stale comment Done http://gerrit.cloudera.org:8080/#/c/9065/11/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@828 PS11, Line 828: fail("Query: " + query + " has no estimated > move under L834 Done http://gerrit.cloudera.org:8080/#/c/9065/11/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@829 PS11, Line 829: > is every node that's passed in here expected to have estimated_fields set? Done http://gerrit.cloudera.org:8080/#/c/9065/11/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@830 PS11, Line 830: node.estimated_stats.cardinality; > inline directly into the "fail". Done http://gerrit.cloudera.org:8080/#/c/9065/11/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@840 PS11, Line 840: > remove. we know its an error at this point. 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: 12 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: Fri, 09 Feb 2018 15:51:24 +0000 Gerrit-HasComments: Yes
