# New logical plan: Plan.connect with position is misused in some places

## Details

• Type: Bug
• Status: Closed
• Priority: Major
• Resolution: Fixed
• Affects Version/s: 0.8.0
• Fix Version/s:
• Component/s:
• Labels:
None
Reviewed

## Description

When we replace/remove/insert a node, we will use disconnect/connect methods of OperatorPlan. When we disconnect an edge, we shall save the position of the edge in origination and destination, and use this position when connect to the new predecessor/successor. Some of the pattens are:

Insert a new node:

```Pair<Integer, Integer> pos = plan.disconnect(pred, succ);
plan.connect(pred, pos.first, newnode, 0);
plan.connect(newnode, 0, succ, pos.second);
```

Remove a node:

```Pair<Integer, Integer> pos1 = plan.disconnect(pred, nodeToRemove);
Pair<Integer, Integer> pos2 = plan.disconnect(nodeToRemove, succ);
plan.connect(pred, pos1.first, succ, pos2.second);
```

Replace a node:

```Pair<Integer, Integer> pos1 = plan.disconnect(pred, nodeToReplace);
Pair<Integer, Integer> pos2 = plan.disconnect(nodeToReplace, succ);
plan.connect(pred, pos1.first, newNode, pos1.second);
plan.connect(newNode, pos2.first, succ, pos2.second);
```

There are couple of places of we does not follow this pattern, that results some error. For example, the following script fail:

```a = load '1.txt' as (a0, a1, a2, a3);
b = foreach a generate a0, a1, a2;
store b into 'aaa';
c = order b by a2;
d = foreach c generate a2;
store d into 'bbb';
```

## Attachments

1. PIG-1644-1.patch
15 kB
Daniel Dai
2. PIG-1644-2.patch
44 kB
Daniel Dai
3. PIG-1644-3.patch
42 kB
Daniel Dai
4. PIG-1644-4.patch
43 kB
Daniel Dai

## Activity

Hide
Daniel Dai added a comment -

[exec] +1 overall.
[exec]
[exec] +1 @author. The patch does not contain any @author tags.
[exec]
[exec] +1 tests included. The patch appears to include 6 new or modified tests.
[exec]
[exec] +1 javadoc. The javadoc tool did not generate any warning messages.
[exec]
[exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
[exec]
[exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
[exec]
[exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.

All tests pass.

Patch committed to both trunk and 0.8 branch.

Show
Daniel Dai added a comment - [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 6 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. All tests pass. Patch committed to both trunk and 0.8 branch.
Hide
Daniel Dai added a comment -

PIG-1644-4.patch fix findbug warnings and additional unit failures.

Show
Daniel Dai added a comment - PIG-1644 -4.patch fix findbug warnings and additional unit failures.
Hide
Daniel Dai added a comment -

Find one bug introduced by refactory. Attach PIG-1644-3.patch with the fix, and running the tests again.

Show
Daniel Dai added a comment - Find one bug introduced by refactory. Attach PIG-1644 -3.patch with the fix, and running the tests again.
Hide
Thejas M Nair added a comment -

Looks good. +1
Please commit after test-patch and unit tests pass.

Show
Thejas M Nair added a comment - Looks good. +1 Please commit after test-patch and unit tests pass.
Hide
Daniel Dai added a comment -

Attach the patch with new methods and refactory of existing code.

Show
Daniel Dai added a comment - Attach the patch with new methods and refactory of existing code.
Hide
Daniel Dai added a comment -

After looking into the existing code, seems insertBetween is a more useful method. So I want to drop insertBefore/insertAfter, and add insertBetween

```insertBetween(Operator pred, Operator operatorToInsert, Operator succ)
```
Show
Daniel Dai added a comment - After looking into the existing code, seems insertBetween is a more useful method. So I want to drop insertBefore/insertAfter, and add insertBetween insertBetween(Operator pred, Operator operatorToInsert, Operator succ)
Hide
Thejas M Nair added a comment -

I think insertAsPredecessor and insertAsSuccessor (instead of insertBefore and insertAfter) will convey the idea of what it does a little better.

Show
Thejas M Nair added a comment - I think insertAsPredecessor and insertAsSuccessor (instead of insertBefore and insertAfter) will convey the idea of what it does a little better.
Hide
Daniel Dai added a comment -

Yes, I think we can do replace/remove/insert. They should be simple and clear enough to use. Here is the new methods adding to OperatorPlan:

```replace(Operator oldOperator, Operator newOperator)
remove(Operator operatorToRemove) // Connect all its successors to predecessor/connect all it's predecessors to successor
insertBefore(Operator operatorToInsert, Operator pos) // Insert operatorToInsert before pos, connect all pos's predecessors to operatorToInsert
insertAfter(Operator operatorToInsert, Operator pos) // Insert operatorToInsert after pos, connect operatorToInsert to all pos's successor
```

How does it sounds?

Show
Daniel Dai added a comment - Yes, I think we can do replace/remove/insert. They should be simple and clear enough to use. Here is the new methods adding to OperatorPlan: replace(Operator oldOperator, Operator newOperator) remove(Operator operatorToRemove) // Connect all its successors to predecessor/connect all it's predecessors to successor insertBefore(Operator operatorToInsert, Operator pos) // Insert operatorToInsert before pos, connect all pos's predecessors to operatorToInsert insertAfter(Operator operatorToInsert, Operator pos) // Insert operatorToInsert after pos, connect operatorToInsert to all pos's successor How does it sounds?
Hide
Thejas M Nair added a comment -

These operations will be fairly common in the optimizer. I think it would be good to have functions in the OperatorPlan that support these operations, that will reduce the chances of bugs and also make the code more readable.

Show
Thejas M Nair added a comment - These operations will be fairly common in the optimizer. I think it would be good to have functions in the OperatorPlan that support these operations, that will reduce the chances of bugs and also make the code more readable.
Hide
Daniel Dai added a comment -

Attach the patch to address all such places in new logical plan, except for ExpressionSimplifier. There is some work underway for ExpressionSimplifier (PIG-1635) include some of these changes, I don't want to conflict with that patch. So after PIG-1635, we may also review the connect/disconnect usage of ExpressionSimplifier.

Show
Daniel Dai added a comment - Attach the patch to address all such places in new logical plan, except for ExpressionSimplifier. There is some work underway for ExpressionSimplifier ( PIG-1635 ) include some of these changes, I don't want to conflict with that patch. So after PIG-1635 , we may also review the connect/disconnect usage of ExpressionSimplifier.

## People

• Assignee:
Daniel Dai
Reporter:
Daniel Dai