You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pig.apache.org by "Santhosh Srinivasan (JIRA)" <ji...@apache.org> on 2008/07/29 23:55:31 UTC
[jira] Issue Comment Edited: (PIG-331) Combiner needs to be used in
the types branch
[ https://issues.apache.org/jira/browse/PIG-331?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12617972#action_12617972 ]
sms edited comment on PIG-331 at 7/29/08 2:55 PM:
------------------------------------------------------------------
- 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.
{code}
return new Iterator<Tuple>() {
+ Tuple t;
+ boolean atEnd;
+ public boolean hasNext() {
+ if (atEnd)
+ return false;
+ try {
+ if (t == null)
{code}
- File:
src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/CombinerOptimizer.java
* Minor Comment:
The log message indicates a leaf while the checks are being made for the roots.
{code}
if (reduceRoots.size() != 1) {
+ log.warn("Expected reduce to have single leaf");
+ return;
+ }
{code}
* 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(
{code}
//current code
if (!atLeastOneAlgebraic || !noNonAlgebraics) return null;
//more readable code?
if (atLeastOneAlgebraic && noNonAlgebraics) return null;
{code}
- 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.
{code}
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)
{code}
- 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:
src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/expressionOperators/POUserComparisonFunc.java
* Major Comment:
In the clone() method, the ComparisonFunc func is not cloned
- File:
src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/expressionOperators/POUserFunc.java
* 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.
{code}
+ POUserFunc clone = new POUserFunc(new OperatorKey(mKey.scope,
+ NodeIdGenerator.getGenerator().getNextNodeId(mKey.scope)),
+ requestedParallelism, null, funcSpec.clone());
{code}
- File:
src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/expressionOperators/BinaryExpressionOperator.java
* 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.
{code}
//in UnaryExpressionOperator.java's cloneHelper
+ resultType = op.getResultType();
{code}
- File:
src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/PODistinct.java
* Minor Comment:
The clone method is missing in PODistinct. Is this due to the re-write of distinct in LocalRearrange?
was (Author: sms):
- 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.
{code}
return new Iterator<Tuple>() {
+ Tuple t;
+ boolean atEnd;
+ public boolean hasNext() {
+ if (atEnd)
+ return false;
+ try {
+ if (t == null)
{code}
- File:
src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/CombinerOptimizer.java
* Minor Comment: the log message indicates a leaf while the checks are being made for
the roots.
{code}
if (reduceRoots.size() != 1) {
+ log.warn("Expected reduce to have single leaf");
+ return;
+ }
{code}
* 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(
{code}
//current code
if (!atLeastOneAlgebraic || !noNonAlgebraics) return null;
//more readable code?
if (atLeastOneAlgebraic && noNonAlgebraics) return null;
{code}
- 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.
{code}
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)
{code}
- 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:
src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/expressionOperators/POUserCo
mparisonFunc.java
* Major Comment:
In the clone() method, the ComparisonFunc func is not cloned
- File:
src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/expressionOperators/POUserFu
nc.java
* 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.
{code}
+ POUserFunc clone = new POUserFunc(new OperatorKey(mKey.scope,
+ NodeIdGenerator.getGenerator().getNextNodeId(mKey.scope)),
+ requestedParallelism, null, funcSpec.clone());
{code}
- File:
src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/expressionOperators/BinaryEx
pressionOperator.java
* 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.
{code}
//in UnaryExpressionOperator.java's cloneHelper
+ resultType = op.getResultType();
{code}
- File:
src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/PODistin
ct.java
* Minor Comment:
The clone method is missing in PODistinct. Is this due to the re-write of distinct in
LocalRearrange?
> 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.