Re: Review Request 61985: HIVE-17399

[Deepak Jaiswal]

---
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

[Deepak Jaiswal]

> 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)
> > 
> >
> > 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

[Deepak Jaiswal]

---
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

[Deepak Jaiswal]

> 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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

[Gopal V]

---
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)


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)


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)


NPE issues possible?



ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java
Lines 1404 (patched)


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)


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
> 
>

Review Request 61985: HIVE-17399

[Deepak Jaiswal]

---
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