> On July 2, 2012, 5:15 a.m., Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/RollupDimensions.java, > > line 49 > > <https://reviews.apache.org/r/5521/diff/1/?file=115854#file115854line49> > > > > how are you planning to inherit null handling behavior from > > CubeDimensions?
By using static function inside CubeDimensions function for converting nulls to unknown. This seems to be an easy solution which solves the purpose of reusing the same code for CubeDimensions and RollupDimensions. Ideally this should be done outside the UDFs in some Util class or somewhere before passing the tuples to UDFs. > On July 2, 2012, 5:15 a.m., Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/RollupDimensions.java, > > line 62 > > <https://reviews.apache.org/r/5521/diff/1/?file=115854#file115854line62> > > > > 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) ? > > > > Good catch. Updated. Yeah SQL rolls all the way up to (null, null, null, null). So the capacity should be tuple.size() + 1. > On July 2, 2012, 5:15 a.m., Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/RollupDimensions.java, > > line 71 > > <https://reviews.apache.org/r/5521/diff/1/?file=115854#file115854line71> > > > > 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. Fixed. As explained above using static function. > On July 2, 2012, 5:15 a.m., Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/RollupDimensions.java, > > line 83 > > <https://reviews.apache.org/r/5521/diff/1/?file=115854#file115854line83> > > > > 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. Updated to an iterative way. > On July 2, 2012, 5:15 a.m., Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/builtin/RollupDimensions.java, > > line 95 > > <https://reviews.apache.org/r/5521/diff/1/?file=115854#file115854line95> > > > > can you document here what happens to "dimensions" the string as it is > > adjusted by the cube/rollup operator? Updated. > On July 2, 2012, 5:15 a.m., Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LOCube.java, > > line 45 > > <https://reviews.apache.org/r/5521/diff/1/?file=115855#file115855line45> > > > > let's remove the word "now" Updated. > On July 2, 2012, 5:15 a.m., Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java, > > line 416 > > <https://reviews.apache.org/r/5521/diff/1/?file=115859#file115859line416> > > > > there is an implicit else here (because of the "continue") -- make it > > explicit Updated. > On July 2, 2012, 5:15 a.m., Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java, > > line 419 > > <https://reviews.apache.org/r/5521/diff/1/?file=115859#file115859line419> > > > > style: prefer > > > > if { > > > > } else if { > > > > } > > Yeah. there was an issue with formatting. Updated all over the patch. > On July 2, 2012, 5:15 a.m., Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java, > > line 434 > > <https://reviews.apache.org/r/5521/diff/1/?file=115859#file115859line434> > > > > document what the corner case is? Updated. > On July 2, 2012, 5:15 a.m., Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java, > > line 587 > > <https://reviews.apache.org/r/5521/diff/1/?file=115859#file115859line587> > > > > this will need to change to match the null handling change in the other > > ticket Updated. > On July 2, 2012, 5:15 a.m., Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanGenerator.g, > > line 509 > > <https://reviews.apache.org/r/5521/diff/1/?file=115860#file115860line509> > > > > something odd is going on with the indentation here Fixed. > On July 2, 2012, 5:15 a.m., Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestRollupDimensions.java, > > line 44 > > <https://reviews.apache.org/r/5521/diff/1/?file=115869#file115869line44> > > > > this will need to change with the new null handling Updated. > On July 2, 2012, 5:15 a.m., Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestQueryParser.java, > > line 217 > > <https://reviews.apache.org/r/5521/diff/1/?file=115867#file115867line217> > > > > 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. Fixed. Updated shouldFail method. Negative test cases uses expected @Test annotation now. - Prasanth_J ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5521/#review8778 ----------------------------------------------------------- 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 > >
