Mailing-List: contact dev-help@hive.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hive.apache.org
Content-Type: multipart/alternative;
 boundary="===============6118282584854520432=="
MIME-Version: 1.0
Subject: Re: Review Request 59984: Improve plans for subqueries with non-equi
 co-related predicates
From: Vineet Garg <vgarg@hortonworks.com>
To: Ashutosh Chauhan <hashutosh@apache.org>
Cc: hive <dev@hive.apache.org>, Vineet Garg <vgarg@hortonworks.com>
Date: Wed, 21 Jun 2017 23:20:35 -0000
Message-ID: <20170621232035.39411.55311@reviews-vm2.apache.org>
Auto-Submitted: auto-generated
Sender: Vineet Garg <noreply@reviews.apache.org>
References: <20170620001733.43050.88875@reviews-vm2.apache.org>
In-Reply-To: <20170620001733.43050.88875@reviews-vm2.apache.org>
Reply-To: Vineet Garg <vgarg@hortonworks.com>
archived-at: Wed, 21 Jun 2017 23:20:47 -0000

--===============6118282584854520432==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


> On June 20, 2017, 12:17 a.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveAggregate.java
> > Lines 137-139 (patched)
> > <https://reviews.apache.org/r/59984/diff/1/?file=1747770#file1747770line137>
> >
> >     Seems like this if branch is unnecessary.
> 
> Vineet Garg wrote:
>     I don't think so, most of the calcite code/ hive rules implement RelShuttle interface. So accept should reroute accordingly
> 
> Ashutosh Chauhan wrote:
>     Its unnecessary in sense that even without this branch logic of this function doesn't change.

I am not sure if I understand this. If we remove 'if' branch and just have 'return shuttle.visit(this)' wouldn't this  call RelShuttle's version 'visit(relnode)' instead of HiveRelShuttle's visit(HiveAggregate) ?


> On June 20, 2017, 12:17 a.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelDecorrelator.java
> > Lines 3088 (patched)
> > <https://reviews.apache.org/r/59984/diff/1/?file=1747773#file1747773line3109>
> >
> >     Might be better to initialize this variable to true. Since, value being false is known and limited set.
> 
> Vineet Garg wrote:
>     I think this works better for readability. e.g.  Aggregate looks for grouping sets and if it finds one sets the flag to true and return, otherwise it traveses all its children. Changing the intial flag to true will entail negating this logic and will anyway keep all the code/cases.
> 
> Ashutosh Chauhan wrote:
>     Initialization with false is more robust since that implies by default valueGen is required. Thats a good thing since we know all cases when its not required, but not necessarily otherwise. e.g., if there are more visit methods added in this interface in future and this implementation is not updated, initialization with false will save us from bugs like those.

That's a good point. I'll update the code.


- Vineet


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


On June 11, 2017, 7:17 p.m., Vineet Garg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59984/
> -----------------------------------------------------------
> 
> (Updated June 11, 2017, 7:17 p.m.)
> 
> 
> Review request for hive and Ashutosh Chauhan.
> 
> 
> Bugs: HIVE-16838
>     https://issues.apache.org/jira/browse/HIVE-16838
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> This patch improves plans for subqueries which have not equal corelated predicates. 
> Currently to retrieve all possible correlated predicates inner table is joined with outer query. This is un-necessary in most of the cases (exception is if subquery has an aggregate).
> 
> 
> Diffs
> -----
> 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveAggregate.java 63bbdaccfb 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveIntersect.java 19e1e026f4 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveUnion.java 7cfb007a9d 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelDecorrelator.java 4c99932759 
>   ql/src/test/queries/clientpositive/subquery_in.q 4ba170a706 
>   ql/src/test/results/clientpositive/constprog_partitioner.q.out 8c7f9d3f29 
>   ql/src/test/results/clientpositive/llap/explainuser_1.q.out 8b04bc9261 
>   ql/src/test/results/clientpositive/llap/subquery_exists.q.out 3004e36c9d 
>   ql/src/test/results/clientpositive/llap/subquery_in.q.out 1f9c9e4474 
>   ql/src/test/results/clientpositive/llap/subquery_multi.q.out 29516eff82 
>   ql/src/test/results/clientpositive/llap/subquery_notin.q.out b4af91579b 
>   ql/src/test/results/clientpositive/llap/subquery_scalar.q.out b78df8b9f5 
>   ql/src/test/results/clientpositive/llap/subquery_select.q.out 202980e975 
>   ql/src/test/results/clientpositive/llap/subquery_views.q.out 1a21a02a30 
>   ql/src/test/results/clientpositive/llap/vector_mapjoin_reduce.q.out d3586e0db2 
>   ql/src/test/results/clientpositive/perf/query16.q.out a7f93f9ec2 
>   ql/src/test/results/clientpositive/perf/query94.q.out c5fc9e7f00 
>   ql/src/test/results/clientpositive/spark/constprog_partitioner.q.out 3467215d63 
>   ql/src/test/results/clientpositive/spark/subquery_exists.q.out 8768b45166 
>   ql/src/test/results/clientpositive/spark/subquery_in.q.out 80a350656d 
>   ql/src/test/results/clientpositive/spark/vector_mapjoin_reduce.q.out 2f2609f03e 
>   ql/src/test/results/clientpositive/subquery_exists.q.out cfc76520ce 
>   ql/src/test/results/clientpositive/subquery_exists_having.q.out 2c41ff6c33 
>   ql/src/test/results/clientpositive/subquery_in_having.q.out c4569ba035 
>   ql/src/test/results/clientpositive/subquery_notexists.q.out 039df03819 
>   ql/src/test/results/clientpositive/subquery_notexists_having.q.out fda801d387 
>   ql/src/test/results/clientpositive/subquery_notin_having.q.out 462dda5e14 
>   ql/src/test/results/clientpositive/subquery_unqualcolumnrefs.q.out 03eb4b6ba4 
>   ql/src/test/results/clientpositive/vector_mapjoin_reduce.q.out 82bef24230 
> 
> 
> Diff: https://reviews.apache.org/r/59984/diff/1/
> 
> 
> Testing
> -------
> 
> * Added new tests
> * Pre-commit testing
> 
> 
> Thanks,
> 
> Vineet Garg
> 
>


--===============6118282584854520432==--