Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Pig, mail # dev - Review Request 15194: Support multiple inputs for PigProcessor


Copy link to this message
-
Re: Review Request 15194: Support multiple inputs for PigProcessor
Cheolsoo Park 2013-11-03, 20:31

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15194/#review28080
-----------------------------------------------------------
Looks good overall. I made a few minor comments below:
src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/POSimpleTezLoad.java
<https://reviews.apache.org/r/15194/#comment54652>

    Please remove white spaces in the patch.

src/org/apache/pig/backend/hadoop/executionengine/tez/POShuffleTezLoad.java
<https://reviews.apache.org/r/15194/#comment54642>

    Do we need this field? It doesn't seem used anywhere.

src/org/apache/pig/backend/hadoop/executionengine/tez/POShuffleTezLoad.java
<https://reviews.apache.org/r/15194/#comment54643>

    Private?

src/org/apache/pig/backend/hadoop/executionengine/tez/POShuffleTezLoad.java
<https://reviews.apache.org/r/15194/#comment54644>

    Private?

src/org/apache/pig/backend/hadoop/executionengine/tez/POShuffleTezLoad.java
<https://reviews.apache.org/r/15194/#comment54645>

    This constructor doesn't seem needed. Can we delete it?

src/org/apache/pig/backend/hadoop/executionengine/tez/POShuffleTezLoad.java
<https://reviews.apache.org/r/15194/#comment54659>

    Perhaps we add a copy constructor to POPackage and call it here? Just a thought.

src/org/apache/pig/backend/hadoop/executionengine/tez/POShuffleTezLoad.java
<https://reviews.apache.org/r/15194/#comment54647>

    Please remove unnecessary empty lines.

src/org/apache/pig/backend/hadoop/executionengine/tez/POShuffleTezLoad.java
<https://reviews.apache.org/r/15194/#comment54646>

    Can you run two for loops inside a single try-catch block instead of duplicating it?

src/org/apache/pig/backend/hadoop/executionengine/tez/POShuffleTezLoad.java
<https://reviews.apache.org/r/15194/#comment54648>

    If you keep the index of minimum instead of minimum itself, then you don't need to call comparator.compare() here, no?

src/org/apache/pig/backend/hadoop/executionengine/tez/POShuffleTezLoad.java
<https://reviews.apache.org/r/15194/#comment54649>

    Do we need this? Doesn't seem used anywhere.

src/org/apache/pig/backend/hadoop/executionengine/tez/POShuffleTezLoad.java
<https://reviews.apache.org/r/15194/#comment54650>

    Do we need this? Doesn't seem used anywhere.

src/org/apache/pig/backend/hadoop/executionengine/tez/POShuffleTezLoad.java
<https://reviews.apache.org/r/15194/#comment54651>

    You can initialize inputKeys in declaration and remove these lines, can't you?

src/org/apache/pig/backend/hadoop/executionengine/tez/TezDagBuilder.java
<https://reviews.apache.org/r/15194/#comment54660>

    Can't you do "tezOp.plan.replace(pack, shuffleLoad)" instead of remove and add? Then, you don't need to copy the succsList and re-connect them to shuffleLoad at all, no?

src/org/apache/pig/backend/hadoop/executionengine/tez/TezDagBuilder.java
<https://reviews.apache.org/r/15194/#comment54655>

    Remove.

src/org/apache/pig/backend/hadoop/executionengine/tez/TezDagBuilder.java
<https://reviews.apache.org/r/15194/#comment54658>

    If we use tezOp.plan.replace() below, we don't need to copy the ldSucs, no?

src/org/apache/pig/backend/hadoop/executionengine/tez/TezDagBuilder.java
<https://reviews.apache.org/r/15194/#comment54656>

    Remove commented out code.

src/org/apache/pig/backend/hadoop/executionengine/tez/TezDagBuilder.java
<https://reviews.apache.org/r/15194/#comment54657>

    Looks like this can be rewritten as follows:
    
    POSimpleTezLoad tezLoad = new POSimpleTezLoad(new OperatorKey(scope , nig.getNextNodeId(scope)));
    tezLoad.setInputKey(ld.getOperatorKey().toString());
    tezOp.plan.replace(ld, tezLoad);
- Cheolsoo Park
On Nov. 2, 2013, 1:17 a.m., Mark Wagner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15194/
> ---