> 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. > > Prasanth_J wrote: > 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.
Are you sure it's not called from anywhere else? LogicalRelationalOperator.getSchema has over 200 references. Some of those are likely to be specific to individual operators, but it seems likely that there's something out there that generically calls for a schema. ProjectExpression, for example, seems to. > 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. > > Prasanth_J wrote: > 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. The documented style is Sun's, which does call for 80. In practice I use 100.. Break on operators? > 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. > > Prasanth_J wrote: > 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. Let's tackle that later if we need it. Seems like unnecessary complexity at this time (and users can write the join themselves, since knowing there will be a cube operation happening doesn't present an opportunity for optimization iirc. > 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? > > Prasanth_J wrote: > Packed this code segment to a private method and replaced all similar > code snippets with this method call. Thanks! > 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? > > Prasanth_J wrote: > 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? sgtm, let's add some docs / todos about that. > 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? > > Prasanth_J wrote: > 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. Throwing seems like a better idea, it's likely that a script that cubes by the same dimension several times is buggy. - Dmitriy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4670/#review6779 ----------------------------------------------------------- On 2012-04-12 07:12:48, Prasanth_J wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4670/ > ----------------------------------------------------------- > > (Updated 2012-04-12 07:12:48) > > > 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/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 > >
