pig-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gianmarco De Francisci Morales" <g...@apache.org>
Subject Re: Review Request: RANK function like in SQL
Date Thu, 28 Jun 2012 14:07:00 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5523/#review8710
-----------------------------------------------------------


Overall looks good.


http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/JobControlCompiler.java
<https://reviews.apache.org/r/5523/#comment18377>

    The same constant is defined in PigMapOnly.MapRank
    It should be defined only in one place.



http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/JobControlCompiler.java
<https://reviews.apache.org/r/5523/#comment18373>

    What if we have more than one job doing a rank in the same MROoPlan at the same time?
    I think this piece of code would not work as you would overwrite the rankGroup.



http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/JobControlCompiler.java
<https://reviews.apache.org/r/5523/#comment18374>

    Logging all this information at INFO level is too much.
    I would either reduce the amount of logged info or put it at DEBUG level.



http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/MRCompiler.java
<https://reviews.apache.org/r/5523/#comment18375>

    How do we enforce that the number of mappers is the same in the Counter and Rank jobs?
    It is not too relevant given the other optimized implementation we discussed about, but
worth pointing it out.



http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PhyPlanSetter.java
<https://reviews.apache.org/r/5523/#comment18376>

    Why is this commented?
    What would be the purpose of this piece of code?



http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POCounter.java
<https://reviews.apache.org/r/5523/#comment18378>

    Is POCounter responsible for sorting the input?
    



http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POCounter.java
<https://reviews.apache.org/r/5523/#comment18379>

    What role do the rankPlans play in POCounter? It is unclear to me.



http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/PORank.java
<https://reviews.apache.org/r/5523/#comment18381>

    We know the size of the final tuple, let's use the optimized constructor to preallocate
the right amount of memory.



http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/PORank.java
<https://reviews.apache.org/r/5523/#comment18380>

    Why a separate method for incrementing a counter?
    Why is it different from countNext?



http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/PORank.java
<https://reviews.apache.org/r/5523/#comment18382>

    No need for getAll(), Tuple is Iterable:
    
    for (Object o : in)
      out.append(o);



http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LORank.java
<https://reviews.apache.org/r/5523/#comment18383>

    Some imports are unused, we can clean it up.


- Gianmarco De Francisci Morales


On June 25, 2012, 10:46 a.m., aavendan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5523/
> -----------------------------------------------------------
> 
> (Updated June 25, 2012, 10:46 a.m.)
> 
> 
> Review request for pig, aavendan and Gianmarco De Francisci Morales.
> 
> 
> Description
> -------
> 
> Review board for https://issues.apache.org/jira/browse/PIG-2353
> 
> 
> This addresses bug PIG-2353.
>     https://issues.apache.org/jira/browse/PIG-2353
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/JobControlCompiler.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/MRCompiler.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/MapReduceOper.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PhyPlanSetter.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/PigMapOnly.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/plans/DotPOPrinter.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/plans/PhyPlanVisitor.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/plans/PlanPrinter.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POCounter.java
PRE-CREATION 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/PORank.java
PRE-CREATION 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/optimizer/AllExpressionVisitor.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/optimizer/AllSameRalationalNodesVisitor.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/optimizer/LogicalPlanPrinter.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/optimizer/SchemaResetter.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/optimizer/UidResetter.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LORank.java
PRE-CREATION 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LogToPhyTranslationVisitor.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/relational/LogicalRelationalNodesVisitor.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/rules/ColumnPruneHelper.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/rules/ColumnPruneVisitor.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/visitor/LineageFindRelVisitor.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/visitor/ProjectStarExpander.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/visitor/SchemaAliasVisitor.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/logical/visitor/TypeCheckingRelVisitor.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AliasMasker.g 1353202

>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AstPrinter.g 1353202

>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/AstValidator.g
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanBuilder.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/LogicalPlanGenerator.g
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/QueryLexer.g 1353202

>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/parser/QueryParser.g 1353202

>   http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/tools/pigstats/ScriptState.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestLexer.pig
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestLogicalPlanGenerator.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestParser.pig
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestQueryLexer.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/parser/TestQueryParser.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/OptimizeLimitPlanPrinter.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestLogicalPlanBuilder.java
1353202 
>   http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestOrderBy3.java
PRE-CREATION 
>   http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestRank1.java PRE-CREATION

>   http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestRank2.java PRE-CREATION

> 
> Diff: https://reviews.apache.org/r/5523/diff/
> 
> 
> Testing
> -------
> 
> All pre-commit tests passed
> 
> 
> Thanks,
> 
> aavendan
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message