----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4670/#review6779 -----------------------------------------------------------
http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LOCube.java <https://reviews.apache.org/r/4670/#comment14961> might as well pull this up into a proper javadoc. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LOCube.java <https://reviews.apache.org/r/4670/#comment14972> I don't think you ever populate this. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LOCube.java <https://reviews.apache.org/r/4670/#comment14962> the tabulation seems to float across this patch. Please make sure to keep things at 4 spaces (no tabs). http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LOCube.java <https://reviews.apache.org/r/4670/#comment14969> That doesn't seem right. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LOCube.java <https://reviews.apache.org/r/4670/#comment14967> the if check is unnecessary -- you can simply catch a casting exception here, and wrap a frontendException around it. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LOCube.java <https://reviews.apache.org/r/4670/#comment14968> same comment as above -- the if is unnecessary, just catch an exception. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LOCube.java <https://reviews.apache.org/r/4670/#comment14970> Doesn't have to be ArrayList, per se, right? Any List (or even Collection) will do? http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LOCube.java <https://reviews.apache.org/r/4670/#comment14971> again, tabulation makes this hard to read right. please set up your ide to auto-format. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AliasMasker.g <https://reviews.apache.org/r/4670/#comment14974> I don't think you want the + here. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AliasMasker.g <https://reviews.apache.org/r/4670/#comment14973> why "join"? do you intend "cube" to also do a "cogroup"-like behavior? http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AstValidator.g <https://reviews.apache.org/r/4670/#comment14975> same comments here as for AliasMaker http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java <https://reviews.apache.org/r/4670/#comment14977> seems like the pattern of calling these 3 methods is all over this file. Do a good deed and pull it out into a private method that throws FE, and replace these blocks with a method call? http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java <https://reviews.apache.org/r/4670/#comment14978> This is confusing. So the cube operator is cut out and its predecessors are connected to a groupby instead? Why have a cube operator? This is a lot of pain to go through just to inject a single UDF into a group-by flow. Is it that you expect to have a proper physical Cube operator down the road, when you implement the more advanced cubing features? http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java <https://reviews.apache.org/r/4670/#comment14979> this method is entirely too long. Please refactor so that it's easier to follow, modify, maintain. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java <https://reviews.apache.org/r/4670/#comment14988> if you let this throw a FrontendException, a lot of unnecessary try/catches below go away. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java <https://reviews.apache.org/r/4670/#comment14982> please use better naming -- very hard to follow the code between src, srcs, srcsc. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java <https://reviews.apache.org/r/4670/#comment14983> convention is to put the "{" on the same line as the for loop. http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java <https://reviews.apache.org/r/4670/#comment14984> again, better naming. lExpr vs logExpr? http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java <https://reviews.apache.org/r/4670/#comment14985> why 0 http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java <https://reviews.apache.org/r/4670/#comment14986> how would the duplicates get there? http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/QueryLexer.g <https://reviews.apache.org/r/4670/#comment14990> pelase keep the spacing as it's set up in the rest of the file (the ":"s are aligned.) http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestCubeOperator.java <https://reviews.apache.org/r/4670/#comment14991> you don't need MR tests, and they are extremely slow. You can convert this to local mode. http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestCubeOperator.java <https://reviews.apache.org/r/4670/#comment14992> the method names are not very descriptive. Could you rename them and/or add documentation about what you are trying to test? - Dmitriy On 2012-04-07 17:53:13, Prasanth_J wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4670/ > ----------------------------------------------------------- > > (Updated 2012-04-07 17:53:13) > > > 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 https://issues.apache.org/jira/browse/PIG-2167. > > https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/PIG-2167 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AstValidator.g > 1303571 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java > 1303571 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AliasMasker.g > 1303571 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AstPrinter.g > 1303571 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LogicalRelationalNodesVisitor.java > 1303571 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/visitor/ProjectStarExpander.java > 1303571 > > 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/optimizer/SchemaResetter.java > 1303571 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/optimizer/AllExpressionVisitor.java > 1303571 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanGenerator.g > 1303571 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/QueryLexer.g > 1303571 > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/QueryParser.g > 1303571 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestLexer.pig > 1303571 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestLogicalPlanGenerator.java > 1303571 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestParser.pig > 1303571 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestQueryLexer.java > 1303571 > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestQueryParser.java > 1303571 > > 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 > >
