> On Jan. 16, 2015, 5:54 p.m., Rohini Palaniswamy wrote: > > trunk/src/org/apache/pig/backend/hadoop/executionengine/tez/plan/TezCompiler.java, > > line 300 > > <https://reviews.apache.org/r/29645/diff/1/?file=808175#file808175line300> > > > > Why do we need this condition. i.e, In what case is the store operator > > predecessor of the tezOp?
I simply prevent connecting twice. If "from" and "tezOp" is already connected, skip it. > On Jan. 16, 2015, 5:54 p.m., Rohini Palaniswamy wrote: > > trunk/src/org/apache/pig/builtin/mock/Storage.java, line 462 > > <https://reviews.apache.org/r/29645/diff/1/?file=808177#file808177line462> > > > > why should we do this? Having to create a copy in a StoreFunc does not > > look good. We have to do it since the reference of the tuple will be saved in output dataset in Storage, and this tuple keeps changing if we don't make a copy > On Jan. 16, 2015, 5:54 p.m., Rohini Palaniswamy wrote: > > trunk/test/org/apache/pig/test/TestJoin.java, line 61 > > <https://reviews.apache.org/r/29645/diff/1/?file=808181#file808181line61> > > > > TestJoinLocal seems to be missing in the patch. > > > > I think we can keep TestJoin as is for cluster tests without having to > > create a abstract TestJoinBase and just create a TestJoinLocal which > > extends TestJoin and uses Local mode as all tests are same. Will help keep > > changes simple and have just two classes instead of three. Not every test is tested in both mode. Some only in cluster mode (TestJoin) and some only in local mode (TestJoinLocal). For those tested in both mode, I put into TestJoinBase > On Jan. 16, 2015, 5:54 p.m., Rohini Palaniswamy wrote: > > trunk/test/org/apache/pig/test/TestPigStatsMR.java, line 46 > > <https://reviews.apache.org/r/29645/diff/1/?file=808188#file808188line46> > > > > Make method more generic > > > > assertNumberOfJobs(ExecJob job, int expectedNumJobs) { > > .... > > > > assertEquals(expectedNumJobs, jobGraph.getJobList().size()); > > > > } This check method is very specific to the test case testPigStatsAlias. expectedNumJobs has different values in tez and mr. If we make checkPigStats more generic, then we need to make another abstract method, in MR, we check assertNumberOfJobs(job, 2), and in Tez, we check assertNumberOfJobs(job, 1) > On Jan. 16, 2015, 5:54 p.m., Rohini Palaniswamy wrote: > > trunk/test/tez-local-tests, line 72 > > <https://reviews.apache.org/r/29645/diff/1/?file=808195#file808195line72> > > > > Can we add tests in the order they will be executed? Find it easy to > > comment the already run ones and re-run if I find a failure in the middle. Sure, but I want to do it later after most of local mode tests patches check in. The file always create conflicts and need to manually edit from time to time. - Daniel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29645/#review67688 ----------------------------------------------------------- On Jan. 6, 2015, 11:49 p.m., Daniel Dai wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29645/ > ----------------------------------------------------------- > > (Updated Jan. 6, 2015, 11:49 p.m.) > > > Review request for pig and Rohini Palaniswamy. > > > Bugs: PIG-4366 > https://issues.apache.org/jira/browse/PIG-4366 > > > Repository: pig > > > Description > ------- > > See PIG-4366 > > > Diffs > ----- > > > trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POReservoirSample.java > 1649730 > > trunk/src/org/apache/pig/backend/hadoop/executionengine/tez/plan/TezCompiler.java > 1649730 > > trunk/src/org/apache/pig/backend/hadoop/executionengine/tez/plan/optimizer/MultiQueryOptimizerTez.java > 1649730 > trunk/src/org/apache/pig/builtin/mock/Storage.java 1649730 > trunk/src/org/apache/pig/tools/pigstats/tez/TezScriptState.java 1649730 > trunk/test/excluded-tests-20 1649730 > trunk/test/org/apache/pig/test/TestCubeOperator.java 1649730 > trunk/test/org/apache/pig/test/TestJoin.java 1649730 > trunk/test/org/apache/pig/test/TestJoinBase.java PRE-CREATION > trunk/test/org/apache/pig/test/TestMultiQuery.java 1649730 > trunk/test/org/apache/pig/test/TestNewPlanColumnPrune.java 1649730 > trunk/test/org/apache/pig/test/TestPigServer.java 1649730 > trunk/test/org/apache/pig/test/TestPigServerLocal.java PRE-CREATION > trunk/test/org/apache/pig/test/TestPigStats.java 1649730 > trunk/test/org/apache/pig/test/TestPigStatsMR.java PRE-CREATION > trunk/test/org/apache/pig/test/TestPruneColumn.java 1649730 > trunk/test/org/apache/pig/test/TestRank3.java 1649730 > trunk/test/org/apache/pig/test/TestScalarAliases.java 1649730 > trunk/test/org/apache/pig/test/TestScalarAliasesLocal.java PRE-CREATION > trunk/test/org/apache/pig/test/Util.java 1649730 > trunk/test/org/apache/pig/tez/TestPigStatsTez.java PRE-CREATION > trunk/test/tez-local-tests 1649730 > > Diff: https://reviews.apache.org/r/29645/diff/ > > > Testing > ------- > > > Thanks, > > Daniel Dai > >
