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 2018/05/24 20:20:48 UTC
Review Request 67296: HIVE-18875 : Enable SMB Join by default in Tez
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67296/
-----------------------------------------------------------
Review request for hive, Gunther Hagleitner and Jason Dere.
Bugs: HIVE-18875
https://issues.apache.org/jira/browse/HIVE-18875
Repository: hive-git
Description
-------
Fixed various issues with SMB, mostly on the Reducer side join.
GBY Op now uses inputObjectInspector[0] all the time as it is the only OI it has. The tag is irrelevant here. Was causing problem with SMB.
Disabled SMB in spark on hive tests as the same config for Tez was enabling it there.
Some SMB specific tests were designed to first run without SMB and then with SMB. With SMB enabled by default, it is explicitely turned off to make sure the behavior is maintained.
Please go through JIRA comments as they may clear out some questions.
Diffs
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 931533a556
ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java aefaa0586e
ql/src/java/org/apache/hadoop/hive/ql/exec/GroupByOperator.java 4b766382ef
ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java 4019f132d3
ql/src/java/org/apache/hadoop/hive/ql/optimizer/metainfo/annotation/OpTraitsRulesProcFactory.java 9e5446566b
ql/src/test/queries/clientpositive/auto_sortmerge_join_11.q 7416eb0ec0
ql/src/test/queries/clientpositive/skewjoinopt19.q 02cadda7f5
ql/src/test/queries/clientpositive/skewjoinopt20.q 160e5b82d9
ql/src/test/queries/clientpositive/smb_mapjoin_11.q 6ce49b83c2
ql/src/test/queries/clientpositive/smb_mapjoin_12.q 753e4d3c9a
ql/src/test/queries/clientpositive/smb_mapjoin_17.q d68f5f3139
ql/src/test/queries/clientpositive/subquery_notin.q 64940277bb
ql/src/test/results/clientpositive/llap/correlationoptimizer2.q.out 0f839ead0e
ql/src/test/results/clientpositive/llap/correlationoptimizer6.q.out 499ef4b178
ql/src/test/results/clientpositive/llap/explainuser_1.q.out f4852d2f15
ql/src/test/results/clientpositive/llap/limit_pushdown.q.out 79311d0415
ql/src/test/results/clientpositive/llap/mergejoin.q.out 832ed487ec
ql/src/test/results/clientpositive/llap/mrr.q.out 737c73893f
ql/src/test/results/clientpositive/llap/offset_limit_ppd_optimizer.q.out 09a120ae12
ql/src/test/results/clientpositive/llap/smb_cache.q.out 7c885d1ffa
ql/src/test/results/clientpositive/llap/smb_mapjoin_14.q.out c334b9386b
ql/src/test/results/clientpositive/llap/smb_mapjoin_15.q.out 21aac455f2
ql/src/test/results/clientpositive/llap/smb_mapjoin_4.q.out 4b8728fbff
ql/src/test/results/clientpositive/llap/smb_mapjoin_5.q.out a1313696f0
ql/src/test/results/clientpositive/llap/smb_mapjoin_6.q.out f44a0dbc70
ql/src/test/results/clientpositive/llap/subquery_in_having.q.out c9956121f8
ql/src/test/results/clientpositive/llap/subquery_notin.q.out d72e8c349c
ql/src/test/results/clientpositive/llap/vectorized_bucketmapjoin1.q.out 61c5051bb9
ql/src/test/results/clientpositive/spark/bucketmapjoin1.q.out a79a8c466a
ql/src/test/results/clientpositive/spark/smb_mapjoin_14.q.out 1fd4490ac4
ql/src/test/results/clientpositive/spark/smb_mapjoin_15.q.out 6ca577fdbb
ql/src/test/results/clientpositive/spark/smb_mapjoin_4.q.out 629a6c428a
ql/src/test/results/clientpositive/spark/smb_mapjoin_5.q.out 7d0934010e
ql/src/test/results/clientpositive/spark/smb_mapjoin_6.q.out 7445135159
ql/src/test/results/clientpositive/spark/subquery_notin.q.out ea473c3b40
Diff: https://reviews.apache.org/r/67296/diff/1/
Testing
-------
Thanks,
Deepak Jaiswal
Re: Review Request 67296: HIVE-18875 : Enable SMB Join by default in
Tez
Posted by Deepak Jaiswal <dj...@hortonworks.com>.
> On June 9, 2018, 6:02 a.m., Gunther Hagleitner wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java
> > Lines 167 (patched)
> > <https://reviews.apache.org/r/67296/diff/4/?file=2036590#file2036590line167>
> >
> > This should be ok to do always (regardless of whether it's map or reduce)? If not can you explain why?
It maybe, however, I just wanted to isolate the cases which fail and to not introduce unwanted bugs.
There are 3 types of joins using this operator
1. Shuffle Join on reducer does not need to flush out the last record as it gets it as soon as the record is read.
2. Mapside SMB also gets the records as soon as read as the TS Op does not keep them holding.
3. ReduceSide SMB is the only one which is prevented from the last record as it is held by GBY Op and only flushed out when Op is closed leading to potential join misses.
> On June 9, 2018, 6:02 a.m., Gunther Hagleitner wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java
> > Lines 547 (patched)
> > <https://reviews.apache.org/r/67296/diff/4/?file=2036590#file2036590line548>
> >
> > please don't do this in the inner loop. this has to be removed or changed. if you want a safety check, just write an if loop.
Sure, will do that.
> On June 9, 2018, 6:02 a.m., Gunther Hagleitner wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/ReduceRecordSource.java
> > Line 246 (original), 249 (patched)
> > <https://reviews.apache.org/r/67296/diff/4/?file=2036592#file2036592line249>
> >
> > does this need a parallel fix for the vectorized path?
There is no vectorized path for this operator.
> On June 9, 2018, 6:02 a.m., Gunther Hagleitner wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/ReduceRecordSource.java
> > Lines 261 (patched)
> > <https://reviews.apache.org/r/67296/diff/4/?file=2036592#file2036592line261>
> >
> > You're only doing this for the "reducer". The Operator<> class implements this as an empty method. Theoretically if you have a plan like:
> >
> > Fil -> Gby -> Join
> >
> > This fix wouldn't work, because the Fil would be the reducer and it doesn't forward the flush to Gby.
> >
> > Unfortunately DemuxOperator uses flush and I don't know if recursive would work for it. So maybe add a
> >
> > flushRecursive() {
> > flush();
> > for all children:
> > child.flushRecursive()
> > }
> >
> > in Operator and use it?
Thanks. It definitely looks more fool proof.
> On June 9, 2018, 6:02 a.m., Gunther Hagleitner wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/ReduceRecordSource.java
> > Lines 518 (patched)
> > <https://reviews.apache.org/r/67296/diff/4/?file=2036592#file2036592line518>
> >
> > Stylistically I think "setFlushLastRecord(boolean flushLastRecord)" would be nicer.
the idea is to prevent user from setting it to false.
Anyway, I will do it.
> On June 9, 2018, 6:02 a.m., Gunther Hagleitner wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java
> > Lines 611 (patched)
> > <https://reviews.apache.org/r/67296/diff/4/?file=2036593#file2036593line611>
> >
> > Open follow up jira? Still doesn't make sense that this is needed given the code below.
https://issues.apache.org/jira/browse/HIVE-19840
> On June 9, 2018, 6:02 a.m., Gunther Hagleitner wrote:
> > ql/src/test/results/clientpositive/llap/mrr.q.out
> > Line 460 (original), 460 (patched)
> > <https://reviews.apache.org/r/67296/diff/4/?file=2036608#file2036608line460>
> >
> > these diffs are weird... but ok.
yeah, I got a merge conflict due to these when rebased. Weird.
- Deepak
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67296/#review204524
-----------------------------------------------------------
On June 9, 2018, 2:19 a.m., Deepak Jaiswal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67296/
> -----------------------------------------------------------
>
> (Updated June 9, 2018, 2:19 a.m.)
>
>
> Review request for hive, Gunther Hagleitner and Jason Dere.
>
>
> Bugs: HIVE-18875
> https://issues.apache.org/jira/browse/HIVE-18875
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Fixed various issues with SMB, mostly on the Reducer side join.
> GBY Op now uses inputObjectInspector[0] all the time as it is the only OI it has. The tag is irrelevant here. Was causing problem with SMB.
> Disabled SMB in spark on hive tests as the same config for Tez was enabling it there.
> Some SMB specific tests were designed to first run without SMB and then with SMB. With SMB enabled by default, it is explicitely turned off to make sure the behavior is maintained.
>
> Please go through JIRA comments as they may clear out some questions.
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java b5e2d86e62
> itests/src/test/resources/testconfiguration.properties b584c72650
> ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java aefaa0586e
> ql/src/java/org/apache/hadoop/hive/ql/exec/GroupByOperator.java 4b766382ef
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/ReduceRecordSource.java fca783c35e
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java 4019f132d3
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/metainfo/annotation/OpTraitsRulesProcFactory.java 9e5446566b
> ql/src/test/queries/clientpositive/auto_sortmerge_join_11.q 7416eb0ec0
> ql/src/test/queries/clientpositive/skewjoinopt19.q 02cadda7f5
> ql/src/test/queries/clientpositive/skewjoinopt20.q 160e5b82d9
> ql/src/test/queries/clientpositive/smb_mapjoin_11.q 6ce49b83c2
> ql/src/test/queries/clientpositive/smb_mapjoin_12.q 753e4d3c9a
> ql/src/test/queries/clientpositive/smb_mapjoin_17.q d68f5f3139
> ql/src/test/queries/clientpositive/subquery_notin.q 64940277bb
> ql/src/test/queries/clientpositive/tez_smb_reduce_side.q PRE-CREATION
> ql/src/test/results/clientpositive/llap/correlationoptimizer2.q.out 8e17d952d4
> ql/src/test/results/clientpositive/llap/correlationoptimizer6.q.out 9e424c2f16
> ql/src/test/results/clientpositive/llap/explainuser_1.q.out 0ebd5caf28
> ql/src/test/results/clientpositive/llap/limit_pushdown.q.out fe8b98f21f
> ql/src/test/results/clientpositive/llap/mergejoin.q.out 832ed487ec
> ql/src/test/results/clientpositive/llap/mrr.q.out cb25b8c2f9
> ql/src/test/results/clientpositive/llap/offset_limit_ppd_optimizer.q.out ca0de47b5a
> ql/src/test/results/clientpositive/llap/smb_cache.q.out 7c885d1ffa
> ql/src/test/results/clientpositive/llap/smb_mapjoin_14.q.out c334b9386b
> ql/src/test/results/clientpositive/llap/smb_mapjoin_15.q.out 21aac455f2
> ql/src/test/results/clientpositive/llap/smb_mapjoin_4.q.out 4b8728fbff
> ql/src/test/results/clientpositive/llap/smb_mapjoin_5.q.out a1313696f0
> ql/src/test/results/clientpositive/llap/smb_mapjoin_6.q.out 3e5acd08a7
> ql/src/test/results/clientpositive/llap/subquery_in_having.q.out b4ce6f8777
> ql/src/test/results/clientpositive/llap/subquery_notin.q.out 21a0f84f33
> ql/src/test/results/clientpositive/llap/tez_smb_reduce_side.q.out PRE-CREATION
> ql/src/test/results/clientpositive/llap/vectorized_bucketmapjoin1.q.out 61c5051bb9
> ql/src/test/results/clientpositive/spark/bucketmapjoin1.q.out a79a8c466a
> ql/src/test/results/clientpositive/spark/smb_mapjoin_14.q.out 1fd4490ac4
> ql/src/test/results/clientpositive/spark/smb_mapjoin_15.q.out 6ca577fdbb
> ql/src/test/results/clientpositive/spark/smb_mapjoin_4.q.out 629a6c428a
> ql/src/test/results/clientpositive/spark/smb_mapjoin_5.q.out 7d0934010e
> ql/src/test/results/clientpositive/spark/smb_mapjoin_6.q.out 7445135159
> ql/src/test/results/clientpositive/spark/subquery_notin.q.out a53c31353b
>
>
> Diff: https://reviews.apache.org/r/67296/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Deepak Jaiswal
>
>
Re: Review Request 67296: HIVE-18875 : Enable SMB Join by default in
Tez
Posted by Gunther Hagleitner <gu...@apache.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67296/#review204524
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java
Lines 167 (patched)
<https://reviews.apache.org/r/67296/#comment287068>
This should be ok to do always (regardless of whether it's map or reduce)? If not can you explain why?
ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java
Lines 547 (patched)
<https://reviews.apache.org/r/67296/#comment287067>
please don't do this in the inner loop. this has to be removed or changed. if you want a safety check, just write an if loop.
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/ReduceRecordSource.java
Line 246 (original), 249 (patched)
<https://reviews.apache.org/r/67296/#comment287073>
does this need a parallel fix for the vectorized path?
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/ReduceRecordSource.java
Lines 261 (patched)
<https://reviews.apache.org/r/67296/#comment287069>
You're only doing this for the "reducer". The Operator<> class implements this as an empty method. Theoretically if you have a plan like:
Fil -> Gby -> Join
This fix wouldn't work, because the Fil would be the reducer and it doesn't forward the flush to Gby.
Unfortunately DemuxOperator uses flush and I don't know if recursive would work for it. So maybe add a
flushRecursive() {
flush();
for all children:
child.flushRecursive()
}
in Operator and use it?
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/ReduceRecordSource.java
Lines 518 (patched)
<https://reviews.apache.org/r/67296/#comment287070>
Stylistically I think "setFlushLastRecord(boolean flushLastRecord)" would be nicer.
ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java
Lines 611 (patched)
<https://reviews.apache.org/r/67296/#comment287071>
Open follow up jira? Still doesn't make sense that this is needed given the code below.
ql/src/test/results/clientpositive/llap/mrr.q.out
Line 460 (original), 460 (patched)
<https://reviews.apache.org/r/67296/#comment287072>
these diffs are weird... but ok.
- Gunther Hagleitner
On June 9, 2018, 2:19 a.m., Deepak Jaiswal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67296/
> -----------------------------------------------------------
>
> (Updated June 9, 2018, 2:19 a.m.)
>
>
> Review request for hive, Gunther Hagleitner and Jason Dere.
>
>
> Bugs: HIVE-18875
> https://issues.apache.org/jira/browse/HIVE-18875
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Fixed various issues with SMB, mostly on the Reducer side join.
> GBY Op now uses inputObjectInspector[0] all the time as it is the only OI it has. The tag is irrelevant here. Was causing problem with SMB.
> Disabled SMB in spark on hive tests as the same config for Tez was enabling it there.
> Some SMB specific tests were designed to first run without SMB and then with SMB. With SMB enabled by default, it is explicitely turned off to make sure the behavior is maintained.
>
> Please go through JIRA comments as they may clear out some questions.
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java b5e2d86e62
> itests/src/test/resources/testconfiguration.properties b584c72650
> ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java aefaa0586e
> ql/src/java/org/apache/hadoop/hive/ql/exec/GroupByOperator.java 4b766382ef
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/ReduceRecordSource.java fca783c35e
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java 4019f132d3
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/metainfo/annotation/OpTraitsRulesProcFactory.java 9e5446566b
> ql/src/test/queries/clientpositive/auto_sortmerge_join_11.q 7416eb0ec0
> ql/src/test/queries/clientpositive/skewjoinopt19.q 02cadda7f5
> ql/src/test/queries/clientpositive/skewjoinopt20.q 160e5b82d9
> ql/src/test/queries/clientpositive/smb_mapjoin_11.q 6ce49b83c2
> ql/src/test/queries/clientpositive/smb_mapjoin_12.q 753e4d3c9a
> ql/src/test/queries/clientpositive/smb_mapjoin_17.q d68f5f3139
> ql/src/test/queries/clientpositive/subquery_notin.q 64940277bb
> ql/src/test/queries/clientpositive/tez_smb_reduce_side.q PRE-CREATION
> ql/src/test/results/clientpositive/llap/correlationoptimizer2.q.out 8e17d952d4
> ql/src/test/results/clientpositive/llap/correlationoptimizer6.q.out 9e424c2f16
> ql/src/test/results/clientpositive/llap/explainuser_1.q.out 0ebd5caf28
> ql/src/test/results/clientpositive/llap/limit_pushdown.q.out fe8b98f21f
> ql/src/test/results/clientpositive/llap/mergejoin.q.out 832ed487ec
> ql/src/test/results/clientpositive/llap/mrr.q.out cb25b8c2f9
> ql/src/test/results/clientpositive/llap/offset_limit_ppd_optimizer.q.out ca0de47b5a
> ql/src/test/results/clientpositive/llap/smb_cache.q.out 7c885d1ffa
> ql/src/test/results/clientpositive/llap/smb_mapjoin_14.q.out c334b9386b
> ql/src/test/results/clientpositive/llap/smb_mapjoin_15.q.out 21aac455f2
> ql/src/test/results/clientpositive/llap/smb_mapjoin_4.q.out 4b8728fbff
> ql/src/test/results/clientpositive/llap/smb_mapjoin_5.q.out a1313696f0
> ql/src/test/results/clientpositive/llap/smb_mapjoin_6.q.out 3e5acd08a7
> ql/src/test/results/clientpositive/llap/subquery_in_having.q.out b4ce6f8777
> ql/src/test/results/clientpositive/llap/subquery_notin.q.out 21a0f84f33
> ql/src/test/results/clientpositive/llap/tez_smb_reduce_side.q.out PRE-CREATION
> ql/src/test/results/clientpositive/llap/vectorized_bucketmapjoin1.q.out 61c5051bb9
> ql/src/test/results/clientpositive/spark/bucketmapjoin1.q.out a79a8c466a
> ql/src/test/results/clientpositive/spark/smb_mapjoin_14.q.out 1fd4490ac4
> ql/src/test/results/clientpositive/spark/smb_mapjoin_15.q.out 6ca577fdbb
> ql/src/test/results/clientpositive/spark/smb_mapjoin_4.q.out 629a6c428a
> ql/src/test/results/clientpositive/spark/smb_mapjoin_5.q.out 7d0934010e
> ql/src/test/results/clientpositive/spark/smb_mapjoin_6.q.out 7445135159
> ql/src/test/results/clientpositive/spark/subquery_notin.q.out a53c31353b
>
>
> Diff: https://reviews.apache.org/r/67296/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Deepak Jaiswal
>
>
Re: Review Request 67296: HIVE-18875 : Enable SMB Join by default in
Tez
Posted by Deepak Jaiswal <dj...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67296/
-----------------------------------------------------------
(Updated June 9, 2018, 10:12 a.m.)
Review request for hive, Gunther Hagleitner and Jason Dere.
Changes
-------
Implemented review comments.
Updated result files for mrr and another one for stats diff.
Updated result file for auto_sortmerge_join_6 where it incorrectly created SMB for non-bucketed columns giving wrong results.
Bugs: HIVE-18875
https://issues.apache.org/jira/browse/HIVE-18875
Repository: hive-git
Description
-------
Fixed various issues with SMB, mostly on the Reducer side join.
GBY Op now uses inputObjectInspector[0] all the time as it is the only OI it has. The tag is irrelevant here. Was causing problem with SMB.
Disabled SMB in spark on hive tests as the same config for Tez was enabling it there.
Some SMB specific tests were designed to first run without SMB and then with SMB. With SMB enabled by default, it is explicitely turned off to make sure the behavior is maintained.
Please go through JIRA comments as they may clear out some questions.
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java b5e2d86e62
itests/src/test/resources/testconfiguration.properties b584c72650
ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java aefaa0586e
ql/src/java/org/apache/hadoop/hive/ql/exec/GroupByOperator.java 4b766382ef
ql/src/java/org/apache/hadoop/hive/ql/exec/Operator.java 108bb57c41
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/ReduceRecordSource.java fca783c35e
ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java 4019f132d3
ql/src/java/org/apache/hadoop/hive/ql/optimizer/metainfo/annotation/OpTraitsRulesProcFactory.java 9e5446566b
ql/src/test/queries/clientpositive/auto_sortmerge_join_11.q 7416eb0ec0
ql/src/test/queries/clientpositive/auto_sortmerge_join_6.q 551e5f7e47
ql/src/test/queries/clientpositive/skewjoinopt19.q 02cadda7f5
ql/src/test/queries/clientpositive/skewjoinopt20.q 160e5b82d9
ql/src/test/queries/clientpositive/smb_mapjoin_11.q 6ce49b83c2
ql/src/test/queries/clientpositive/smb_mapjoin_12.q 753e4d3c9a
ql/src/test/queries/clientpositive/smb_mapjoin_17.q d68f5f3139
ql/src/test/queries/clientpositive/subquery_notin.q 64940277bb
ql/src/test/queries/clientpositive/tez_smb_reduce_side.q PRE-CREATION
ql/src/test/results/clientpositive/llap/auto_sortmerge_join_6.q.out b13beab49b
ql/src/test/results/clientpositive/llap/correlationoptimizer2.q.out 8e17d952d4
ql/src/test/results/clientpositive/llap/correlationoptimizer6.q.out 9e424c2f16
ql/src/test/results/clientpositive/llap/explainuser_1.q.out 0ebd5caf28
ql/src/test/results/clientpositive/llap/limit_pushdown.q.out fe8b98f21f
ql/src/test/results/clientpositive/llap/mergejoin.q.out 832ed487ec
ql/src/test/results/clientpositive/llap/mrr.q.out cb25b8c2f9
ql/src/test/results/clientpositive/llap/offset_limit_ppd_optimizer.q.out ca0de47b5a
ql/src/test/results/clientpositive/llap/smb_cache.q.out 7c885d1ffa
ql/src/test/results/clientpositive/llap/smb_mapjoin_14.q.out c334b9386b
ql/src/test/results/clientpositive/llap/smb_mapjoin_15.q.out 21aac455f2
ql/src/test/results/clientpositive/llap/smb_mapjoin_4.q.out 4b8728fbff
ql/src/test/results/clientpositive/llap/smb_mapjoin_5.q.out a1313696f0
ql/src/test/results/clientpositive/llap/smb_mapjoin_6.q.out 3e5acd08a7
ql/src/test/results/clientpositive/llap/subquery_in_having.q.out b4ce6f8777
ql/src/test/results/clientpositive/llap/subquery_notin.q.out 21a0f84f33
ql/src/test/results/clientpositive/llap/tez_smb_reduce_side.q.out PRE-CREATION
ql/src/test/results/clientpositive/llap/vectorized_bucketmapjoin1.q.out 61c5051bb9
ql/src/test/results/clientpositive/spark/bucketmapjoin1.q.out a79a8c466a
ql/src/test/results/clientpositive/spark/smb_mapjoin_14.q.out 1fd4490ac4
ql/src/test/results/clientpositive/spark/smb_mapjoin_15.q.out 6ca577fdbb
ql/src/test/results/clientpositive/spark/smb_mapjoin_4.q.out 629a6c428a
ql/src/test/results/clientpositive/spark/smb_mapjoin_5.q.out 7d0934010e
ql/src/test/results/clientpositive/spark/smb_mapjoin_6.q.out 7445135159
ql/src/test/results/clientpositive/spark/subquery_notin.q.out a53c31353b
Diff: https://reviews.apache.org/r/67296/diff/5/
Changes: https://reviews.apache.org/r/67296/diff/4-5/
Testing
-------
Thanks,
Deepak Jaiswal
Re: Review Request 67296: HIVE-18875 : Enable SMB Join by default in
Tez
Posted by Deepak Jaiswal <dj...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67296/
-----------------------------------------------------------
(Updated June 9, 2018, 2:19 a.m.)
Review request for hive, Gunther Hagleitner and Jason Dere.
Changes
-------
Departed from the GBY Op Forward strategy as it will fail with multiple splits.
Flush the last record from ReduceRecordSource when it is runs out of records to make sure Join op gets the last record instead of waiting for the close() call on the parent to flush it.
Added a new test to cover multiple different states not covered in existing ptests.
Bugs: HIVE-18875
https://issues.apache.org/jira/browse/HIVE-18875
Repository: hive-git
Description
-------
Fixed various issues with SMB, mostly on the Reducer side join.
GBY Op now uses inputObjectInspector[0] all the time as it is the only OI it has. The tag is irrelevant here. Was causing problem with SMB.
Disabled SMB in spark on hive tests as the same config for Tez was enabling it there.
Some SMB specific tests were designed to first run without SMB and then with SMB. With SMB enabled by default, it is explicitely turned off to make sure the behavior is maintained.
Please go through JIRA comments as they may clear out some questions.
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java b5e2d86e62
itests/src/test/resources/testconfiguration.properties b584c72650
ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java aefaa0586e
ql/src/java/org/apache/hadoop/hive/ql/exec/GroupByOperator.java 4b766382ef
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/ReduceRecordSource.java fca783c35e
ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java 4019f132d3
ql/src/java/org/apache/hadoop/hive/ql/optimizer/metainfo/annotation/OpTraitsRulesProcFactory.java 9e5446566b
ql/src/test/queries/clientpositive/auto_sortmerge_join_11.q 7416eb0ec0
ql/src/test/queries/clientpositive/skewjoinopt19.q 02cadda7f5
ql/src/test/queries/clientpositive/skewjoinopt20.q 160e5b82d9
ql/src/test/queries/clientpositive/smb_mapjoin_11.q 6ce49b83c2
ql/src/test/queries/clientpositive/smb_mapjoin_12.q 753e4d3c9a
ql/src/test/queries/clientpositive/smb_mapjoin_17.q d68f5f3139
ql/src/test/queries/clientpositive/subquery_notin.q 64940277bb
ql/src/test/queries/clientpositive/tez_smb_reduce_side.q PRE-CREATION
ql/src/test/results/clientpositive/llap/correlationoptimizer2.q.out 8e17d952d4
ql/src/test/results/clientpositive/llap/correlationoptimizer6.q.out 9e424c2f16
ql/src/test/results/clientpositive/llap/explainuser_1.q.out 0ebd5caf28
ql/src/test/results/clientpositive/llap/limit_pushdown.q.out fe8b98f21f
ql/src/test/results/clientpositive/llap/mergejoin.q.out 832ed487ec
ql/src/test/results/clientpositive/llap/mrr.q.out cb25b8c2f9
ql/src/test/results/clientpositive/llap/offset_limit_ppd_optimizer.q.out ca0de47b5a
ql/src/test/results/clientpositive/llap/smb_cache.q.out 7c885d1ffa
ql/src/test/results/clientpositive/llap/smb_mapjoin_14.q.out c334b9386b
ql/src/test/results/clientpositive/llap/smb_mapjoin_15.q.out 21aac455f2
ql/src/test/results/clientpositive/llap/smb_mapjoin_4.q.out 4b8728fbff
ql/src/test/results/clientpositive/llap/smb_mapjoin_5.q.out a1313696f0
ql/src/test/results/clientpositive/llap/smb_mapjoin_6.q.out 3e5acd08a7
ql/src/test/results/clientpositive/llap/subquery_in_having.q.out b4ce6f8777
ql/src/test/results/clientpositive/llap/subquery_notin.q.out 21a0f84f33
ql/src/test/results/clientpositive/llap/tez_smb_reduce_side.q.out PRE-CREATION
ql/src/test/results/clientpositive/llap/vectorized_bucketmapjoin1.q.out 61c5051bb9
ql/src/test/results/clientpositive/spark/bucketmapjoin1.q.out a79a8c466a
ql/src/test/results/clientpositive/spark/smb_mapjoin_14.q.out 1fd4490ac4
ql/src/test/results/clientpositive/spark/smb_mapjoin_15.q.out 6ca577fdbb
ql/src/test/results/clientpositive/spark/smb_mapjoin_4.q.out 629a6c428a
ql/src/test/results/clientpositive/spark/smb_mapjoin_5.q.out 7d0934010e
ql/src/test/results/clientpositive/spark/smb_mapjoin_6.q.out 7445135159
ql/src/test/results/clientpositive/spark/subquery_notin.q.out a53c31353b
Diff: https://reviews.apache.org/r/67296/diff/4/
Changes: https://reviews.apache.org/r/67296/diff/3-4/
Testing
-------
Thanks,
Deepak Jaiswal
Re: Review Request 67296: HIVE-18875 : Enable SMB Join by default in
Tez
Posted by Deepak Jaiswal <dj...@hortonworks.com>.
> On June 5, 2018, 3:21 a.m., Gunther Hagleitner wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/GroupByOperator.java
> > Lines 733 (patched)
> > <https://reviews.apache.org/r/67296/diff/3/?file=2034851#file2034851line733>
> >
> > this adds another branch in the inner loop also. might have perf implications.
>
> Deepak Jaiswal wrote:
> Sure, will find a way to avoid it.
On code inspection, I just made it part of the main if..else.
- Deepak
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67296/#review204301
-----------------------------------------------------------
On June 4, 2018, 5:38 a.m., Deepak Jaiswal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67296/
> -----------------------------------------------------------
>
> (Updated June 4, 2018, 5:38 a.m.)
>
>
> Review request for hive, Gunther Hagleitner and Jason Dere.
>
>
> Bugs: HIVE-18875
> https://issues.apache.org/jira/browse/HIVE-18875
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Fixed various issues with SMB, mostly on the Reducer side join.
> GBY Op now uses inputObjectInspector[0] all the time as it is the only OI it has. The tag is irrelevant here. Was causing problem with SMB.
> Disabled SMB in spark on hive tests as the same config for Tez was enabling it there.
> Some SMB specific tests were designed to first run without SMB and then with SMB. With SMB enabled by default, it is explicitely turned off to make sure the behavior is maintained.
>
> Please go through JIRA comments as they may clear out some questions.
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 3295d1dbc5
> ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java aefaa0586e
> ql/src/java/org/apache/hadoop/hive/ql/exec/GroupByOperator.java 4b766382ef
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java 4019f132d3
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/metainfo/annotation/OpTraitsRulesProcFactory.java 9e5446566b
> ql/src/test/queries/clientpositive/auto_sortmerge_join_11.q 7416eb0ec0
> ql/src/test/queries/clientpositive/skewjoinopt19.q 02cadda7f5
> ql/src/test/queries/clientpositive/skewjoinopt20.q 160e5b82d9
> ql/src/test/queries/clientpositive/smb_mapjoin_11.q 6ce49b83c2
> ql/src/test/queries/clientpositive/smb_mapjoin_12.q 753e4d3c9a
> ql/src/test/queries/clientpositive/smb_mapjoin_17.q d68f5f3139
> ql/src/test/queries/clientpositive/subquery_notin.q 64940277bb
> ql/src/test/results/clientpositive/llap/correlationoptimizer2.q.out 0f839ead0e
> ql/src/test/results/clientpositive/llap/correlationoptimizer6.q.out 499ef4b178
> ql/src/test/results/clientpositive/llap/explainuser_1.q.out 0c339e5c8f
> ql/src/test/results/clientpositive/llap/limit_pushdown.q.out 76fae9a152
> ql/src/test/results/clientpositive/llap/mergejoin.q.out 832ed487ec
> ql/src/test/results/clientpositive/llap/mrr.q.out 737c73893f
> ql/src/test/results/clientpositive/llap/offset_limit_ppd_optimizer.q.out 66460271b4
> ql/src/test/results/clientpositive/llap/smb_cache.q.out 7c885d1ffa
> ql/src/test/results/clientpositive/llap/smb_mapjoin_14.q.out c334b9386b
> ql/src/test/results/clientpositive/llap/smb_mapjoin_15.q.out 21aac455f2
> ql/src/test/results/clientpositive/llap/smb_mapjoin_4.q.out 4b8728fbff
> ql/src/test/results/clientpositive/llap/smb_mapjoin_5.q.out a1313696f0
> ql/src/test/results/clientpositive/llap/smb_mapjoin_6.q.out f44a0dbc70
> ql/src/test/results/clientpositive/llap/subquery_in_having.q.out c9956121f8
> ql/src/test/results/clientpositive/llap/subquery_notin.q.out d72e8c349c
> ql/src/test/results/clientpositive/llap/vectorized_bucketmapjoin1.q.out 61c5051bb9
> ql/src/test/results/clientpositive/spark/bucketmapjoin1.q.out a79a8c466a
> ql/src/test/results/clientpositive/spark/smb_mapjoin_14.q.out 1fd4490ac4
> ql/src/test/results/clientpositive/spark/smb_mapjoin_15.q.out 6ca577fdbb
> ql/src/test/results/clientpositive/spark/smb_mapjoin_4.q.out 629a6c428a
> ql/src/test/results/clientpositive/spark/smb_mapjoin_5.q.out 7d0934010e
> ql/src/test/results/clientpositive/spark/smb_mapjoin_6.q.out 7445135159
> ql/src/test/results/clientpositive/spark/subquery_notin.q.out ea473c3b40
>
>
> Diff: https://reviews.apache.org/r/67296/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Deepak Jaiswal
>
>
Re: Review Request 67296: HIVE-18875 : Enable SMB Join by default in
Tez
Posted by Deepak Jaiswal <dj...@hortonworks.com>.
> On June 5, 2018, 3:21 a.m., Gunther Hagleitner wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java
> > Lines 614 (patched)
> > <https://reviews.apache.org/r/67296/diff/3/?file=2034852#file2034852line614>
> >
> > I still don't think this code should be here. This seems to be doing the exact same thing as the "checkColEquality" below. If not can you tell me how this is different and why the calls below don't suffice? Otherwise let's remove this?
>
> Deepak Jaiswal wrote:
> I will see if this code can be removed.
I did some testing and there are cases where sort col in optraits and key col on which data is sorted are different which never gets checked without this code.
checkColEquality looks at optraits only.
The optraits of RSOp used to have the key cols as sort cols but that changed for bucket mapjoin. Looks like we need this change afterall.
- Deepak
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67296/#review204301
-----------------------------------------------------------
On June 4, 2018, 5:38 a.m., Deepak Jaiswal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67296/
> -----------------------------------------------------------
>
> (Updated June 4, 2018, 5:38 a.m.)
>
>
> Review request for hive, Gunther Hagleitner and Jason Dere.
>
>
> Bugs: HIVE-18875
> https://issues.apache.org/jira/browse/HIVE-18875
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Fixed various issues with SMB, mostly on the Reducer side join.
> GBY Op now uses inputObjectInspector[0] all the time as it is the only OI it has. The tag is irrelevant here. Was causing problem with SMB.
> Disabled SMB in spark on hive tests as the same config for Tez was enabling it there.
> Some SMB specific tests were designed to first run without SMB and then with SMB. With SMB enabled by default, it is explicitely turned off to make sure the behavior is maintained.
>
> Please go through JIRA comments as they may clear out some questions.
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 3295d1dbc5
> ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java aefaa0586e
> ql/src/java/org/apache/hadoop/hive/ql/exec/GroupByOperator.java 4b766382ef
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java 4019f132d3
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/metainfo/annotation/OpTraitsRulesProcFactory.java 9e5446566b
> ql/src/test/queries/clientpositive/auto_sortmerge_join_11.q 7416eb0ec0
> ql/src/test/queries/clientpositive/skewjoinopt19.q 02cadda7f5
> ql/src/test/queries/clientpositive/skewjoinopt20.q 160e5b82d9
> ql/src/test/queries/clientpositive/smb_mapjoin_11.q 6ce49b83c2
> ql/src/test/queries/clientpositive/smb_mapjoin_12.q 753e4d3c9a
> ql/src/test/queries/clientpositive/smb_mapjoin_17.q d68f5f3139
> ql/src/test/queries/clientpositive/subquery_notin.q 64940277bb
> ql/src/test/results/clientpositive/llap/correlationoptimizer2.q.out 0f839ead0e
> ql/src/test/results/clientpositive/llap/correlationoptimizer6.q.out 499ef4b178
> ql/src/test/results/clientpositive/llap/explainuser_1.q.out 0c339e5c8f
> ql/src/test/results/clientpositive/llap/limit_pushdown.q.out 76fae9a152
> ql/src/test/results/clientpositive/llap/mergejoin.q.out 832ed487ec
> ql/src/test/results/clientpositive/llap/mrr.q.out 737c73893f
> ql/src/test/results/clientpositive/llap/offset_limit_ppd_optimizer.q.out 66460271b4
> ql/src/test/results/clientpositive/llap/smb_cache.q.out 7c885d1ffa
> ql/src/test/results/clientpositive/llap/smb_mapjoin_14.q.out c334b9386b
> ql/src/test/results/clientpositive/llap/smb_mapjoin_15.q.out 21aac455f2
> ql/src/test/results/clientpositive/llap/smb_mapjoin_4.q.out 4b8728fbff
> ql/src/test/results/clientpositive/llap/smb_mapjoin_5.q.out a1313696f0
> ql/src/test/results/clientpositive/llap/smb_mapjoin_6.q.out f44a0dbc70
> ql/src/test/results/clientpositive/llap/subquery_in_having.q.out c9956121f8
> ql/src/test/results/clientpositive/llap/subquery_notin.q.out d72e8c349c
> ql/src/test/results/clientpositive/llap/vectorized_bucketmapjoin1.q.out 61c5051bb9
> ql/src/test/results/clientpositive/spark/bucketmapjoin1.q.out a79a8c466a
> ql/src/test/results/clientpositive/spark/smb_mapjoin_14.q.out 1fd4490ac4
> ql/src/test/results/clientpositive/spark/smb_mapjoin_15.q.out 6ca577fdbb
> ql/src/test/results/clientpositive/spark/smb_mapjoin_4.q.out 629a6c428a
> ql/src/test/results/clientpositive/spark/smb_mapjoin_5.q.out 7d0934010e
> ql/src/test/results/clientpositive/spark/smb_mapjoin_6.q.out 7445135159
> ql/src/test/results/clientpositive/spark/subquery_notin.q.out ea473c3b40
>
>
> Diff: https://reviews.apache.org/r/67296/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Deepak Jaiswal
>
>
Re: Review Request 67296: HIVE-18875 : Enable SMB Join by default in
Tez
Posted by Deepak Jaiswal <dj...@hortonworks.com>.
> On June 5, 2018, 3:21 a.m., Gunther Hagleitner wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java
> > Lines 172 (patched)
> > <https://reviews.apache.org/r/67296/diff/3/?file=2034850#file2034850line172>
> >
> > It seems you fixed the same problem twice now. Once by fixing the close logic, and a second time with this. Did the close logic by itself not suffice?
> >
> > I prefer the fix in the join operator to be honest. For multiple reasons:
> >
> > a) This is a lot of new code.
> > b) The code assumes a lot about surrounding operators, that can easily break when you add new code paths.
> > c) Fixing it in the group by operator seems wrong. What if other operators flush on close? PTF? other joins? This seems brittle.
> >
> > Can you go back to the original exec fix? Was there an issue with it?
The fix in join op is not sufficient. I have multiple scenarios in which it broke that too with a lot of additional fixes (See Patch 11). The reason being, the join op is written with shuffle join in mind first where it is the top operator in the Reducer. If it pulls a row, it gets it immediately, whereas a reduce side SMB does not. A parent GBY Op has it and it depends on reading the next group for pushing the current group. This behavior differs from regular shuffle. The numerous fixes are just patching one case at a time which makes this code really ugly, bug prone and inefficient due to too much branching.
Inorder to fix this properly in Join Op, the correct fix would be to go back to drawing board and rewrite the entire thing.
a) The new code is very specific to SMB on reduce side and only applies to it. This makes sure it mimics the state machine assumed by shuffle join. Infact, I think the fix in close() may not be even needed, but I need to verify that.
b) The assumptions are very rigid. A lot of conditions have to be met inorder to set reduceSMB true, however, I think, we can do this at compile time when SMB Op is created right after DummyOp is created. If a new code path is added, it has to make sure existing ones don't break.
c) The rows are forwarded as they are read. Once a row is pushed out of GBY, it is pushed out until it reaches JoinOp. Good point about PTF, I think we need to do similar thing in PTF as well. There is no other known case for reduce side SMB.
As mentioned above, the fix before this is a giant pool of patches which can run clean on ptests but can break on very simple and small tests which I plan to add once this code is green lit.
> On June 5, 2018, 3:21 a.m., Gunther Hagleitner wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java
> > Lines 313 (patched)
> > <https://reviews.apache.org/r/67296/diff/3/?file=2034850#file2034850line319>
> >
> > introducing method calls in the inner loop can have negative perf implications, are you sure this won't hurt?
Thanks for pointing this out, its a remnant of the earlier patch which needs to go away.
> On June 5, 2018, 3:21 a.m., Gunther Hagleitner wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/GroupByOperator.java
> > Lines 733 (patched)
> > <https://reviews.apache.org/r/67296/diff/3/?file=2034851#file2034851line733>
> >
> > this adds another branch in the inner loop also. might have perf implications.
Sure, will find a way to avoid it.
> On June 5, 2018, 3:21 a.m., Gunther Hagleitner wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/GroupByOperator.java
> > Lines 906 (patched)
> > <https://reviews.apache.org/r/67296/diff/3/?file=2034851#file2034851line906>
> >
> > see other comment. this is a lot of new code - and unnecessary if you've already fixed it in join.
Explained in 1st comment.
> On June 5, 2018, 3:21 a.m., Gunther Hagleitner wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java
> > Lines 614 (patched)
> > <https://reviews.apache.org/r/67296/diff/3/?file=2034852#file2034852line614>
> >
> > I still don't think this code should be here. This seems to be doing the exact same thing as the "checkColEquality" below. If not can you tell me how this is different and why the calls below don't suffice? Otherwise let's remove this?
I will see if this code can be removed.
> On June 5, 2018, 3:21 a.m., Gunther Hagleitner wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/metainfo/annotation/OpTraitsRulesProcFactory.java
> > Lines 265 (patched)
> > <https://reviews.apache.org/r/67296/diff/3/?file=2034853#file2034853line265>
> >
> > I don't think this comment should be here. In this class you should be explaining what the rules are not what certain situations you ran in on the execution side. Can you please remove?
I will rewrite it.
> On June 5, 2018, 3:21 a.m., Gunther Hagleitner wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/metainfo/annotation/OpTraitsRulesProcFactory.java
> > Lines 299 (patched)
> > <https://reviews.apache.org/r/67296/diff/3/?file=2034853#file2034853line299>
> >
> > This is a nit, feel free to ignore: I don't think it's necessarily a "parent gby" that creates the bucketing. Just drop after "than".
Sure, thanks.
- Deepak
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67296/#review204301
-----------------------------------------------------------
On June 4, 2018, 5:38 a.m., Deepak Jaiswal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67296/
> -----------------------------------------------------------
>
> (Updated June 4, 2018, 5:38 a.m.)
>
>
> Review request for hive, Gunther Hagleitner and Jason Dere.
>
>
> Bugs: HIVE-18875
> https://issues.apache.org/jira/browse/HIVE-18875
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Fixed various issues with SMB, mostly on the Reducer side join.
> GBY Op now uses inputObjectInspector[0] all the time as it is the only OI it has. The tag is irrelevant here. Was causing problem with SMB.
> Disabled SMB in spark on hive tests as the same config for Tez was enabling it there.
> Some SMB specific tests were designed to first run without SMB and then with SMB. With SMB enabled by default, it is explicitely turned off to make sure the behavior is maintained.
>
> Please go through JIRA comments as they may clear out some questions.
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 3295d1dbc5
> ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java aefaa0586e
> ql/src/java/org/apache/hadoop/hive/ql/exec/GroupByOperator.java 4b766382ef
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java 4019f132d3
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/metainfo/annotation/OpTraitsRulesProcFactory.java 9e5446566b
> ql/src/test/queries/clientpositive/auto_sortmerge_join_11.q 7416eb0ec0
> ql/src/test/queries/clientpositive/skewjoinopt19.q 02cadda7f5
> ql/src/test/queries/clientpositive/skewjoinopt20.q 160e5b82d9
> ql/src/test/queries/clientpositive/smb_mapjoin_11.q 6ce49b83c2
> ql/src/test/queries/clientpositive/smb_mapjoin_12.q 753e4d3c9a
> ql/src/test/queries/clientpositive/smb_mapjoin_17.q d68f5f3139
> ql/src/test/queries/clientpositive/subquery_notin.q 64940277bb
> ql/src/test/results/clientpositive/llap/correlationoptimizer2.q.out 0f839ead0e
> ql/src/test/results/clientpositive/llap/correlationoptimizer6.q.out 499ef4b178
> ql/src/test/results/clientpositive/llap/explainuser_1.q.out 0c339e5c8f
> ql/src/test/results/clientpositive/llap/limit_pushdown.q.out 76fae9a152
> ql/src/test/results/clientpositive/llap/mergejoin.q.out 832ed487ec
> ql/src/test/results/clientpositive/llap/mrr.q.out 737c73893f
> ql/src/test/results/clientpositive/llap/offset_limit_ppd_optimizer.q.out 66460271b4
> ql/src/test/results/clientpositive/llap/smb_cache.q.out 7c885d1ffa
> ql/src/test/results/clientpositive/llap/smb_mapjoin_14.q.out c334b9386b
> ql/src/test/results/clientpositive/llap/smb_mapjoin_15.q.out 21aac455f2
> ql/src/test/results/clientpositive/llap/smb_mapjoin_4.q.out 4b8728fbff
> ql/src/test/results/clientpositive/llap/smb_mapjoin_5.q.out a1313696f0
> ql/src/test/results/clientpositive/llap/smb_mapjoin_6.q.out f44a0dbc70
> ql/src/test/results/clientpositive/llap/subquery_in_having.q.out c9956121f8
> ql/src/test/results/clientpositive/llap/subquery_notin.q.out d72e8c349c
> ql/src/test/results/clientpositive/llap/vectorized_bucketmapjoin1.q.out 61c5051bb9
> ql/src/test/results/clientpositive/spark/bucketmapjoin1.q.out a79a8c466a
> ql/src/test/results/clientpositive/spark/smb_mapjoin_14.q.out 1fd4490ac4
> ql/src/test/results/clientpositive/spark/smb_mapjoin_15.q.out 6ca577fdbb
> ql/src/test/results/clientpositive/spark/smb_mapjoin_4.q.out 629a6c428a
> ql/src/test/results/clientpositive/spark/smb_mapjoin_5.q.out 7d0934010e
> ql/src/test/results/clientpositive/spark/smb_mapjoin_6.q.out 7445135159
> ql/src/test/results/clientpositive/spark/subquery_notin.q.out ea473c3b40
>
>
> Diff: https://reviews.apache.org/r/67296/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Deepak Jaiswal
>
>
Re: Review Request 67296: HIVE-18875 : Enable SMB Join by default in
Tez
Posted by Gunther Hagleitner <gu...@apache.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67296/#review204301
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java
Lines 172 (patched)
<https://reviews.apache.org/r/67296/#comment286714>
It seems you fixed the same problem twice now. Once by fixing the close logic, and a second time with this. Did the close logic by itself not suffice?
I prefer the fix in the join operator to be honest. For multiple reasons:
a) This is a lot of new code.
b) The code assumes a lot about surrounding operators, that can easily break when you add new code paths.
c) Fixing it in the group by operator seems wrong. What if other operators flush on close? PTF? other joins? This seems brittle.
Can you go back to the original exec fix? Was there an issue with it?
ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java
Lines 313 (patched)
<https://reviews.apache.org/r/67296/#comment286713>
introducing method calls in the inner loop can have negative perf implications, are you sure this won't hurt?
ql/src/java/org/apache/hadoop/hive/ql/exec/GroupByOperator.java
Lines 733 (patched)
<https://reviews.apache.org/r/67296/#comment286718>
this adds another branch in the inner loop also. might have perf implications.
ql/src/java/org/apache/hadoop/hive/ql/exec/GroupByOperator.java
Lines 906 (patched)
<https://reviews.apache.org/r/67296/#comment286719>
see other comment. this is a lot of new code - and unnecessary if you've already fixed it in join.
ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java
Lines 614 (patched)
<https://reviews.apache.org/r/67296/#comment286712>
I still don't think this code should be here. This seems to be doing the exact same thing as the "checkColEquality" below. If not can you tell me how this is different and why the calls below don't suffice? Otherwise let's remove this?
ql/src/java/org/apache/hadoop/hive/ql/optimizer/metainfo/annotation/OpTraitsRulesProcFactory.java
Lines 265 (patched)
<https://reviews.apache.org/r/67296/#comment286715>
I don't think this comment should be here. In this class you should be explaining what the rules are not what certain situations you ran in on the execution side. Can you please remove?
ql/src/java/org/apache/hadoop/hive/ql/optimizer/metainfo/annotation/OpTraitsRulesProcFactory.java
Lines 299 (patched)
<https://reviews.apache.org/r/67296/#comment286716>
This is a nit, feel free to ignore: I don't think it's necessarily a "parent gby" that creates the bucketing. Just drop after "than".
- Gunther Hagleitner
On June 4, 2018, 5:38 a.m., Deepak Jaiswal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67296/
> -----------------------------------------------------------
>
> (Updated June 4, 2018, 5:38 a.m.)
>
>
> Review request for hive, Gunther Hagleitner and Jason Dere.
>
>
> Bugs: HIVE-18875
> https://issues.apache.org/jira/browse/HIVE-18875
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Fixed various issues with SMB, mostly on the Reducer side join.
> GBY Op now uses inputObjectInspector[0] all the time as it is the only OI it has. The tag is irrelevant here. Was causing problem with SMB.
> Disabled SMB in spark on hive tests as the same config for Tez was enabling it there.
> Some SMB specific tests were designed to first run without SMB and then with SMB. With SMB enabled by default, it is explicitely turned off to make sure the behavior is maintained.
>
> Please go through JIRA comments as they may clear out some questions.
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 3295d1dbc5
> ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java aefaa0586e
> ql/src/java/org/apache/hadoop/hive/ql/exec/GroupByOperator.java 4b766382ef
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java 4019f132d3
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/metainfo/annotation/OpTraitsRulesProcFactory.java 9e5446566b
> ql/src/test/queries/clientpositive/auto_sortmerge_join_11.q 7416eb0ec0
> ql/src/test/queries/clientpositive/skewjoinopt19.q 02cadda7f5
> ql/src/test/queries/clientpositive/skewjoinopt20.q 160e5b82d9
> ql/src/test/queries/clientpositive/smb_mapjoin_11.q 6ce49b83c2
> ql/src/test/queries/clientpositive/smb_mapjoin_12.q 753e4d3c9a
> ql/src/test/queries/clientpositive/smb_mapjoin_17.q d68f5f3139
> ql/src/test/queries/clientpositive/subquery_notin.q 64940277bb
> ql/src/test/results/clientpositive/llap/correlationoptimizer2.q.out 0f839ead0e
> ql/src/test/results/clientpositive/llap/correlationoptimizer6.q.out 499ef4b178
> ql/src/test/results/clientpositive/llap/explainuser_1.q.out 0c339e5c8f
> ql/src/test/results/clientpositive/llap/limit_pushdown.q.out 76fae9a152
> ql/src/test/results/clientpositive/llap/mergejoin.q.out 832ed487ec
> ql/src/test/results/clientpositive/llap/mrr.q.out 737c73893f
> ql/src/test/results/clientpositive/llap/offset_limit_ppd_optimizer.q.out 66460271b4
> ql/src/test/results/clientpositive/llap/smb_cache.q.out 7c885d1ffa
> ql/src/test/results/clientpositive/llap/smb_mapjoin_14.q.out c334b9386b
> ql/src/test/results/clientpositive/llap/smb_mapjoin_15.q.out 21aac455f2
> ql/src/test/results/clientpositive/llap/smb_mapjoin_4.q.out 4b8728fbff
> ql/src/test/results/clientpositive/llap/smb_mapjoin_5.q.out a1313696f0
> ql/src/test/results/clientpositive/llap/smb_mapjoin_6.q.out f44a0dbc70
> ql/src/test/results/clientpositive/llap/subquery_in_having.q.out c9956121f8
> ql/src/test/results/clientpositive/llap/subquery_notin.q.out d72e8c349c
> ql/src/test/results/clientpositive/llap/vectorized_bucketmapjoin1.q.out 61c5051bb9
> ql/src/test/results/clientpositive/spark/bucketmapjoin1.q.out a79a8c466a
> ql/src/test/results/clientpositive/spark/smb_mapjoin_14.q.out 1fd4490ac4
> ql/src/test/results/clientpositive/spark/smb_mapjoin_15.q.out 6ca577fdbb
> ql/src/test/results/clientpositive/spark/smb_mapjoin_4.q.out 629a6c428a
> ql/src/test/results/clientpositive/spark/smb_mapjoin_5.q.out 7d0934010e
> ql/src/test/results/clientpositive/spark/smb_mapjoin_6.q.out 7445135159
> ql/src/test/results/clientpositive/spark/subquery_notin.q.out ea473c3b40
>
>
> Diff: https://reviews.apache.org/r/67296/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Deepak Jaiswal
>
>
Re: Review Request 67296: HIVE-18875 : Enable SMB Join by default in
Tez
Posted by Deepak Jaiswal <dj...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67296/
-----------------------------------------------------------
(Updated June 4, 2018, 5:38 a.m.)
Review request for hive, Gunther Hagleitner and Jason Dere.
Changes
-------
This patch departs from previous ones on execution side.
Once established that the GBY Op is part of reduce side SMB, it forwards the row immediately instead of caching it until a row from next group is seen.
This can be done safely because in mergepartial phase of GBY Op for SMB, each row belongs to separate group. This helps mimic same behavior as existing shuffle join and avoids endless patching of state machine instead.
Bugs: HIVE-18875
https://issues.apache.org/jira/browse/HIVE-18875
Repository: hive-git
Description
-------
Fixed various issues with SMB, mostly on the Reducer side join.
GBY Op now uses inputObjectInspector[0] all the time as it is the only OI it has. The tag is irrelevant here. Was causing problem with SMB.
Disabled SMB in spark on hive tests as the same config for Tez was enabling it there.
Some SMB specific tests were designed to first run without SMB and then with SMB. With SMB enabled by default, it is explicitely turned off to make sure the behavior is maintained.
Please go through JIRA comments as they may clear out some questions.
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 3295d1dbc5
ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java aefaa0586e
ql/src/java/org/apache/hadoop/hive/ql/exec/GroupByOperator.java 4b766382ef
ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java 4019f132d3
ql/src/java/org/apache/hadoop/hive/ql/optimizer/metainfo/annotation/OpTraitsRulesProcFactory.java 9e5446566b
ql/src/test/queries/clientpositive/auto_sortmerge_join_11.q 7416eb0ec0
ql/src/test/queries/clientpositive/skewjoinopt19.q 02cadda7f5
ql/src/test/queries/clientpositive/skewjoinopt20.q 160e5b82d9
ql/src/test/queries/clientpositive/smb_mapjoin_11.q 6ce49b83c2
ql/src/test/queries/clientpositive/smb_mapjoin_12.q 753e4d3c9a
ql/src/test/queries/clientpositive/smb_mapjoin_17.q d68f5f3139
ql/src/test/queries/clientpositive/subquery_notin.q 64940277bb
ql/src/test/results/clientpositive/llap/correlationoptimizer2.q.out 0f839ead0e
ql/src/test/results/clientpositive/llap/correlationoptimizer6.q.out 499ef4b178
ql/src/test/results/clientpositive/llap/explainuser_1.q.out 0c339e5c8f
ql/src/test/results/clientpositive/llap/limit_pushdown.q.out 76fae9a152
ql/src/test/results/clientpositive/llap/mergejoin.q.out 832ed487ec
ql/src/test/results/clientpositive/llap/mrr.q.out 737c73893f
ql/src/test/results/clientpositive/llap/offset_limit_ppd_optimizer.q.out 66460271b4
ql/src/test/results/clientpositive/llap/smb_cache.q.out 7c885d1ffa
ql/src/test/results/clientpositive/llap/smb_mapjoin_14.q.out c334b9386b
ql/src/test/results/clientpositive/llap/smb_mapjoin_15.q.out 21aac455f2
ql/src/test/results/clientpositive/llap/smb_mapjoin_4.q.out 4b8728fbff
ql/src/test/results/clientpositive/llap/smb_mapjoin_5.q.out a1313696f0
ql/src/test/results/clientpositive/llap/smb_mapjoin_6.q.out f44a0dbc70
ql/src/test/results/clientpositive/llap/subquery_in_having.q.out c9956121f8
ql/src/test/results/clientpositive/llap/subquery_notin.q.out d72e8c349c
ql/src/test/results/clientpositive/llap/vectorized_bucketmapjoin1.q.out 61c5051bb9
ql/src/test/results/clientpositive/spark/bucketmapjoin1.q.out a79a8c466a
ql/src/test/results/clientpositive/spark/smb_mapjoin_14.q.out 1fd4490ac4
ql/src/test/results/clientpositive/spark/smb_mapjoin_15.q.out 6ca577fdbb
ql/src/test/results/clientpositive/spark/smb_mapjoin_4.q.out 629a6c428a
ql/src/test/results/clientpositive/spark/smb_mapjoin_5.q.out 7d0934010e
ql/src/test/results/clientpositive/spark/smb_mapjoin_6.q.out 7445135159
ql/src/test/results/clientpositive/spark/subquery_notin.q.out ea473c3b40
Diff: https://reviews.apache.org/r/67296/diff/3/
Changes: https://reviews.apache.org/r/67296/diff/2-3/
Testing
-------
Thanks,
Deepak Jaiswal
Re: Review Request 67296: HIVE-18875 : Enable SMB Join by default in
Tez
Posted by Deepak Jaiswal <dj...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67296/
-----------------------------------------------------------
(Updated May 25, 2018, 3:06 a.m.)
Review request for hive, Gunther Hagleitner and Jason Dere.
Changes
-------
Updated result files.
Bugs: HIVE-18875
https://issues.apache.org/jira/browse/HIVE-18875
Repository: hive-git
Description
-------
Fixed various issues with SMB, mostly on the Reducer side join.
GBY Op now uses inputObjectInspector[0] all the time as it is the only OI it has. The tag is irrelevant here. Was causing problem with SMB.
Disabled SMB in spark on hive tests as the same config for Tez was enabling it there.
Some SMB specific tests were designed to first run without SMB and then with SMB. With SMB enabled by default, it is explicitely turned off to make sure the behavior is maintained.
Please go through JIRA comments as they may clear out some questions.
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 931533a556
ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java aefaa0586e
ql/src/java/org/apache/hadoop/hive/ql/exec/GroupByOperator.java 4b766382ef
ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java 4019f132d3
ql/src/java/org/apache/hadoop/hive/ql/optimizer/metainfo/annotation/OpTraitsRulesProcFactory.java 9e5446566b
ql/src/test/queries/clientpositive/auto_sortmerge_join_11.q 7416eb0ec0
ql/src/test/queries/clientpositive/skewjoinopt19.q 02cadda7f5
ql/src/test/queries/clientpositive/skewjoinopt20.q 160e5b82d9
ql/src/test/queries/clientpositive/smb_mapjoin_11.q 6ce49b83c2
ql/src/test/queries/clientpositive/smb_mapjoin_12.q 753e4d3c9a
ql/src/test/queries/clientpositive/smb_mapjoin_17.q d68f5f3139
ql/src/test/queries/clientpositive/subquery_notin.q 64940277bb
ql/src/test/results/clientpositive/llap/bucketsortoptimize_insert_6.q.out 7533cf8579
ql/src/test/results/clientpositive/llap/correlationoptimizer2.q.out 0f839ead0e
ql/src/test/results/clientpositive/llap/correlationoptimizer6.q.out 499ef4b178
ql/src/test/results/clientpositive/llap/explainuser_1.q.out f4852d2f15
ql/src/test/results/clientpositive/llap/limit_pushdown.q.out 79311d0415
ql/src/test/results/clientpositive/llap/mergejoin.q.out 832ed487ec
ql/src/test/results/clientpositive/llap/mrr.q.out 737c73893f
ql/src/test/results/clientpositive/llap/offset_limit_ppd_optimizer.q.out 09a120ae12
ql/src/test/results/clientpositive/llap/quotedid_smb.q.out 9c271a7958
ql/src/test/results/clientpositive/llap/smb_cache.q.out 7c885d1ffa
ql/src/test/results/clientpositive/llap/smb_mapjoin_14.q.out c334b9386b
ql/src/test/results/clientpositive/llap/smb_mapjoin_15.q.out 21aac455f2
ql/src/test/results/clientpositive/llap/smb_mapjoin_4.q.out 4b8728fbff
ql/src/test/results/clientpositive/llap/smb_mapjoin_5.q.out a1313696f0
ql/src/test/results/clientpositive/llap/smb_mapjoin_6.q.out f44a0dbc70
ql/src/test/results/clientpositive/llap/subquery_in_having.q.out c9956121f8
ql/src/test/results/clientpositive/llap/subquery_notin.q.out d72e8c349c
ql/src/test/results/clientpositive/llap/subquery_views.q.out 2c8530933c
ql/src/test/results/clientpositive/llap/vector_groupby_grouping_sets4.q.out 285c154f3b
ql/src/test/results/clientpositive/llap/vectorized_bucketmapjoin1.q.out 61c5051bb9
ql/src/test/results/clientpositive/spark/bucketmapjoin1.q.out a79a8c466a
ql/src/test/results/clientpositive/spark/smb_mapjoin_14.q.out 1fd4490ac4
ql/src/test/results/clientpositive/spark/smb_mapjoin_15.q.out 6ca577fdbb
ql/src/test/results/clientpositive/spark/smb_mapjoin_4.q.out 629a6c428a
ql/src/test/results/clientpositive/spark/smb_mapjoin_5.q.out 7d0934010e
ql/src/test/results/clientpositive/spark/smb_mapjoin_6.q.out 7445135159
ql/src/test/results/clientpositive/spark/subquery_notin.q.out ea473c3b40
Diff: https://reviews.apache.org/r/67296/diff/2/
Changes: https://reviews.apache.org/r/67296/diff/1-2/
Testing
-------
Thanks,
Deepak Jaiswal