xyutin...@cloudera.com 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: 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 15:51:24 +0000
Gerrit-HasComments: Yes

Reply via email to