> On 2012-04-07 23:50:33, Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LOCube.java, > > line 37 > > <https://reviews.apache.org/r/4670/diff/1/?file=100633#file100633line37> > > > > might as well pull this up into a proper javadoc.
Done. > On 2012-04-07 23:50:33, Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LOCube.java, > > line 55 > > <https://reviews.apache.org/r/4670/diff/1/?file=100633#file100633line55> > > > > I don't think you ever populate this. Removed it. > On 2012-04-07 23:50:33, Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LOCube.java, > > line 60 > > <https://reviews.apache.org/r/4670/diff/1/?file=100633#file100633line60> > > > > the tabulation seems to float across this patch. Please make sure to > > keep things at 4 spaces (no tabs). Fixed it everywhere. > On 2012-04-07 23:50:33, Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LOCube.java, > > line 69 > > <https://reviews.apache.org/r/4670/diff/1/?file=100633#file100633line69> > > > > That doesn't seem right. This method returns null since this method is not called from anywhere else in the current implementation. The idea is to add a separate physical operator for CUBE later on, after which get schema might be useful. > On 2012-04-07 23:50:33, Dmitriy Ryaboy 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/1/?file=100633#file100633line78> > > > > the if check is unnecessary -- you can simply catch a casting exception > > here, and wrap a frontendException around it. Good idea. Modified it. This kind of snippet floats around many places in the base code. > On 2012-04-07 23:50:33, Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LOCube.java, > > line 94 > > <https://reviews.apache.org/r/4670/diff/1/?file=100633#file100633line94> > > > > same comment as above -- the if is unnecessary, just catch an exception. Done. > On 2012-04-07 23:50:33, Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LOCube.java, > > line 114 > > <https://reviews.apache.org/r/4670/diff/1/?file=100633#file100633line114> > > > > again, tabulation makes this hard to read right. please set up your ide > > to auto-format. Done. Is there any limit for column break? The default java coding standard template breaks the column at 80 because of which some lines are abruptly broken. > On 2012-04-07 23:50:33, Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AstValidator.g, > > line 259 > > <https://reviews.apache.org/r/4670/diff/1/?file=100638#file100638line259> > > > > same comments here as for AliasMaker Updated. > On 2012-04-07 23:50:33, Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java, > > line 380 > > <https://reviews.apache.org/r/4670/diff/1/?file=100639#file100639line380> > > > > 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? Packed this code segment to a private method and replaced all similar code snippets with this method call. > On 2012-04-07 23:50:33, Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java, > > line 392 > > <https://reviews.apache.org/r/4670/diff/1/?file=100639#file100639line392> > > > > 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? Yes. The advance cubing features and optimizations requires a separate physical operator so that all cubing stuffs are kept isolated from other operators. In the current naive implementation the cubing operator is not so relevant, it is just kept to get the predecessors of cube operator and connect them to foreach (with UDF) followed by group by. Any suggestions for a better solution? > On 2012-04-07 23:50:33, Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java, > > line 395 > > <https://reviews.apache.org/r/4670/diff/1/?file=100639#file100639line395> > > > > this method is entirely too long. Please refactor so that it's easier > > to follow, modify, maintain. Refactored. > On 2012-04-07 23:50:33, Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java, > > line 396 > > <https://reviews.apache.org/r/4670/diff/1/?file=100639#file100639line396> > > > > if you let this throw a FrontendException, a lot of unnecessary > > try/catches below go away. Good idea. Updated. > On 2012-04-07 23:50:33, Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java, > > line 433 > > <https://reviews.apache.org/r/4670/diff/1/?file=100639#file100639line433> > > > > please use better naming -- very hard to follow the code between src, > > srcs, srcsc. Updated. > On 2012-04-07 23:50:33, Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java, > > line 443 > > <https://reviews.apache.org/r/4670/diff/1/?file=100639#file100639line443> > > > > convention is to put the "{" on the same line as the for loop. Updated. > On 2012-04-07 23:50:33, Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java, > > line 455 > > <https://reviews.apache.org/r/4670/diff/1/?file=100639#file100639line455> > > > > again, better naming. lExpr vs logExpr? Updated. > On 2012-04-07 23:50:33, Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java, > > line 459 > > <https://reviews.apache.org/r/4670/diff/1/?file=100639#file100639line459> > > > > how would the duplicates get there? a = load 'input' as (x,y,z); b = cube a by ($0,$0,$1); In this case, first column is specified twice. During cube computation duplicate dimensions does not make much sense. Now it is modified to throw an FEexception if there are duplicates in dimension attributes. > On 2012-04-07 23:50:33, Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/QueryLexer.g, > > line 84 > > <https://reviews.apache.org/r/4670/diff/1/?file=100641#file100641line84> > > > > pelase keep the spacing as it's set up in the rest of the file (the > > ":"s are aligned.) Updated. > On 2012-04-07 23:50:33, Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestCubeOperator.java, > > line 110 > > <https://reviews.apache.org/r/4670/diff/1/?file=100648#file100648line110> > > > > the method names are not very descriptive. Could you rename them and/or > > add documentation about what you are trying to test? Updated it to be descriptive with comments. > On 2012-04-07 23:50:33, Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LOCube.java, > > line 100 > > <https://reviews.apache.org/r/4670/diff/1/?file=100633#file100633line100> > > > > Doesn't have to be ArrayList, per se, right? Any List (or even > > Collection) will do? Agreed. Updated. > On 2012-04-07 23:50:33, Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AliasMasker.g, > > line 241 > > <https://reviews.apache.org/r/4670/diff/1/?file=100636#file100636line241> > > > > I don't think you want the + here. Modified. Initial design decision was to provide support for both star and snowflake schema. Do we need support snowflake schema as well? In case of snowflake schema the dimension attributes can be from multiple tables and a join is performed before cube computation. The current implementation has been tested only for star schema. So I have pulled + out of the code for now. > On 2012-04-07 23:50:33, Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AliasMasker.g, > > line 245 > > <https://reviews.apache.org/r/4670/diff/1/?file=100636#file100636line245> > > > > why "join"? > > > > do you intend "cube" to also do a "cogroup"-like behavior? For the purpose mentioned in the above comment (support for snowflake schema requires join). Removed 'join_' in the new patch. > On 2012-04-07 23:50:33, Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java, > > line 456 > > <https://reviews.apache.org/r/4670/diff/1/?file=100639#file100639line456> > > > > why 0 Same reason. Initially was planning to support snowflake schema. Dimensions for table 1 will be at index 0, dimensions for table 2 will be at index 1 and so on.. If we just need star schema then I can modify this as well. > On 2012-04-07 23:50:33, Dmitriy Ryaboy wrote: > > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestCubeOperator.java, > > line 66 > > <https://reviews.apache.org/r/4670/diff/1/?file=100648#file100648line66> > > > > you don't need MR tests, and they are extremely slow. You can convert > > this to local mode. Yeah. Makes sense. Updated to local mode. - Prasanth_J ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4670/#review6779 ----------------------------------------------------------- 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 > >
