You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@hive.apache.org by Ashutosh Chauhan <ha...@apache.org> on 2016/10/07 02:57:17 UTC

Re: Review Request 51755: Support Intersect Distinct

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51755/#review148788
-----------------------------------------------------------




ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelFactories.java (line 209)
<https://reviews.apache.org/r/51755/#comment216301>

    Error message: Union or intersect



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveIntersect.java (line 30)
<https://reviews.apache.org/r/51755/#comment220241>

    whitespace



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectMergeRule.java (line 31)
<https://reviews.apache.org/r/51755/#comment216303>

    It will be good to add a comment describing transformation we are trying to achieve here. Better will be in terms of sql level transform, if possible.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectMergeRule.java (line 59)
<https://reviews.apache.org/r/51755/#comment216304>

    Is this check not required on bottomIntersect?



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectMergeRule.java (line 65)
<https://reviews.apache.org/r/51755/#comment216306>

    This assert should be before if()



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 64)
<https://reviews.apache.org/r/51755/#comment220242>

    Good to add more details on how this rule is transforming operator tree.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 102)
<https://reviews.apache.org/r/51755/#comment220243>

    Will be useful to explain why Project is needed here.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 107)
<https://reviews.apache.org/r/51755/#comment220244>

    Remove TODO



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 108)
<https://reviews.apache.org/r/51755/#comment220245>

    rethrow after logging.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 148)
<https://reviews.apache.org/r/51755/#comment220246>

    Add comments on why we need to have project here which is not doing any transformations.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 150)
<https://reviews.apache.org/r/51755/#comment220247>

    TODO



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 151)
<https://reviews.apache.org/r/51755/#comment220248>

    log and rethrow.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 180)
<https://reviews.apache.org/r/51755/#comment220249>

    It is a deterministic function.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 189)
<https://reviews.apache.org/r/51755/#comment220250>

    TODO



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 190)
<https://reviews.apache.org/r/51755/#comment220251>

    log and rethrow.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 212)
<https://reviews.apache.org/r/51755/#comment220252>

    remove TODO



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 213)
<https://reviews.apache.org/r/51755/#comment220253>

    log and rethrow.



ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java (line 2090)
<https://reviews.apache.org/r/51755/#comment220254>

    ws



ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g (line 93)
<https://reviews.apache.org/r/51755/#comment220255>

    Good to call this TOK_EXCEPTALL since KW is except not minus.



ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g (line 94)
<https://reviews.apache.org/r/51755/#comment220258>

    TOK_EXCEPTDISTINCT



ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java (line 516)
<https://reviews.apache.org/r/51755/#comment220256>

    insideView not needed anymore?



ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java (line 523)
<https://reviews.apache.org/r/51755/#comment220257>

    insideView not needed anymore?



ql/src/test/queries/clientpositive/intersect.q (line 40)
<https://reviews.apache.org/r/51755/#comment220259>

    Add a test where there is a GBy on different columns with different aggregates on two branches of intersect.


- Ashutosh Chauhan


On Sept. 9, 2016, 1:41 a.m., pengcheng xiong wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51755/
> -----------------------------------------------------------
> 
> (Updated Sept. 9, 2016, 1:41 a.m.)
> 
> 
> Review request for hive and Ashutosh Chauhan.
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> HIVE-12765
> 
> 
> Diffs
> -----
> 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelFactories.java cf93ed8 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveIntersect.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectMergeRule.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ASTConverter.java 9f5e733 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java ff94160 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/HiveLexer.g 7ceb005 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g df596ff 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g 9ba1865 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/QBExpr.java cccf0f6 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 943d9d7 
>   ql/src/test/queries/clientpositive/intersect.q PRE-CREATION 
>   ql/src/test/results/clientpositive/intersect.q.out PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51755/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> pengcheng xiong
> 
>

Re: Review Request 51755: Support Intersect Distinct

Posted by pengcheng xiong <px...@hortonworks.com>.


> On Oct. 7, 2016, 2:57 a.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectMergeRule.java, line 59
> > <https://reviews.apache.org/r/51755/diff/1/?file=1494884#file1494884line59>
> >
> >     Is this check not required on bottomIntersect?

We need this. if top is distinct, we can always merge whether bottom is distinct or not. if top is all, we can only merge if bottom is also all. that is to say, we should bail out if top is all and bottom is distinct. (I made some modifications.)


> On Oct. 7, 2016, 2:57 a.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g, line 93
> > <https://reviews.apache.org/r/51755/diff/1/?file=1494889#file1494889line93>
> >
> >     Good to call this TOK_EXCEPTALL since KW is except not minus.

We have both of them... actually both except and minus are kws. anyway, im going to change it to except.


- pengcheng


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51755/#review148788
-----------------------------------------------------------


On Sept. 9, 2016, 1:41 a.m., pengcheng xiong wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51755/
> -----------------------------------------------------------
> 
> (Updated Sept. 9, 2016, 1:41 a.m.)
> 
> 
> Review request for hive and Ashutosh Chauhan.
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> HIVE-12765
> 
> 
> Diffs
> -----
> 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelFactories.java cf93ed8 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveIntersect.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectMergeRule.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ASTConverter.java 9f5e733 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java ff94160 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/HiveLexer.g 7ceb005 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g df596ff 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g 9ba1865 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/QBExpr.java cccf0f6 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 943d9d7 
>   ql/src/test/queries/clientpositive/intersect.q PRE-CREATION 
>   ql/src/test/results/clientpositive/intersect.q.out PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51755/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> pengcheng xiong
> 
>

Re: Review Request 51755: Support Intersect Distinct

Posted by Ashutosh Chauhan <ha...@apache.org>.


> On Oct. 7, 2016, 2:57 a.m., Ashutosh Chauhan wrote:
> >

Took first pass look. You may wait to address these comments till I finish full review.


- Ashutosh


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51755/#review148788
-----------------------------------------------------------


On Oct. 15, 2016, 4:10 a.m., pengcheng xiong wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51755/
> -----------------------------------------------------------
> 
> (Updated Oct. 15, 2016, 4:10 a.m.)
> 
> 
> Review request for hive and Ashutosh Chauhan.
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> HIVE-12765
> 
> 
> Diffs
> -----
> 
>   itests/src/test/resources/testconfiguration.properties 97e310d 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/SimpleFetchOptimizer.java eb0ba7b 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveCalciteUtil.java c527e58 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelFactories.java cf93ed8 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveExcept.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveIntersect.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveExceptRewriteRule.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectMergeRule.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveSortLimitPullUpConstantsRule.java cc318db 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ASTConverter.java 8d738aa 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java e6ab947 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/HiveLexer.g 5d3fa6a 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g 5c16c55 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g 50987c3 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/QBExpr.java cccf0f6 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 747f387 
>   ql/src/test/org/apache/hadoop/hive/ql/parse/TestSQL11ReservedKeyWordsNegative.java a427803 
>   ql/src/test/queries/clientpositive/except_all.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/except_distinct.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/intersect_all.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/intersect_distinct.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/intersect_merge.q PRE-CREATION 
>   ql/src/test/results/clientpositive/auto_join8.q.out ccbafba 
>   ql/src/test/results/clientpositive/except_all.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/except_distinct.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/input25.q.out 465173d 
>   ql/src/test/results/clientpositive/intersect_all.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/intersect_distinct.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/join8.q.out 47b821a 
>   ql/src/test/results/clientpositive/list_bucket_query_oneskew_2.q.out 45f2481 
>   ql/src/test/results/clientpositive/smb_mapjoin_25.q.out aba899e 
>   ql/src/test/results/clientpositive/spark/auto_join8.q.out 6909113 
>   ql/src/test/results/clientpositive/spark/join8.q.out 4903c90 
>   ql/src/test/results/clientpositive/spark/smb_mapjoin_25.q.out eacb438 
>   ql/src/test/results/clientpositive/spark/union_top_level.q.out e1c7fc7 
>   ql/src/test/results/clientpositive/tez/intersect_merge.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/timestamp.q.out 7c08ec8 
>   ql/src/test/results/clientpositive/vector_null_projection.q.out 779f787 
> 
> Diff: https://reviews.apache.org/r/51755/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> pengcheng xiong
> 
>