You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Deepak Jaiswal <dj...@hortonworks.com> on 2017/08/30 03:07:38 UTC
Review Request 61985: HIVE-17399
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61985/
-----------------------------------------------------------
Review request for hive, Gopal V and Jason Dere.
Bugs: HIVE-17399
https://issues.apache.org/jira/browse/HIVE-17399
Repository: hive-git
Description
-------
Do not remove semijoin branch if it feeds to TS->DPP_EVENT
Diffs
-----
ql/src/java/org/apache/hadoop/hive/ql/parse/SemiJoinBranchInfo.java 5d7b9e5c6d
ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java 1671773d4a
ql/src/test/queries/clientpositive/dynamic_semijoin_reduction.q b22890bc9d
ql/src/test/results/clientpositive/llap/dynamic_semijoin_reduction.q.out 3bd35bf2d8
Diff: https://reviews.apache.org/r/61985/diff/1/
Testing
-------
Thanks,
Deepak Jaiswal
Re: Review Request 61985: HIVE-17399
Posted by Deepak Jaiswal <dj...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61985/
-----------------------------------------------------------
(Updated Aug. 31, 2017, 7:26 a.m.)
Review request for hive, Gopal V and Jason Dere.
Changes
-------
Fixed the failing test.
Use try catch block to handle NPE.
Return after assert.
Bugs: HIVE-17399
https://issues.apache.org/jira/browse/HIVE-17399
Repository: hive-git
Description
-------
Do not remove semijoin branch if it feeds to TS->DPP_EVENT
Diffs (updated)
-----
ql/src/java/org/apache/hadoop/hive/ql/parse/SemiJoinBranchInfo.java 5d7b9e5c6d
ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java 1671773d4a
ql/src/test/queries/clientpositive/dynamic_semijoin_reduction.q b22890bc9d
ql/src/test/results/clientpositive/llap/dynamic_semijoin_reduction.q.out 3bd35bf2d8
Diff: https://reviews.apache.org/r/61985/diff/3/
Changes: https://reviews.apache.org/r/61985/diff/2-3/
Testing
-------
Thanks,
Deepak Jaiswal
Re: Review Request 61985: HIVE-17399
Posted by Deepak Jaiswal <dj...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61985/
-----------------------------------------------------------
(Updated Aug. 30, 2017, 5:58 a.m.)
Review request for hive, Gopal V and Jason Dere.
Changes
-------
Implemented the review suggestions except for converting to Boolean Object. The usecase is pretty basic here.
Bugs: HIVE-17399
https://issues.apache.org/jira/browse/HIVE-17399
Repository: hive-git
Description
-------
Do not remove semijoin branch if it feeds to TS->DPP_EVENT
Diffs (updated)
-----
ql/src/java/org/apache/hadoop/hive/ql/parse/SemiJoinBranchInfo.java 5d7b9e5c6d
ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java 1671773d4a
ql/src/test/queries/clientpositive/dynamic_semijoin_reduction.q b22890bc9d
ql/src/test/results/clientpositive/llap/dynamic_semijoin_reduction.q.out 3bd35bf2d8
Diff: https://reviews.apache.org/r/61985/diff/2/
Changes: https://reviews.apache.org/r/61985/diff/1-2/
Testing
-------
Thanks,
Deepak Jaiswal
Re: Review Request 61985: HIVE-17399
Posted by Deepak Jaiswal <dj...@hortonworks.com>.
> On Aug. 30, 2017, 3:35 a.m., Gopal V wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/SemiJoinBranchInfo.java
> > Lines 30 (patched)
> > <https://reviews.apache.org/r/61985/diff/1/?file=1807523#file1807523line30>
> >
> > I prefer a Boolean object, which goes from null -> false/true, so that you can encode (not-set, true, false) in one field.
>
> Deepak Jaiswal wrote:
> Will do that.
In this case, we have very straightforward states so keeping it as it is.
- Deepak
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61985/#review184120
-----------------------------------------------------------
On Aug. 30, 2017, 5:58 a.m., Deepak Jaiswal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61985/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2017, 5:58 a.m.)
>
>
> Review request for hive, Gopal V and Jason Dere.
>
>
> Bugs: HIVE-17399
> https://issues.apache.org/jira/browse/HIVE-17399
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Do not remove semijoin branch if it feeds to TS->DPP_EVENT
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/parse/SemiJoinBranchInfo.java 5d7b9e5c6d
> ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java 1671773d4a
> ql/src/test/queries/clientpositive/dynamic_semijoin_reduction.q b22890bc9d
> ql/src/test/results/clientpositive/llap/dynamic_semijoin_reduction.q.out 3bd35bf2d8
>
>
> Diff: https://reviews.apache.org/r/61985/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Deepak Jaiswal
>
>
Re: Review Request 61985: HIVE-17399
Posted by Deepak Jaiswal <dj...@hortonworks.com>.
> On Aug. 30, 2017, 3:35 a.m., Gopal V wrote:
> >
Thanks for quick turnaround. Working on the comments.
> On Aug. 30, 2017, 3:35 a.m., Gopal V wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/SemiJoinBranchInfo.java
> > Lines 30 (patched)
> > <https://reviews.apache.org/r/61985/diff/1/?file=1807523#file1807523line30>
> >
> > I prefer a Boolean object, which goes from null -> false/true, so that you can encode (not-set, true, false) in one field.
Will do that.
> On Aug. 30, 2017, 3:35 a.m., Gopal V wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/SemiJoinBranchInfo.java
> > Lines 57 (patched)
> > <https://reviews.apache.org/r/61985/diff/1/?file=1807523#file1807523line57>
> >
> > Add a Preconditions checkState that prevents this from going from true -> false.
Makes sense.
> On Aug. 30, 2017, 3:35 a.m., Gopal V wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java
> > Lines 1371 (patched)
> > <https://reviews.apache.org/r/61985/diff/1/?file=1807524#file1807524line1371>
> >
> > NPE issues possible?
It can't happen. However, I can put this in a try catch block and assert if it happens.
> On Aug. 30, 2017, 3:35 a.m., Gopal V wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java
> > Lines 1404 (patched)
> > <https://reviews.apache.org/r/61985/diff/1/?file=1807524#file1807524line1404>
> >
> > Is that a break or a continue?
> >
> > If this is a break out of the loop, then what happens to the rest of the items in dequeue?
Yes, it is a break. For a given TS, there is only one DPP Event. No need to process the rest of the graph.
> On Aug. 30, 2017, 3:35 a.m., Gopal V wrote:
> > ql/src/test/queries/clientpositive/dynamic_semijoin_reduction.q
> > Lines 115 (patched)
> > <https://reviews.apache.org/r/61985/diff/1/?file=1807525#file1807525line115>
> >
> > Pick a query with a non-zero result, so that we can see when it has false negatives (i.e loses rows it is meant to have?).
Sure.
- Deepak
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61985/#review184120
-----------------------------------------------------------
On Aug. 30, 2017, 3:07 a.m., Deepak Jaiswal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61985/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2017, 3:07 a.m.)
>
>
> Review request for hive, Gopal V and Jason Dere.
>
>
> Bugs: HIVE-17399
> https://issues.apache.org/jira/browse/HIVE-17399
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Do not remove semijoin branch if it feeds to TS->DPP_EVENT
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/parse/SemiJoinBranchInfo.java 5d7b9e5c6d
> ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java 1671773d4a
> ql/src/test/queries/clientpositive/dynamic_semijoin_reduction.q b22890bc9d
> ql/src/test/results/clientpositive/llap/dynamic_semijoin_reduction.q.out 3bd35bf2d8
>
>
> Diff: https://reviews.apache.org/r/61985/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Deepak Jaiswal
>
>
Re: Review Request 61985: HIVE-17399
Posted by Gopal V <go...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61985/#review184120
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/parse/SemiJoinBranchInfo.java
Lines 30 (patched)
<https://reviews.apache.org/r/61985/#comment260177>
I prefer a Boolean object, which goes from null -> false/true, so that you can encode (not-set, true, false) in one field.
ql/src/java/org/apache/hadoop/hive/ql/parse/SemiJoinBranchInfo.java
Lines 57 (patched)
<https://reviews.apache.org/r/61985/#comment260175>
Add a Preconditions checkState that prevents this from going from true -> false.
ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java
Lines 1371 (patched)
<https://reviews.apache.org/r/61985/#comment260178>
NPE issues possible?
ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java
Lines 1404 (patched)
<https://reviews.apache.org/r/61985/#comment260176>
Is that a break or a continue?
If this is a break out of the loop, then what happens to the rest of the items in dequeue?
ql/src/test/queries/clientpositive/dynamic_semijoin_reduction.q
Lines 115 (patched)
<https://reviews.apache.org/r/61985/#comment260179>
Pick a query with a non-zero result, so that we can see when it has false negatives (i.e loses rows it is meant to have?).
- Gopal V
On Aug. 30, 2017, 3:07 a.m., Deepak Jaiswal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61985/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2017, 3:07 a.m.)
>
>
> Review request for hive, Gopal V and Jason Dere.
>
>
> Bugs: HIVE-17399
> https://issues.apache.org/jira/browse/HIVE-17399
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Do not remove semijoin branch if it feeds to TS->DPP_EVENT
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/parse/SemiJoinBranchInfo.java 5d7b9e5c6d
> ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java 1671773d4a
> ql/src/test/queries/clientpositive/dynamic_semijoin_reduction.q b22890bc9d
> ql/src/test/results/clientpositive/llap/dynamic_semijoin_reduction.q.out 3bd35bf2d8
>
>
> Diff: https://reviews.apache.org/r/61985/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Deepak Jaiswal
>
>