----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5521/#review8778 -----------------------------------------------------------
overall, looks good. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/RollupDimensions.java <https://reviews.apache.org/r/5521/#comment18556> how are you planning to inherit null handling behavior from CubeDimensions? http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/RollupDimensions.java <https://reviews.apache.org/r/5521/#comment18554> the capacity set here is too large -- for hierarchical rollup, we just need tuple.size() elements does the sql standard rollup go all the way up to (null, null, null, null) or does it stop at (a, null, null, null) ? http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/RollupDimensions.java <https://reviews.apache.org/r/5521/#comment18555> I believe that's also in the null-handling patch. Since you need it both here and in CubeDimensions, it would be better to reuse that bit of code than to make copies of it. The comment about not allocating new tuples and copying into them unnecessarily applies here as well. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/RollupDimensions.java <https://reviews.apache.org/r/5521/#comment18557> seems like recursion makes the code too complex when all we need to do is tuple.size() loops in which we make a copy of the tuple and set some fields to null. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/RollupDimensions.java <https://reviews.apache.org/r/5521/#comment18558> can you document here what happens to "dimensions" the string as it is adjusted by the cube/rollup operator? http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LOCube.java <https://reviews.apache.org/r/5521/#comment18559> let's remove the word "now" http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java <https://reviews.apache.org/r/5521/#comment18560> there is an implicit else here (because of the "continue") -- make it explicit http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java <https://reviews.apache.org/r/5521/#comment18561> style: prefer if { } else if { } http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java <https://reviews.apache.org/r/5521/#comment18562> } else { http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java <https://reviews.apache.org/r/5521/#comment18563> document what the corner case is? http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java <https://reviews.apache.org/r/5521/#comment18581> this will need to change to match the null handling change in the other ticket http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanGenerator.g <https://reviews.apache.org/r/5521/#comment18585> something odd is going on with the indentation here http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestQueryParser.java <https://reviews.apache.org/r/5521/#comment18600> There is a parameter one can supply to the JUnit @Test annotation that can more clearly describe what failure is expected : http://junit.sourceforge.net/javadoc/org/junit/Test.html (see 'expected'). What you've done here is consistent with the rest of the test, but might be worth fixing up shouldFail to use this instead of catching exception and returning true -- cleaner that way. Not required though. http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestRollupDimensions.java <https://reviews.apache.org/r/5521/#comment18590> this will need to change with the new null handling - Dmitriy Ryaboy On June 22, 2012, 7:35 a.m., Prasanth_J wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5521/ > ----------------------------------------------------------- > > (Updated June 22, 2012, 7:35 a.m.) > > > Review request for pig and Dmitriy Ryaboy. > > > Description > ------- > > This is a review board request for > https://issues.apache.org/jira/browse/PIG-2765 > > > This addresses bug PIG-2765. > https://issues.apache.org/jira/browse/PIG-2765 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/RollupDimensions.java > PRE-CREATION > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LOCube.java > 1352776 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AliasMasker.g > 1352776 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AstPrinter.g > 1352776 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AstValidator.g > 1352776 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java > 1352776 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanGenerator.g > 1352776 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/QueryLexer.g > 1352776 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/QueryParser.g > 1352776 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestLexer.pig > 1352776 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestLogicalPlanGenerator.java > 1352776 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestParser.pig > 1352776 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestQueryLexer.java > 1352776 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestQueryParser.java > 1352776 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestCubeOperator.java > 1352776 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestRollupDimensions.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/5521/diff/ > > > Testing > ------- > > Unit tests: All passed > > Pre-commit tests: All passed > ant clean test-commit > > > Thanks, > > Prasanth_J > >
