----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15194/#review28080 -----------------------------------------------------------
Looks good overall. I made a few minor comments below: src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/POSimpleTezLoad.java <https://reviews.apache.org/r/15194/#comment54652> Please remove white spaces in the patch. src/org/apache/pig/backend/hadoop/executionengine/tez/POShuffleTezLoad.java <https://reviews.apache.org/r/15194/#comment54642> Do we need this field? It doesn't seem used anywhere. src/org/apache/pig/backend/hadoop/executionengine/tez/POShuffleTezLoad.java <https://reviews.apache.org/r/15194/#comment54643> Private? src/org/apache/pig/backend/hadoop/executionengine/tez/POShuffleTezLoad.java <https://reviews.apache.org/r/15194/#comment54644> Private? src/org/apache/pig/backend/hadoop/executionengine/tez/POShuffleTezLoad.java <https://reviews.apache.org/r/15194/#comment54645> This constructor doesn't seem needed. Can we delete it? src/org/apache/pig/backend/hadoop/executionengine/tez/POShuffleTezLoad.java <https://reviews.apache.org/r/15194/#comment54659> Perhaps we add a copy constructor to POPackage and call it here? Just a thought. src/org/apache/pig/backend/hadoop/executionengine/tez/POShuffleTezLoad.java <https://reviews.apache.org/r/15194/#comment54647> Please remove unnecessary empty lines. src/org/apache/pig/backend/hadoop/executionengine/tez/POShuffleTezLoad.java <https://reviews.apache.org/r/15194/#comment54646> Can you run two for loops inside a single try-catch block instead of duplicating it? src/org/apache/pig/backend/hadoop/executionengine/tez/POShuffleTezLoad.java <https://reviews.apache.org/r/15194/#comment54648> If you keep the index of minimum instead of minimum itself, then you don't need to call comparator.compare() here, no? src/org/apache/pig/backend/hadoop/executionengine/tez/POShuffleTezLoad.java <https://reviews.apache.org/r/15194/#comment54649> Do we need this? Doesn't seem used anywhere. src/org/apache/pig/backend/hadoop/executionengine/tez/POShuffleTezLoad.java <https://reviews.apache.org/r/15194/#comment54650> Do we need this? Doesn't seem used anywhere. src/org/apache/pig/backend/hadoop/executionengine/tez/POShuffleTezLoad.java <https://reviews.apache.org/r/15194/#comment54651> You can initialize inputKeys in declaration and remove these lines, can't you? src/org/apache/pig/backend/hadoop/executionengine/tez/TezDagBuilder.java <https://reviews.apache.org/r/15194/#comment54660> Can't you do "tezOp.plan.replace(pack, shuffleLoad)" instead of remove and add? Then, you don't need to copy the succsList and re-connect them to shuffleLoad at all, no? src/org/apache/pig/backend/hadoop/executionengine/tez/TezDagBuilder.java <https://reviews.apache.org/r/15194/#comment54655> Remove. src/org/apache/pig/backend/hadoop/executionengine/tez/TezDagBuilder.java <https://reviews.apache.org/r/15194/#comment54658> If we use tezOp.plan.replace() below, we don't need to copy the ldSucs, no? src/org/apache/pig/backend/hadoop/executionengine/tez/TezDagBuilder.java <https://reviews.apache.org/r/15194/#comment54656> Remove commented out code. src/org/apache/pig/backend/hadoop/executionengine/tez/TezDagBuilder.java <https://reviews.apache.org/r/15194/#comment54657> Looks like this can be rewritten as follows: POSimpleTezLoad tezLoad = new POSimpleTezLoad(new OperatorKey(scope , nig.getNextNodeId(scope))); tezLoad.setInputKey(ld.getOperatorKey().toString()); tezOp.plan.replace(ld, tezLoad); - Cheolsoo Park On Nov. 2, 2013, 1:17 a.m., Mark Wagner wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15194/ > ----------------------------------------------------------- > > (Updated Nov. 2, 2013, 1:17 a.m.) > > > Review request for pig, Cheolsoo Park, Daniel Dai, and Rohini Palaniswamy. > > > Bugs: PIG-3527 > https://issues.apache.org/jira/browse/PIG-3527 > > > Repository: pig-git > > > Description > ------- > > Adds support for multiple LogicalInputs to the PigProcessor. This is done by > adding a new TezLoad interface which PhysicalOperators may implement. On the > backend, any operators implementing this interface will have the LogicalInput > attached to them. 2 implementations are included: > * POSimpleTezLoad which consumes a single MRInput > * POShuffleTezLoad which consumes one or more ShuffledMergedInputs. > The POShuffleTezLoad does a k-way merge of the shuffle inputs to package for > the operator pipeline. This required a change to the comparators used so that > the sort order remained consistent. There is also a fix to POForEach where it > was using the incorrect status code for signaling (although it produced the > same end result in the MR pipeline). > > > Diffs > ----- > > > src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigBigDecimalRawComparator.java > ddea99e > > src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigBigIntegerRawComparator.java > 5ea3fc7 > > src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigBooleanRawComparator.java > dfd4ebf > > src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigBytesRawComparator.java > 09397e5 > > src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigDateTimeRawComparator.java > a87161f > > src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigDoubleRawComparator.java > cbf457f > > src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigFloatRawComparator.java > 1d86e3f > > src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigIntRawComparator.java > bb6c9df > > src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigLongRawComparator.java > b3ded76 > > src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigSecondaryKeyComparator.java > 5ad334b > > src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigTextRawComparator.java > 022f37b > > src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigTupleDefaultRawComparator.java > 866c39d > > src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigTupleSortComparator.java > 9724b9f > > src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/POSimpleTezLoad.java > PRE-CREATION > > src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/TezLoad.java > PRE-CREATION > > src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POForEach.java > eb9f62a > > src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPackage.java > 86314d9 > > src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPackageLite.java > c200715 > src/org/apache/pig/backend/hadoop/executionengine/tez/FileInputHandler.java > d29e330 > src/org/apache/pig/backend/hadoop/executionengine/tez/InputHandler.java > d2298ca > src/org/apache/pig/backend/hadoop/executionengine/tez/POShuffleTezLoad.java > PRE-CREATION > src/org/apache/pig/backend/hadoop/executionengine/tez/PigProcessor.java > ebb3145 > > src/org/apache/pig/backend/hadoop/executionengine/tez/ShuffledInputHandler.java > d7b42b8 > src/org/apache/pig/backend/hadoop/executionengine/tez/TezDagBuilder.java > 45e47b0 > src/org/apache/pig/data/BinInterSedes.java b3ec51e > src/org/apache/pig/data/DefaultTuple.java 2e7ca5f > test/e2e/pig/tests/tez.conf 24af8d3 > > Diff: https://reviews.apache.org/r/15194/diff/ > > > Testing > ------- > > Manual testing and an e2e test has been added. Because of the comparator > change, some of the tests fail because of bag ordering. > > > Thanks, > > Mark Wagner > >
