----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4670/#review7484 -----------------------------------------------------------
This review took a little time as I got interrupted in the middle. :) Great job overall. Some comments are about refactoring a little part of the code your modifying. Let me know what you think. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/optimizer/AllExpressionVisitor.java <https://reviews.apache.org/r/4670/#comment16557> iterate on entries() instead http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/optimizer/AllExpressionVisitor.java <https://reviews.apache.org/r/4670/#comment16561> All those for loops could be factored into a private visitAll(Collection<LogicalExpressionPlan>) http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/optimizer/SchemaResetter.java <https://reviews.apache.org/r/4670/#comment16562> same comment. You can factor out the for loops in those methods. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LOCube.java <https://reviews.apache.org/r/4670/#comment16563> chain all exceptions http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LOCube.java <https://reviews.apache.org/r/4670/#comment16564> same http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AstValidator.g <https://reviews.apache.org/r/4670/#comment16566> do you know if there's a way to avoid duplication across .g files? http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java <https://reviews.apache.org/r/4670/#comment16567> you can make this a javadoc comment http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java <https://reviews.apache.org/r/4670/#comment16568> this is not affected to anything. Is it intended? http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java <https://reviews.apache.org/r/4670/#comment16569> same comment. I guess it is intended, but it looks strange. If the constructor has side effect, maybe there should be another way http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java <https://reviews.apache.org/r/4670/#comment16776> add the name of the duplicate in the error message http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanGenerator.g <https://reviews.apache.org/r/4670/#comment16777> please add some comments to explain what you're doing here http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestCubeOperator.java <https://reviews.apache.org/r/4670/#comment16778> you van now use mock.Storage() for those tests. see PIG-2650 http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestCubeOperator.java <https://reviews.apache.org/r/4670/#comment16779> Use the message part of the assert statement to ensure a good error message when it fails. Tuple t = it.next() assertTrue(expected+" contains "+t, expected.contains(t)) - Julien On 2012-04-30 04:05:03, Prasanth_J wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4670/ > ----------------------------------------------------------- > > (Updated 2012-04-30 04:05:03) > > > Review request for pig and Dmitriy Ryaboy. > > > Summary > ------- > > This is a review board for https://issues.apache.org/jira/browse/PIG-2167 > > > This addresses bug PIG-2167. > https://issues.apache.org/jira/browse/PIG-2167 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/optimizer/AllExpressionVisitor.java > 1325115 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/optimizer/SchemaResetter.java > 1325115 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LOCube.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LogicalRelationalNodesVisitor.java > 1325115 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/visitor/ProjectStarExpander.java > 1325115 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AliasMasker.g > 1325115 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AstPrinter.g > 1325115 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AstValidator.g > 1325115 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java > 1325115 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanGenerator.g > 1325115 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/QueryLexer.g > 1325115 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/QueryParser.g > 1325115 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestLexer.pig > 1325115 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestLogicalPlanGenerator.java > 1325115 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestParser.pig > 1325115 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestQueryLexer.java > 1325115 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestQueryParser.java > 1325115 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestCubeOperator.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/4670/diff > > > Testing > ------- > > Unit tests: All passed > > Pre-commit tests: All passed > ant clean test-commit > > > Thanks, > > Prasanth_J > >
