[
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.
