Pig
  1. Pig
  2. PIG-3057

Make PigStorage.readField() protected

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 0.8.0, 0.8.1, 0.9.0, 0.9.1, 0.9.2, 0.10.0
    • Fix Version/s: 0.12.0
    • Component/s: build, internal-udfs
    • Labels:
    • Patch Info:
      Patch Available

      Description

      for the cases when we need to extend PigStorage just to override readField. Currently, we need to copy/paste several private fields and all getNext

      I've changed readField from private to protected and added a new method: protected void addToCurrentTuple(DataByteArray data)

      1. PIG-3057_1.patch
        2 kB
        Bill Graham
      2. PigStorage_readField.patch
        0.8 kB
        pablo martinez

        Activity

        pablo martinez created issue -
        pablo martinez made changes -
        Field Original Value New Value
        Attachment PigStorage_readField.patch [ 12554264 ]
        Dmitriy V. Ryaboy made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Bill Graham added a comment -

        Here's a patch that does what I'm describing. I kept readField but made it return DataByteArray and changed it to protected.

        Show
        Bill Graham added a comment - Here's a patch that does what I'm describing. I kept readField but made it return DataByteArray and changed it to protected.
        Bill Graham made changes -
        Attachment PIG-3057_1.patch [ 12563523 ]
        Hide
        Bill Graham added a comment -

        Oops, I thought I had submitted a comment before the last one that was really just in preview-mode...

        What if we instead change readField top this:

        protected DataByteArray readField(byte[] bytes, int start, int end) {
          if (start == end) {
            return null;
          }
          else {
            return new DataByteArray(bytes, start, end);
          }
        }
        

        and then added a new private addTupleValue(..) method that calls it. It seems like that would achieve the same goal in a more intuitive way w.r.t subclassing.

        Show
        Bill Graham added a comment - Oops, I thought I had submitted a comment before the last one that was really just in preview-mode... What if we instead change readField top this: protected DataByteArray readField(byte[] bytes, int start, int end) { if (start == end) { return null; } else { return new DataByteArray(bytes, start, end); } } and then added a new private addTupleValue(..) method that calls it. It seems like that would achieve the same goal in a more intuitive way w.r.t subclassing.
        Hide
        pablo martinez added a comment -

        sure, I like better your solution

        let's see if I understood correctly.
        1_ the new method addTupleValue is gonna be something like this ?

        private void addTupleValue(byte[] buf, int start, int len)

        { mProtoTuple.add(readField(buf, start, len)); }

        2_ and getNext() will replace its 2 calls to readField with something like the following ?
        now: readField(buf, start, i);
        new one: addTupleValue(buf, start, i);

        now: readField(buf, start, len);
        new one: addTupleValue(buf, start, len);

        thank you so much,

        Show
        pablo martinez added a comment - sure, I like better your solution let's see if I understood correctly. 1_ the new method addTupleValue is gonna be something like this ? private void addTupleValue(byte[] buf, int start, int len) { mProtoTuple.add(readField(buf, start, len)); } 2_ and getNext() will replace its 2 calls to readField with something like the following ? now: readField(buf, start, i); new one: addTupleValue(buf, start, i); now: readField(buf, start, len); new one: addTupleValue(buf, start, len); thank you so much,
        Hide
        Bill Graham added a comment -

        Yes, basically. One minor difference is that I'm passing mProtoTuple to addTupleValue instead of referencing it as an instance object. It's a bit cleaner that way. You can apply the attached patch to see how it looks.

        Show
        Bill Graham added a comment - Yes, basically. One minor difference is that I'm passing mProtoTuple to addTupleValue instead of referencing it as an instance object. It's a bit cleaner that way. You can apply the attached patch to see how it looks.
        Hide
        pablo martinez added a comment -

        good. Also, with your proposition, mProtoTuple could be local to getNext instead of private at class level

        Show
        pablo martinez added a comment - good. Also, with your proposition, mProtoTuple could be local to getNext instead of private at class level
        Hide
        Bill Graham added a comment -

        True, although I believe it's at the class level as a performance optimization. We re-set and re-use the same list in each call to getNext.

        Show
        Bill Graham added a comment - True, although I believe it's at the class level as a performance optimization. We re-set and re-use the same list in each call to getNext.
        Hide
        pablo martinez added a comment -

        thanks for the explanation. So I'm good with your proposal as it is
        thank you very much

        Show
        pablo martinez added a comment - thanks for the explanation. So I'm good with your proposal as it is thank you very much
        Bill Graham made changes -
        Summary make readField protected to be able to override it if we extend PigStorage Make PigStorage.readField() protected
        Hide
        Bill Graham added a comment -

        Committed, thanks Pablo!

        Show
        Bill Graham added a comment - Committed, thanks Pablo!
        Bill Graham made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Release Note this change does not make it incompatible
        Assignee pablo martinez [ pablomar ]
        Fix Version/s 0.12 [ 12323380 ]
        Resolution Fixed [ 1 ]
        Hide
        pablo martinez added a comment -

        thank you !

        Show
        pablo martinez added a comment - thank you !
        Daniel Dai made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            pablo martinez
            Reporter:
            pablo martinez
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 2h
              2h
              Remaining:
              Remaining Estimate - 2h
              2h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development