hadoop-pig-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Santhosh Srinivasan (JIRA)" <j...@apache.org>
Subject [jira] Commented: (PIG-331) Combiner needs to be used in the types branch
Date Tue, 29 Jul 2008 21:53:31 GMT

    [ https://issues.apache.org/jira/browse/PIG-331?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12617972#action_12617972

Santhosh Srinivasan commented on PIG-331:

   - File: src/org/apache/pig/backend/local/executionengine/LocalJob.java

      * In the anonymous Iterator that is returned, I noticed that the public member 

variables are not initialized but they are used in the hasNext() method.

return new Iterator<Tuple>() {

+            Tuple   t;
+            boolean atEnd;
+            public boolean hasNext() {
+                if (atEnd)
+                    return false;
+                try {
+                    if (t == null)

   - File: 


      * Minor Comment: the log message indicates a leaf while the checks are being made for

the roots.

 if (reduceRoots.size() != 1) {
+            log.warn("Expected reduce to have single leaf");
+            return;
+        }


      * Minor Comment: The condition to check for the existence of at least one algebraic

and no instances of non-algebraic types is probably better written using && instead
of ||

Method: +    private List<ExprType> algebraic(

//current code
if (!atLeastOneAlgebraic || !noNonAlgebraics) return null;

//more readable code?

if (atLeastOneAlgebraic && noNonAlgebraics) return null;


   - File: src/org/apache/pig/impl/plan/OperatorPlan.java

      * Major Comment:

The new replace(oldNode, newNode) method is similar to the private replaceNode(src, 

replacement, dst, multiMap) method added as part of 

https://issues.apache.org/jira/browse/PIG-290. The difference lies in the method signature

and the access specifier.

public void replace(E oldNode, E newNode) throws PlanException {
//Pig-290 change below
private boolean replaceNode(E src, E replacement, E dst, MultiMap<E, E> multiMap) 

   - File: src/org/apache/pig/impl/util/Pair.java

      * Minor Comment:
Do we need equals() and hashCode() methods to make the class usable in containers?

   - File: 

   - File: 



      * Major Comment:

In the clone() method, the ComparisonFunc func is not cloned

   - File: 



   * Major Comment:

In the clone method, the final two constructor arguments are in the wrong order. POUserFunc

expects FuncSpec, EvalFunc while the clone method uses null, funcSpec.clone. This should not

compile in the first place.

+        POUserFunc clone = new POUserFunc(new OperatorKey(mKey.scope, 
+            NodeIdGenerator.getGenerator().getNextNodeId(mKey.scope)),
+            requestedParallelism, null, funcSpec.clone());

   - File: 



   * Major Comment:

In the cloneHelper method, the operator's resultType is not set. In the corresponding 

cloneHelper method in UnaryExpressionOperator, the restultType is set. The resultType is 

also set in PhysicalOperator's cloneHelper method.

//in UnaryExpressionOperator.java's cloneHelper

+        resultType = op.getResultType();

   - File: 



   * Minor Comment:

The clone method is missing in PODistinct. Is this due to the re-write of distinct in 


> Combiner needs to be used in the types branch
> ---------------------------------------------
>                 Key: PIG-331
>                 URL: https://issues.apache.org/jira/browse/PIG-331
>             Project: Pig
>          Issue Type: Bug
>    Affects Versions: types_branch
>            Reporter: Alan Gates
>            Assignee: Alan Gates
>            Priority: Critical
>             Fix For: types_branch
>         Attachments: combiner.patch
> The initial implementation in the types branch does not make use of the combiner.  For
performance, it needs to make use of the combiner.

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

View raw message