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