Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Hive >> mail # dev >> Review Request 15718: HIVE-5614: Subquery support: allow subquery expressions in having clause


Copy link to this message
-
Re: Review Request 15718: HIVE-5614: Subquery support: allow subquery expressions in having clause


> On Nov. 22, 2013, 7:02 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/QBSubQuery.java, line 131
> > <https://reviews.apache.org/r/15718/diff/1/?file=388900#file388900line131>
> >
> >     It will be good to add a comment about various fields in Conjunct class.

done
> On Nov. 22, 2013, 7:02 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/QBSubQuery.java, line 138
> > <https://reviews.apache.org/r/15718/diff/1/?file=388900#file388900line138>
> >
> >     Can this constructor be package protected instead of public ?

done
> On Nov. 22, 2013, 7:02 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/QBSubQuery.java, line 203
> > <https://reviews.apache.org/r/15718/diff/1/?file=388900#file388900line203>
> >
> >     It will be good to add a comment, how behavior of ConjunctAnalyzer changes when forHavingClause = true instead of false.

done
> On Nov. 22, 2013, 7:02 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/QBSubQuery.java, line 298
> > <https://reviews.apache.org/r/15718/diff/1/?file=388900#file388900line298>
> >
> >     Should this exception needs to be propagated up the stack. At the least, we should have LOG.warn() message here.

this is not an error. This is only a check if the expression is in the OuterQuery RR
> On Nov. 22, 2013, 7:02 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java, line 1916
> > <https://reviews.apache.org/r/15718/diff/1/?file=388901#file388901line1916>
> >
> >     It will be good to add a comment here along the lines of
> >      there could be a subq in having clause, if so we need to generate subq plan followed by semi-join.

done
> On Nov. 22, 2013, 7:02 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java, line 1926
> > <https://reviews.apache.org/r/15718/diff/1/?file=388901#file388901line1926>
> >
> >     It will be good to add a comment how this new boolean changes behavior of this method.

done
> On Nov. 22, 2013, 7:02 p.m., Ashutosh Chauhan wrote:
> > ql/src/test/queries/clientpositive/subquery_in_having.q, line 25
> > <https://reviews.apache.org/r/15718/diff/1/?file=388903#file388903line25>
> >
> >     It will be good to add a test which has a subq in both where clause as well as having clause

done
> On Nov. 22, 2013, 7:02 p.m., Ashutosh Chauhan wrote:
> > ql/src/test/queries/clientpositive/subquery_in_having.q, line 63
> > <https://reviews.apache.org/r/15718/diff/1/?file=388903#file388903line63>
> >
> >     Same comment w.r.t map-join on. Also, if we support over clause in subq, it will be good to have a test for that.

done
> On Nov. 22, 2013, 7:02 p.m., Ashutosh Chauhan wrote:
> > ql/src/test/queries/clientpositive/subquery_notexists_having.q, line 46
> > <https://reviews.apache.org/r/15718/diff/1/?file=388904#file388904line46>
> >
> >     It will be good to add a negative test where subq and outer query both uses same table alias. It seems in such cases we may generate incorrect results, so we should disable those.

this is not just for having. Added a jira HIVE-5874 to give a better error for this case.
> On Nov. 22, 2013, 7:02 p.m., Ashutosh Chauhan wrote:
> > ql/src/test/results/clientpositive/subquery_in_having.q.out, line 57
> > <https://reviews.apache.org/r/15718/diff/1/?file=388907#file388907line57>
> >
> >     In this plan, we are first computing outq, then subq and then doing left semi-join on resultset of those two. As we discussed efficient way for this is to push filter conditions in subq to outer query to cut-down the output generated by outq. Though, I am not sure whether its better to do it in optimizer phase via Transformer or right here. Either ways, I think thats an optimization which we can do as a follow-up.

agreed
> On Nov. 22, 2013, 7:02 p.m., Ashutosh Chauhan wrote:
> > ql/src/test/results/clientpositive/subquery_notexists_having.q.out, line 149

(1=1) is added as a placeholder. Yes it should be removed.
- Harish
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15718/#review29298
On Nov. 23, 2013, 12:14 a.m., Harish Butani wrote: