Re: Review Request 58914: HIVE-16550

2017-05-02 Thread Deepak Jaiswal 

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

(Updated May 3, 2017, 1:16 a.m.)


Review request for hive, Jason Dere and Sergey Shelukhin.


Changes
---

Worked on Jason's comments and added some more tests.


Bugs: HIVE-16550
https://issues.apache.org/jira/browse/HIVE-16550


Repository: hive-git


Description
---

Semijoin Hints should be able to skip the optimization if needed.
In addition to that, the patch fixes several issues with hints in general such 
as,
- It now works with subqueries and unions.
- Uses a global data structure instead of per QB.


Diffs (updated)
-

  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/DynamicPartitionPruningOptimization.java
 e1a69526bc 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/HiveOpConverter.java
 40c0f3ba2a 
  ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java 1b054a7e24 
  ql/src/java/org/apache/hadoop/hive/ql/parse/HintParser.g e110fb33df 
  ql/src/java/org/apache/hadoop/hive/ql/parse/ParseContext.java 3a1f821bd3 
  ql/src/java/org/apache/hadoop/hive/ql/parse/ParseUtils.java 54e37f7c80 
  ql/src/java/org/apache/hadoop/hive/ql/parse/QBParseInfo.java 7bf1c599a5 
  ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java cbbb7d0c94 
  ql/src/java/org/apache/hadoop/hive/ql/parse/TaskCompiler.java 5ea7800528 
  ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeDynamicListDesc.java 
3143554ec6 
  ql/src/java/org/apache/hadoop/hive/ql/plan/JoinDesc.java 7d4267d6a8 
  ql/src/java/org/apache/hadoop/hive/ql/ppd/SyntheticJoinPredicate.java 
f45daa8828 
  ql/src/test/queries/clientpositive/semijoin_hint.q 5de0c8c8c1 
  ql/src/test/results/clientpositive/llap/semijoin_hint.q.out bc248930ec 


Diff: https://reviews.apache.org/r/58914/diff/3/

Changes: https://reviews.apache.org/r/58914/diff/2-3/


Testing
---


Thanks,

Deepak Jaiswal

Re: Review Request 58914: HIVE-16550

2017-05-02 Thread Deepak Jaiswal 

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

(Updated May 2, 2017, 9:01 p.m.)


Review request for hive, Jason Dere and Sergey Shelukhin.


Changes
---

Implemented review comments.
Added the special case for "None" to skip the runtime filtering.


Bugs: HIVE-16550
https://issues.apache.org/jira/browse/HIVE-16550


Repository: hive-git


Description
---

Semijoin Hints should be able to skip the optimization if needed.
In addition to that, the patch fixes several issues with hints in general such 
as,
- It now works with subqueries and unions.
- Uses a global data structure instead of per QB.


Diffs (updated)
-

  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/DynamicPartitionPruningOptimization.java
 e1a69526bc 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/HiveOpConverter.java
 40c0f3ba2a 
  ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java 1b054a7e24 
  ql/src/java/org/apache/hadoop/hive/ql/parse/HintParser.g e110fb33df 
  ql/src/java/org/apache/hadoop/hive/ql/parse/ParseContext.java 3a1f821bd3 
  ql/src/java/org/apache/hadoop/hive/ql/parse/ParseUtils.java 54e37f7c80 
  ql/src/java/org/apache/hadoop/hive/ql/parse/QBParseInfo.java 7bf1c599a5 
  ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java cbbb7d0c94 
  ql/src/java/org/apache/hadoop/hive/ql/parse/TaskCompiler.java 5ea7800528 
  ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeDynamicListDesc.java 
3143554ec6 
  ql/src/java/org/apache/hadoop/hive/ql/plan/JoinDesc.java 7d4267d6a8 
  ql/src/java/org/apache/hadoop/hive/ql/ppd/SyntheticJoinPredicate.java 
f45daa8828 
  ql/src/test/queries/clientpositive/semijoin_hint.q 5de0c8c8c1 
  ql/src/test/results/clientpositive/llap/semijoin_hint.q.out bc248930ec 


Diff: https://reviews.apache.org/r/58914/diff/2/

Changes: https://reviews.apache.org/r/58914/diff/1-2/


Testing
---


Thanks,

Deepak Jaiswal

Re: Review Request 58914: HIVE-16550

2017-05-02 Thread Deepak Jaiswal 


> On May 2, 2017, 8:15 p.m., Jason Dere wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java
> > Lines 11192 (patched)
> > 
> >
> > Can you give the method name something different than OldHints - how 
> > about getHintsFromQB?

sure.


> On May 2, 2017, 8:15 p.m., Jason Dere wrote:
> > ql/src/test/queries/clientpositive/semijoin_hint.q
> > Lines 46 (patched)
> > 
> >
> > Does this query, with no hints, have any runtime filtering? Otherwise 
> > you wouldn't be able to distinguish whether the None option works or not.

Yes it does.


> On May 2, 2017, 8:15 p.m., Jason Dere wrote:
> > ql/src/test/results/clientpositive/llap/semijoin_hint.q.out
> > Lines 233 (patched)
> > 
> >
> > Should the expectedEntries be 5000 per the hint?

Thanks for brining this up. It is happening incase of return path true. 
Investigating.


> On May 2, 2017, 8:15 p.m., Jason Dere wrote:
> > ql/src/test/results/clientpositive/llap/semijoin_hint.q.out
> > Lines 266 (patched)
> > 
> >
> > Should the expectedEntries be 5000 per the hint?

Yes, looking into it.


> On May 2, 2017, 8:15 p.m., Jason Dere wrote:
> > ql/src/test/results/clientpositive/llap/semijoin_hint.q.out
> > Lines 300 (patched)
> > 
> >
> > Should the expectedEntries be 5000 per the hint?

ditto


- Deepak


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


On May 2, 2017, 5:47 a.m., Deepak Jaiswal wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58914/
> ---
> 
> (Updated May 2, 2017, 5:47 a.m.)
> 
> 
> Review request for hive, Jason Dere and Sergey Shelukhin.
> 
> 
> Bugs: HIVE-16550
> https://issues.apache.org/jira/browse/HIVE-16550
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Semijoin Hints should be able to skip the optimization if needed.
> In addition to that, the patch fixes several issues with hints in general 
> such as,
> - It now works with subqueries and unions.
> - Uses a global data structure instead of per QB.
> 
> 
> Diffs
> -
> 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/DynamicPartitionPruningOptimization.java
>  e1a69526bc 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/HiveOpConverter.java
>  d375d1b58d 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java 1b054a7e24 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/HintParser.g e110fb33df 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/ParseContext.java 3a1f821bd3 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/ParseUtils.java 54e37f7c80 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/QBParseInfo.java 7bf1c599a5 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 
> 654f3b1772 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/TaskCompiler.java 5ea7800528 
>   ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeDynamicListDesc.java 
> 3143554ec6 
>   ql/src/java/org/apache/hadoop/hive/ql/plan/JoinDesc.java 032c7bb28d 
>   ql/src/java/org/apache/hadoop/hive/ql/ppd/SyntheticJoinPredicate.java 
> f45daa8828 
>   ql/src/test/queries/clientpositive/semijoin_hint.q 5de0c8c8c1 
>   ql/src/test/results/clientpositive/llap/semijoin_hint.q.out bc248930ec 
> 
> 
> Diff: https://reviews.apache.org/r/58914/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deepak Jaiswal
> 
>

Re: Review Request 58914: HIVE-16550

2017-05-02 Thread Jason Dere 

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




ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java
Lines 11192 (patched)


Can you give the method name something different than OldHints - how about 
getHintsFromQB?



ql/src/test/queries/clientpositive/semijoin_hint.q
Lines 46 (patched)


Does this query, with no hints, have any runtime filtering? Otherwise you 
wouldn't be able to distinguish whether the None option works or not.



ql/src/test/results/clientpositive/llap/semijoin_hint.q.out
Lines 233 (patched)


Should the expectedEntries be 5000 per the hint?



ql/src/test/results/clientpositive/llap/semijoin_hint.q.out
Lines 266 (patched)


Should the expectedEntries be 5000 per the hint?



ql/src/test/results/clientpositive/llap/semijoin_hint.q.out
Lines 300 (patched)


Should the expectedEntries be 5000 per the hint?


- Jason Dere


On May 2, 2017, 5:47 a.m., Deepak Jaiswal wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58914/
> ---
> 
> (Updated May 2, 2017, 5:47 a.m.)
> 
> 
> Review request for hive, Jason Dere and Sergey Shelukhin.
> 
> 
> Bugs: HIVE-16550
> https://issues.apache.org/jira/browse/HIVE-16550
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Semijoin Hints should be able to skip the optimization if needed.
> In addition to that, the patch fixes several issues with hints in general 
> such as,
> - It now works with subqueries and unions.
> - Uses a global data structure instead of per QB.
> 
> 
> Diffs
> -
> 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/DynamicPartitionPruningOptimization.java
>  e1a69526bc 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/HiveOpConverter.java
>  d375d1b58d 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java 1b054a7e24 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/HintParser.g e110fb33df 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/ParseContext.java 3a1f821bd3 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/ParseUtils.java 54e37f7c80 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/QBParseInfo.java 7bf1c599a5 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 
> 654f3b1772 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/TaskCompiler.java 5ea7800528 
>   ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeDynamicListDesc.java 
> 3143554ec6 
>   ql/src/java/org/apache/hadoop/hive/ql/plan/JoinDesc.java 032c7bb28d 
>   ql/src/java/org/apache/hadoop/hive/ql/ppd/SyntheticJoinPredicate.java 
> f45daa8828 
>   ql/src/test/queries/clientpositive/semijoin_hint.q 5de0c8c8c1 
>   ql/src/test/results/clientpositive/llap/semijoin_hint.q.out bc248930ec 
> 
> 
> Diff: https://reviews.apache.org/r/58914/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deepak Jaiswal
> 
>

Re: Review Request 58914: HIVE-16550

2017-05-02 Thread Deepak Jaiswal 


> On May 2, 2017, 6:35 p.m., Vineet Garg wrote:
> > ql/src/test/queries/clientpositive/semijoin_hint.q
> > Line 38 (original), 38 (patched)
> > 
> >
> > Why is returnpath on?
> > This feature is not yet fully developed/supported and is off by 
> > default. 
> > You might want to try the same queries with this feature off.
> 
> Deepak Jaiswal wrote:
> We have to make sure we dont break it.
> There are tests below with the feature turned off.
> 
> Vineet Garg wrote:
> Ok in that case can you add the same tests with returnpath off? I don't 
> see union all query with returnpath off

sure.


- Deepak


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


On May 2, 2017, 5:47 a.m., Deepak Jaiswal wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58914/
> ---
> 
> (Updated May 2, 2017, 5:47 a.m.)
> 
> 
> Review request for hive, Jason Dere and Sergey Shelukhin.
> 
> 
> Bugs: HIVE-16550
> https://issues.apache.org/jira/browse/HIVE-16550
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Semijoin Hints should be able to skip the optimization if needed.
> In addition to that, the patch fixes several issues with hints in general 
> such as,
> - It now works with subqueries and unions.
> - Uses a global data structure instead of per QB.
> 
> 
> Diffs
> -
> 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/DynamicPartitionPruningOptimization.java
>  e1a69526bc 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/HiveOpConverter.java
>  d375d1b58d 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java 1b054a7e24 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/HintParser.g e110fb33df 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/ParseContext.java 3a1f821bd3 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/ParseUtils.java 54e37f7c80 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/QBParseInfo.java 7bf1c599a5 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 
> 654f3b1772 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/TaskCompiler.java 5ea7800528 
>   ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeDynamicListDesc.java 
> 3143554ec6 
>   ql/src/java/org/apache/hadoop/hive/ql/plan/JoinDesc.java 032c7bb28d 
>   ql/src/java/org/apache/hadoop/hive/ql/ppd/SyntheticJoinPredicate.java 
> f45daa8828 
>   ql/src/test/queries/clientpositive/semijoin_hint.q 5de0c8c8c1 
>   ql/src/test/results/clientpositive/llap/semijoin_hint.q.out bc248930ec 
> 
> 
> Diff: https://reviews.apache.org/r/58914/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deepak Jaiswal
> 
>

Re: Review Request 58914: HIVE-16550

2017-05-02 Thread Vineet Garg 


> On May 2, 2017, 6:35 p.m., Vineet Garg wrote:
> > ql/src/test/queries/clientpositive/semijoin_hint.q
> > Line 38 (original), 38 (patched)
> > 
> >
> > Why is returnpath on?
> > This feature is not yet fully developed/supported and is off by 
> > default. 
> > You might want to try the same queries with this feature off.
> 
> Deepak Jaiswal wrote:
> We have to make sure we dont break it.
> There are tests below with the feature turned off.

Ok in that case can you add the same tests with returnpath off? I don't see 
union all query with returnpath off


- Vineet


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


On May 2, 2017, 5:47 a.m., Deepak Jaiswal wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58914/
> ---
> 
> (Updated May 2, 2017, 5:47 a.m.)
> 
> 
> Review request for hive, Jason Dere and Sergey Shelukhin.
> 
> 
> Bugs: HIVE-16550
> https://issues.apache.org/jira/browse/HIVE-16550
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Semijoin Hints should be able to skip the optimization if needed.
> In addition to that, the patch fixes several issues with hints in general 
> such as,
> - It now works with subqueries and unions.
> - Uses a global data structure instead of per QB.
> 
> 
> Diffs
> -
> 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/DynamicPartitionPruningOptimization.java
>  e1a69526bc 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/HiveOpConverter.java
>  d375d1b58d 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java 1b054a7e24 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/HintParser.g e110fb33df 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/ParseContext.java 3a1f821bd3 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/ParseUtils.java 54e37f7c80 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/QBParseInfo.java 7bf1c599a5 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 
> 654f3b1772 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/TaskCompiler.java 5ea7800528 
>   ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeDynamicListDesc.java 
> 3143554ec6 
>   ql/src/java/org/apache/hadoop/hive/ql/plan/JoinDesc.java 032c7bb28d 
>   ql/src/java/org/apache/hadoop/hive/ql/ppd/SyntheticJoinPredicate.java 
> f45daa8828 
>   ql/src/test/queries/clientpositive/semijoin_hint.q 5de0c8c8c1 
>   ql/src/test/results/clientpositive/llap/semijoin_hint.q.out bc248930ec 
> 
> 
> Diff: https://reviews.apache.org/r/58914/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deepak Jaiswal
> 
>

Re: Review Request 58914: HIVE-16550

2017-05-02 Thread Deepak Jaiswal 


> On May 2, 2017, 6:35 p.m., Vineet Garg wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java
> > Line 338 (original), 338 (patched)
> > 
> >
> > Can you add comments to explain why are we doing this i.e. why are we 
> > propagating hints?

Will do it thanks.


> On May 2, 2017, 6:35 p.m., Vineet Garg wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java
> > Lines 368 (patched)
> > 
> >
> > If I understand it correctly before generating calcite plan (i.e. 
> > calling getOptimizedAST) QB has hints but after generating optimized AST QB 
> > is reset and lose hints? That is why you are propagating hints?

Yes.


> On May 2, 2017, 6:35 p.m., Vineet Garg wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java
> > Line 408 (original), 413 (patched)
> > 
> >
> > What happens in this case? Why is it necessary to log warning here?

I think just paranoia. It shouldn't happen, however, if it does, we log it.


> On May 2, 2017, 6:35 p.m., Vineet Garg wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/ParseUtils.java
> > Line 444 (original), 444 (patched)
> > 
> >
> > Is this because we can have hint now? If so can you rather check for 
> > hint and continue instead of continuing for all nodes

Makes sense. Will do.


> On May 2, 2017, 6:35 p.m., Vineet Garg wrote:
> > ql/src/test/queries/clientpositive/semijoin_hint.q
> > Line 38 (original), 38 (patched)
> > 
> >
> > Why is returnpath on?
> > This feature is not yet fully developed/supported and is off by 
> > default. 
> > You might want to try the same queries with this feature off.

We have to make sure we dont break it.
There are tests below with the feature turned off.


- Deepak


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


On May 2, 2017, 5:47 a.m., Deepak Jaiswal wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58914/
> ---
> 
> (Updated May 2, 2017, 5:47 a.m.)
> 
> 
> Review request for hive, Jason Dere and Sergey Shelukhin.
> 
> 
> Bugs: HIVE-16550
> https://issues.apache.org/jira/browse/HIVE-16550
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Semijoin Hints should be able to skip the optimization if needed.
> In addition to that, the patch fixes several issues with hints in general 
> such as,
> - It now works with subqueries and unions.
> - Uses a global data structure instead of per QB.
> 
> 
> Diffs
> -
> 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/DynamicPartitionPruningOptimization.java
>  e1a69526bc 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/HiveOpConverter.java
>  d375d1b58d 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java 1b054a7e24 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/HintParser.g e110fb33df 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/ParseContext.java 3a1f821bd3 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/ParseUtils.java 54e37f7c80 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/QBParseInfo.java 7bf1c599a5 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 
> 654f3b1772 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/TaskCompiler.java 5ea7800528 
>   ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeDynamicListDesc.java 
> 3143554ec6 
>   ql/src/java/org/apache/hadoop/hive/ql/plan/JoinDesc.java 032c7bb28d 
>   ql/src/java/org/apache/hadoop/hive/ql/ppd/SyntheticJoinPredicate.java 
> f45daa8828 
>   ql/src/test/queries/clientpositive/semijoin_hint.q 5de0c8c8c1 
>   ql/src/test/results/clientpositive/llap/semijoin_hint.q.out bc248930ec 
> 
> 
> Diff: https://reviews.apache.org/r/58914/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deepak Jaiswal
> 
>

Re: Review Request 58914: HIVE-16550

2017-05-02 Thread Vineet Garg 

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




ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java
Line 338 (original), 338 (patched)


Can you add comments to explain why are we doing this i.e. why are we 
propagating hints?



ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java
Lines 368 (patched)


If I understand it correctly before generating calcite plan (i.e. calling 
getOptimizedAST) QB has hints but after generating optimized AST QB is reset 
and lose hints? That is why you are propagating hints?



ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java
Line 408 (original), 413 (patched)


What happens in this case? Why is it necessary to log warning here?



ql/src/java/org/apache/hadoop/hive/ql/parse/ParseUtils.java
Line 444 (original), 444 (patched)


Is this because we can have hint now? If so can you rather check for hint 
and continue instead of continuing for all nodes



ql/src/test/queries/clientpositive/semijoin_hint.q
Line 38 (original), 38 (patched)


Why is returnpath on?
This feature is not yet fully developed/supported and is off by default. 
You might want to try the same queries with this feature off.


- Vineet Garg


On May 2, 2017, 5:47 a.m., Deepak Jaiswal wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58914/
> ---
> 
> (Updated May 2, 2017, 5:47 a.m.)
> 
> 
> Review request for hive, Jason Dere and Sergey Shelukhin.
> 
> 
> Bugs: HIVE-16550
> https://issues.apache.org/jira/browse/HIVE-16550
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Semijoin Hints should be able to skip the optimization if needed.
> In addition to that, the patch fixes several issues with hints in general 
> such as,
> - It now works with subqueries and unions.
> - Uses a global data structure instead of per QB.
> 
> 
> Diffs
> -
> 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/DynamicPartitionPruningOptimization.java
>  e1a69526bc 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/HiveOpConverter.java
>  d375d1b58d 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java 1b054a7e24 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/HintParser.g e110fb33df 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/ParseContext.java 3a1f821bd3 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/ParseUtils.java 54e37f7c80 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/QBParseInfo.java 7bf1c599a5 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 
> 654f3b1772 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/TaskCompiler.java 5ea7800528 
>   ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeDynamicListDesc.java 
> 3143554ec6 
>   ql/src/java/org/apache/hadoop/hive/ql/plan/JoinDesc.java 032c7bb28d 
>   ql/src/java/org/apache/hadoop/hive/ql/ppd/SyntheticJoinPredicate.java 
> f45daa8828 
>   ql/src/test/queries/clientpositive/semijoin_hint.q 5de0c8c8c1 
>   ql/src/test/results/clientpositive/llap/semijoin_hint.q.out bc248930ec 
> 
> 
> Diff: https://reviews.apache.org/r/58914/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deepak Jaiswal
> 
>