----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12767/#review23527 -----------------------------------------------------------
Good work, Yin! some minor comments. ql/src/java/org/apache/hadoop/hive/ql/exec/DemuxOperator.java <https://reviews.apache.org/r/12767/#comment47408> I didn't get why there is an if check here? Can you add a comment explaining in which case we need not to update this childOIs map? ql/src/java/org/apache/hadoop/hive/ql/exec/DemuxOperator.java <https://reviews.apache.org/r/12767/#comment47409> I think we should remove this if branch since its in inner loop of processing. We should put this check in initialization time of the Demux operator. Even if we cannot put it there, this will result in runtime exception which I think is fine. ql/src/java/org/apache/hadoop/hive/ql/exec/DemuxOperator.java <https://reviews.apache.org/r/12767/#comment47410> Is this better wording for this comment: // Demux operator forwards a row to exactly one child in its children list based on the tag and newTagToChildIndex in process() method, so we need not to do anything in here. ql/src/java/org/apache/hadoop/hive/ql/exec/MuxOperator.java <https://reviews.apache.org/r/12767/#comment47414> Can you also add a line in comment saying, this key-val-tag structure is used by JoinOperator and Groupby operators to function correctly. ql/src/java/org/apache/hadoop/hive/ql/exec/MuxOperator.java <https://reviews.apache.org/r/12767/#comment47411> Same comment as in Demux operator. ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecReducer.java <https://reviews.apache.org/r/12767/#comment47417> I think this should read as: // remove the tag from key coming out of reducer and store it in separate variable. - Ashutosh Chauhan On July 19, 2013, 5 p.m., Yin Huai wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/12767/ > ----------------------------------------------------------- > > (Updated July 19, 2013, 5 p.m.) > > > Review request for hive. > > > Bugs: HIVE-4877 > https://issues.apache.org/jira/browse/HIVE-4877 > > > Repository: hive-git > > > Description > ------- > > https://issues.apache.org/jira/browse/HIVE-4877 > > > Diffs > ----- > > data/files/kv1kv2.cogroup.txt 6d36e22 > ql/src/java/org/apache/hadoop/hive/ql/exec/DemuxOperator.java 9898495 > ql/src/java/org/apache/hadoop/hive/ql/exec/MuxOperator.java d4be3d9 > ql/src/java/org/apache/hadoop/hive/ql/exec/Operator.java ee76917 > ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java cbda70b > ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecReducer.java d12a53c > ql/src/test/org/apache/hadoop/hive/ql/exec/TestExecDriver.java 6a74ae4 > > Diff: https://reviews.apache.org/r/12767/diff/ > > > Testing > ------- > > Tests Failures Errors Success rate Time > 2688 2 0 99.93% 43249.945 > > Two failures are hbase_stats_empty_partition.q and ppd_key_ranges.q in > TestHBaseCliDriver. > > I manually tested these two in my mac and tests passed. > > > Thanks, > > Yin Huai > >
