Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: tez-branch
    • Component/s: tez
    • Labels:
      None
    1. PIG-3743-1.patch
      30 kB
      Cheolsoo Park
    2. PIG-3743-2.patch
      31 kB
      Cheolsoo Park
    3. PIG-3743-fix-skew.patch
      3 kB
      Cheolsoo Park
    4. PIG-3743-fix-skew-2.patch
      5 kB
      Cheolsoo Park

      Issue Links

        Activity

        Hide
        Cheolsoo Park added a comment -

        First patch. The RB link-
        https://reviews.apache.org/r/19633/

        Show
        Cheolsoo Park added a comment - First patch. The RB link- https://reviews.apache.org/r/19633/
        Hide
        Cheolsoo Park added a comment -

        Second patch.

        Show
        Cheolsoo Park added a comment - Second patch.
        Hide
        Rohini Palaniswamy added a comment -

        +1

        Show
        Rohini Palaniswamy added a comment - +1
        Hide
        Cheolsoo Park added a comment -

        Committed to tez branch. Thank you Rohini for the review!

        Show
        Cheolsoo Park added a comment - Committed to tez branch. Thank you Rohini for the review!
        Hide
        Cheolsoo Park added a comment -

        Filed PIG-3835 for union followed by store.

        Show
        Cheolsoo Park added a comment - Filed PIG-3835 for union followed by store.
        Hide
        Cheolsoo Park added a comment -

        Actually, I introduced a terrible skew to union in my previous patch.

        Since I am setting key to null, every record goes to a single task in reducer vertex. I will post a fix shortly.

        Show
        Cheolsoo Park added a comment - Actually, I introduced a terrible skew to union in my previous patch. Since I am setting key to null, every record goes to a single task in reducer vertex. I will post a fix shortly.
        Hide
        Cheolsoo Park added a comment -

        Verified with a production script that union is distributed. Please review PIG-3743-fix-skew.patch.

        Show
        Cheolsoo Park added a comment - Verified with a production script that union is distributed. Please review PIG-3743 -fix-skew.patch .
        Hide
        Rohini Palaniswamy added a comment - - edited

        Cheolsoo Park,

        POLocalRearrangeTez.java

        if (isUnion) {
                                // Use the entire tuple as both key and value
                                key = HDataType.getWritableComparableTypes(result.get(1), keyType);
                                val = new NullableTuple((Tuple)result.get(1));
                            }
        

        This seems very inefficient as map output size just doubles and the key is just discarded. Can we use POValueOutputTez instead of POLocalRearrangeTez and use RoundRobinPartitioner instead. Till now POValueOutputTez was only used with unordered input. Since the key is always POValueOutputTez.EmptyWritable, can you change it to implement WritableComparable and always return 1 (or -1 based on how comparator is called in reducer) so that no records are equal and they are not grouped together. If every key saying greater than other causes confusion while ordering and will not work, then we cannot use WritableComparable and have to go with each reducer having 1KV pair with all records grouped together. It might be overhead, but still should be better than having duplicating value in key and doubling the memory requirements.

        Also will have to cleanup isUnion code in POLocalRearrangeTez if the solution works fine.

        Show
        Rohini Palaniswamy added a comment - - edited Cheolsoo Park , POLocalRearrangeTez.java if (isUnion) { // Use the entire tuple as both key and value key = HDataType.getWritableComparableTypes(result.get(1), keyType); val = new NullableTuple((Tuple)result.get(1)); } This seems very inefficient as map output size just doubles and the key is just discarded. Can we use POValueOutputTez instead of POLocalRearrangeTez and use RoundRobinPartitioner instead. Till now POValueOutputTez was only used with unordered input. Since the key is always POValueOutputTez.EmptyWritable, can you change it to implement WritableComparable and always return 1 (or -1 based on how comparator is called in reducer) so that no records are equal and they are not grouped together. If every key saying greater than other causes confusion while ordering and will not work, then we cannot use WritableComparable and have to go with each reducer having 1KV pair with all records grouped together. It might be overhead, but still should be better than having duplicating value in key and doubling the memory requirements. Also will have to cleanup isUnion code in POLocalRearrangeTez if the solution works fine.
        Hide
        Cheolsoo Park added a comment -

        Yeah, totally agree with you. I was going to put up a patch in a separate jira since that's not related to VertexGroup. It had been written a long time before POValueOutputTez was added.

        Show
        Cheolsoo Park added a comment - Yeah, totally agree with you. I was going to put up a patch in a separate jira since that's not related to VertexGroup. It had been written a long time before POValueOutputTez was added.
        Hide
        Cheolsoo Park added a comment -

        In addition, can we fix the skew first so that I can benchmark POValueOuputTez vs POLocalRearrangeTez? I'd like to measure the impacts of two changes separately-

        1. VertexGroup change
        2. POValueOutputTez

        With the skew fix, I can measure #1 isolated. Overnight I was running the same script that I used before and seeing a perf regression. I'd like to understand it better. Thanks!

        Show
        Cheolsoo Park added a comment - In addition, can we fix the skew first so that I can benchmark POValueOuputTez vs POLocalRearrangeTez? I'd like to measure the impacts of two changes separately- VertexGroup change POValueOutputTez With the skew fix, I can measure #1 isolated. Overnight I was running the same script that I used before and seeing a perf regression. I'd like to understand it better. Thanks!
        Hide
        Rohini Palaniswamy added a comment -

        In addition, can we fix the skew first

        How bad is the performance regression? Are you asking for committing PIG-3743-fix-skew.patch and then address POValueOuputTez in a separate jira? I don't have a problem with that. Can probably be clubbed with PIG-3835.

        Also for POValueOuputTez instead of implementing WritableComparable it would be better to write a custom Comparator and either return 0 always (if we want all records to be grouped into one) or 1/-1 always (if it works and does not cause too much of reordering) based on what works. Using WritableComparator together with implementing WritableComparable is slightly more overhead.

        Show
        Rohini Palaniswamy added a comment - In addition, can we fix the skew first How bad is the performance regression? Are you asking for committing PIG-3743 -fix-skew.patch and then address POValueOuputTez in a separate jira? I don't have a problem with that. Can probably be clubbed with PIG-3835 . Also for POValueOuputTez instead of implementing WritableComparable it would be better to write a custom Comparator and either return 0 always (if we want all records to be grouped into one) or 1/-1 always (if it works and does not cause too much of reordering) based on what works. Using WritableComparator together with implementing WritableComparable is slightly more overhead.
        Hide
        Cheolsoo Park added a comment -
        1. I see 10% slower in the current head + PIG-3743-fix-skew.patch compared to before. I don't have any specific analysis on what's contributed to it.
        2. Are you asking for committing PIG-3743-fix-skew.patch and then address POValueOuputTez in a separate jira? I don't have a problem with that.

          Good. I'll do that. Thanks!

        3. It might be overhead, but still should be better than having duplicating value in key and doubling the memory requirements.

          A simple alternative is to set value to null in POLR and let POGroupInputTez retrieve key. Do you think this is fine? Here is the implementation-
          https://github.com/piaozhexiu/apache-pig/commit/00f671d563ada56ece50493dd1d24aa5587c8171

        Show
        Cheolsoo Park added a comment - I see 10% slower in the current head + PIG-3743 -fix-skew.patch compared to before. I don't have any specific analysis on what's contributed to it. Are you asking for committing PIG-3743 -fix-skew.patch and then address POValueOuputTez in a separate jira? I don't have a problem with that. Good. I'll do that. Thanks! It might be overhead, but still should be better than having duplicating value in key and doubling the memory requirements. A simple alternative is to set value to null in POLR and let POGroupInputTez retrieve key. Do you think this is fine? Here is the implementation- https://github.com/piaozhexiu/apache-pig/commit/00f671d563ada56ece50493dd1d24aa5587c8171
        Hide
        Rohini Palaniswamy added a comment -

        A simple alternative is to set value to null in POLR and let POGroupInputTez retrieve key.

        POLR adds lot of overhead while processing tuple. POValueOuputTez does no processing and just passes it through which will be more efficient.

        Show
        Rohini Palaniswamy added a comment - A simple alternative is to set value to null in POLR and let POGroupInputTez retrieve key. POLR adds lot of overhead while processing tuple. POValueOuputTez does no processing and just passes it through which will be more efficient.
        Hide
        Cheolsoo Park added a comment -

        Okie. Let me try POValueOutput in PIG-3835. I'll update it's description accordingly.

        For the time being, I will commit PIG-3743-fix-skew-2.patch that uses the whole record as key and sets value to null.

        Thanks!

        Show
        Cheolsoo Park added a comment - Okie. Let me try POValueOutput in PIG-3835 . I'll update it's description accordingly. For the time being, I will commit PIG-3743 -fix-skew-2.patch that uses the whole record as key and sets value to null. Thanks!
        Hide
        Cheolsoo Park added a comment -

        Committed to tez branch. Thank you Rohini for the help!

        Show
        Cheolsoo Park added a comment - Committed to tez branch. Thank you Rohini for the help!

          People

          • Assignee:
            Cheolsoo Park
            Reporter:
            Rohini Palaniswamy
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development