> On 2012-05-05 00:09:48, Julien Le Dem wrote: > > 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.
Thanks for the review julien. I made patch v3 based on Dmitriy's review comments but was not able to upload in RB because git diff's are compatible with RB. I have incorporated most of your review comments in this patch (v4). Please check my comments below. > On 2012-05-05 00:09:48, Julien Le Dem wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/optimizer/AllExpressionVisitor.java, > > line 98 > > <https://reviews.apache.org/r/4670/diff/2/?file=101421#file101421line98> > > > > iterate on entries() instead The MultiMap class under org.apache.pig.impl.util package doesn't seem to support iteration using entrySet. > On 2012-05-05 00:09:48, Julien Le Dem wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/optimizer/AllExpressionVisitor.java, > > lines 100-103 > > <https://reviews.apache.org/r/4670/diff/2/?file=101421#file101421line100> > > > > All those for loops could be factored into a private > > visitAll(Collection<LogicalExpressionPlan>) Good catch. Added new private method and refactored all occurrences of for loop to use this method. > On 2012-05-05 00:09:48, Julien Le Dem wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/optimizer/SchemaResetter.java, > > lines 137-140 > > <https://reviews.apache.org/r/4670/diff/2/?file=101422#file101422line137> > > > > same comment. > > You can factor out the for loops in those methods. Added new private method. > On 2012-05-05 00:09:48, Julien Le Dem wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LOCube.java, > > line 78 > > <https://reviews.apache.org/r/4670/diff/2/?file=101423#file101423line78> > > > > chain all exceptions Modified. > On 2012-05-05 00:09:48, Julien Le Dem wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java, > > line 397 > > <https://reviews.apache.org/r/4670/diff/2/?file=101429#file101429line397> > > > > you can make this a javadoc comment is javadoc generated for private methods as well? Since this is a private method I kept it as a simple comment. > On 2012-05-05 00:09:48, Julien Le Dem wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java, > > line 608 > > <https://reviews.apache.org/r/4670/diff/2/?file=101429#file101429line608> > > > > add the name of the duplicate in the error message Added. > On 2012-05-05 00:09:48, Julien Le Dem wrote: > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestCubeOperator.java, > > line 48 > > <https://reviews.apache.org/r/4670/diff/2/?file=101438#file101438line48> > > > > you van now use mock.Storage() for those tests. > > see PIG-2650 Updated from patch v3 onwards. > On 2012-05-05 00:09:48, Julien Le Dem wrote: > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestCubeOperator.java, > > line 139 > > <https://reviews.apache.org/r/4670/diff/2/?file=101438#file101438line139> > > > > 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)) Updated. > On 2012-05-05 00:09:48, Julien Le Dem wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java, > > line 522 > > <https://reviews.apache.org/r/4670/diff/2/?file=101429#file101429line522> > > > > same comment. I guess it is intended, but it looks strange. If the > > constructor has side effect, maybe there should be another way This inserts a project expression to logical expression plan (lEplan in this case). I found this way in other files too. I could not find a better way to attach a project expression to a logical expression plan. Do you have any other thoughts/ways for doing this? > On 2012-05-05 00:09:48, Julien Le Dem wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanGenerator.g, > > line 469 > > <https://reviews.apache.org/r/4670/diff/2/?file=101430#file101430line469> > > > > please add some comments to explain what you're doing here Updated. > On 2012-05-05 00:09:48, Julien Le Dem wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AstValidator.g, > > lines 259-273 > > <https://reviews.apache.org/r/4670/diff/2/?file=101428#file101428line259> > > > > do you know if there's a way to avoid duplication across .g files? Since all these grammar files serves different purposes, I am not sure if it is possible to avoid duplicates. - Prasanth_J ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4670/#review7484 ----------------------------------------------------------- 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 > >
