> On July 19, 2013, 6:02 p.m., Ashutosh Chauhan wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/DemuxOperator.java, lines 229-231 > > <https://reviews.apache.org/r/12767/diff/1/?file=323660#file323660line229> > > > > 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?
At here, we are iterating the entrySet of newTagToOldTag. Then, we get the childIndex from newTagToChildIndex. It is possible multiple newTag can point to the same child (e.g. when the child is a JoinOperaotr). I will add comments about it. > On July 19, 2013, 6:02 p.m., Ashutosh Chauhan wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/DemuxOperator.java, lines 328-330 > > <https://reviews.apache.org/r/12767/diff/1/?file=323660#file323660line328> > > > > 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. Yes, I agree. Will remove it. > On July 19, 2013, 6:02 p.m., Ashutosh Chauhan wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/DemuxOperator.java, lines 357-358 > > <https://reviews.apache.org/r/12767/diff/1/?file=323660#file323660line357> > > > > 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. Right. This one sounds better. > On July 19, 2013, 6:02 p.m., Ashutosh Chauhan wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/MuxOperator.java, line 79 > > <https://reviews.apache.org/r/12767/diff/1/?file=323661#file323661line79> > > > > Can you also add a line in comment saying, this key-val-tag structure > > is used by JoinOperator and Groupby operators to function correctly. > > Done > On July 19, 2013, 6:02 p.m., Ashutosh Chauhan wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/MuxOperator.java, lines 258-260 > > <https://reviews.apache.org/r/12767/diff/1/?file=323661#file323661line258> > > > > Same comment as in Demux operator. I will remove it. > On July 19, 2013, 6:02 p.m., Ashutosh Chauhan wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecReducer.java, line 194 > > <https://reviews.apache.org/r/12767/diff/1/?file=323664#file323664line194> > > > > I think this should read as: > > // remove the tag from key coming out of reducer and store it in > > separate variable. Done - Yin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12767/#review23527 ----------------------------------------------------------- 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 > >
