[ 
https://issues.apache.org/jira/browse/PIG-958?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12758952#action_12758952
 ] 

Pradeep Kamath commented on PIG-958:
------------------------------------

In general the patch looks good - do address the hudson QA issues. Some code 
review comments:

1) I don't think pig.properties changes should be part of this patch - was this 
accidental? Likewise for Main.java.

2)
{noformat}
 72   /**                                                                       
                                                                                
                                                                           
 73    *  storeUID is needed to get the around the case of multi-query 
optimisation where                                                              
                                                                                
    
 74    *  multiple instances of MultiStorage could be running in a single 
mapper/reducer                                                                  
                                                                                
 
 75    *  and run the risk of overwriting each other's output.                  
                                                                                
                                                                           
 76    */      
{noformat}
Wouldn't the multiple instances be writing to different locations in which case 
there should be no race condition right?

3)
{noformat}
172   private void initJobSpecificParams() throws IOException {                 
                                                                                
                                                                           
173     if (partition == null || outputPath == null) {   
{noformat}
Wouldn't the outputPath never be null since it is initialized in the 
constructor?

4) Consider removing the log.debug() since these are all in the inner loop and 
would possibly impact performance.

5)
{noformat}
291         String fieldValueBasedPathStr = 
fieldValueBasedPath.toUri().getPath(); 
{noformat}
This variable is only really used in a log.debug(), I think removing this and 
using fieldValueBasedPath in all the fs.create() will make the code shorter and 
cleaner.

6) Some comments on why the move of the output dirs is needed and in the 
moveResults() and removePart() methods would be helpful. Additional comments on 
the code flow would also help


> Splitting output data on key field
> ----------------------------------
>
>                 Key: PIG-958
>                 URL: https://issues.apache.org/jira/browse/PIG-958
>             Project: Pig
>          Issue Type: Bug
>    Affects Versions: 0.4.0
>            Reporter: Ankur
>         Attachments: 958.v2.patch
>
>
> Pig users often face the need to split the output records into a bunch of 
> files and directories depending on the type of record. Pig's SPLIT operator 
> is useful when record types are few and known in advance. In cases where type 
> is not directly known but is derived dynamically from values of a key field 
> in the output tuple, a custom store function is a better solution.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply via email to