----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5523/#review10398 -----------------------------------------------------------
http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/JobControlCompiler.java <https://reviews.apache.org/r/5523/#comment22131> small typo here 'teh' http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/JobControlCompiler.java <https://reviews.apache.org/r/5523/#comment22132> Fatal level logging of the counter size looks a bit too much. Maybe debug? http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/JobControlCompiler.java <https://reviews.apache.org/r/5523/#comment22133> Missing a space after 'counterSize ' Also, I think we need to rethrow the exception as a Pig exception here. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/MRCompiler.java <https://reviews.apache.org/r/5523/#comment22134> Typo: On case -> In case http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/MRCompiler.java <https://reviews.apache.org/r/5523/#comment22135> Typo: On case -> In case http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/MapReduceOper.java <https://reviews.apache.org/r/5523/#comment22136> A more semantically oriented comment would be better. Something like: Indicates that there is a counter operation in the MR job. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/MapReduceOper.java <https://reviews.apache.org/r/5523/#comment22137> Same here, a more semantically oriented comment would be better. Something like: Indicates that there is a rank operation in the MR job. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/MapReduceOper.java <https://reviews.apache.org/r/5523/#comment22138> Here I would comment: Indicates that there is a rank operation without sorting (row number) in the MR job. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigMapReduceCounter.java <https://reviews.apache.org/r/5523/#comment22139> On this case -> In this case http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POCounter.java <https://reviews.apache.org/r/5523/#comment22140> Can we be a bit more explicit here on what the class does? Like: This operator is part of the RANK implementation. It adds a local counter and a unique task id to each tuple. There are 2 modes of operations: regular and dense. The local counter is depends on the mode of operation. With regular rank.... With dense rank.... http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/PORank.java <https://reviews.apache.org/r/5523/#comment22141> Missing Apache license. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/PORank.java <https://reviews.apache.org/r/5523/#comment22142> I would add here the fact that this PO relies on being run in a specific MR class because it accesses the counters. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LORank.java <https://reviews.apache.org/r/5523/#comment22143> Very good http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LORank.java <https://reviews.apache.org/r/5523/#comment22144> What do you mean by 'Legal'? http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LORank.java <https://reviews.apache.org/r/5523/#comment22145> Now that I think of it, we should have some validity checks here. The operator can't be at the same time a dense rank and a row number. I would put the other flag off in the set method, i.e.: in setIsDenseRank() you also set this.isRowNumber = false; and log a warning saying that something is strange because this should not happen. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LogToPhyTranslationVisitor.java <https://reviews.apache.org/r/5523/#comment22147> On case -> In case http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LogToPhyTranslationVisitor.java <https://reviews.apache.org/r/5523/#comment22146> On case -> In case - Gianmarco De Francisci Morales On Aug. 14, 2012, 8:19 a.m., aavendan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5523/ > ----------------------------------------------------------- > > (Updated Aug. 14, 2012, 8:19 a.m.) > > > Review request for pig, aavendan and Gianmarco De Francisci Morales. > > > Description > ------- > > Review board for https://issues.apache.org/jira/browse/PIG-2353 > > > This addresses bug PIG-2353. > https://issues.apache.org/jira/browse/PIG-2353 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/JobControlCompiler.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/MRCompiler.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/MapReduceOper.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PhyPlanSetter.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigMapReduceCounter.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/plans/DotPOPrinter.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/plans/PhyPlanVisitor.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/plans/PlanPrinter.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POCounter.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/PORank.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/optimizer/AllExpressionVisitor.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/optimizer/AllSameRalationalNodesVisitor.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/optimizer/LogicalPlanPrinter.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/optimizer/SchemaResetter.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/optimizer/UidResetter.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LORank.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LogToPhyTranslationVisitor.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LogicalRelationalNodesVisitor.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/rules/ColumnPruneHelper.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/rules/ColumnPruneVisitor.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/visitor/LineageFindRelVisitor.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/visitor/ProjectStarExpander.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/visitor/SchemaAliasVisitor.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/visitor/TypeCheckingRelVisitor.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AliasMasker.g > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AstPrinter.g > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AstValidator.g > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanGenerator.g > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/QueryLexer.g > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/QueryParser.g > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/pen/IllustratorAttacher.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/pen/LocalMapReduceSimulator.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/tools/pigstats/ScriptState.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/deployers/ExistingClusterDeployer.pm > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/deployers/LocalDeployer.pm > 1372471 > http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/tests/nightly.conf > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/test/e2e/pig/tools/generate/generate_data.pl > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestLexer.pig > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestLogicalPlanGenerator.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestParser.pig > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestQueryLexer.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestQueryParser.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/OptimizeLimitPlanPrinter.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestLogicalPlanBuilder.java > 1372471 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestOrderBy3.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestRank1.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestRank2.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestRank3.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/5523/diff/ > > > Testing > ------- > > All pre-commit tests passed > > > Thanks, > > aavendan > >
