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