Pig
  1. Pig
  2. PIG-1644

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.8.0
    • Fix Version/s: 0.8.0
    • Component/s: impl
    • Labels:
      None
    • Hadoop Flags:
      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';
      
      1. PIG-1644-4.patch
        43 kB
        Daniel Dai
      2. PIG-1644-3.patch
        42 kB
        Daniel Dai
      3. PIG-1644-2.patch
        44 kB
        Daniel Dai
      4. PIG-1644-1.patch
        15 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
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development