Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.11
    • Component/s: None
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      Seriously, is there a more annoying pattern than the following?

      for (int i = 0; i < t.size(); i++) {
          try {
              doStuff(t.get(i));
          } catch (ExecException e) {
              throw new RuntimeException("BUT I THOUGHT PIG WAS SINGLETHREADED", e);
          }
      }
      

      I mean yeah, you can do the following:

      for (Object o : t.getAll()) {
          doStuff(o);
      }
      

      But I don't even think that should be necessary. I think the following should work:

      for (Object o : t) {
          doStuff(o);
      }
      

      It's a shame we can't give a default implementation (I either shake my fist that this won't be possible until Java 8 OR that Tuple is an interface and not an abstract class). Either way, I even added test! WOAH.

      Thoughts?

      1. PIG-2724-7.patch
        14 kB
        Jonathan Coveney
      2. PIG-2724-6.patch
        22 kB
        Jonathan Coveney
      3. PIG-2724-6.patch
        16 kB
        Jonathan Coveney
      4. PIG-2724-5.patch
        14 kB
        Jonathan Coveney
      5. PIG-2724-4.patch
        14 kB
        Jonathan Coveney
      6. PIG-2724-4.patch
        14 kB
        Jonathan Coveney
      7. PIG-2724-3.patch
        21 kB
        Jonathan Coveney
      8. PIG-2724-2.patch
        15 kB
        Jonathan Coveney
      9. PIG-2724-1.patch
        16 kB
        Jonathan Coveney
      10. PIG-2724-1.patch
        16 kB
        Jonathan Coveney
      11. PIG-2724-0.patch
        5 kB
        Jonathan Coveney

        Activity

        Hide
        Prashant Kommireddi added a comment -

        Hi Jon, patch looks good to me. Neat idea.

        Minor thing, probably you could set the size of Tuple in TestTuple as that never changes in your test.

        public void testTupleIterator() {
                Tuple t = TupleFactory.getInstance().newTuple();
        

        This can be changed to

        public void testTupleIterator() {
                Tuple t = TupleFactory.getInstance().newTuple(2000);
        
        Show
        Prashant Kommireddi added a comment - Hi Jon, patch looks good to me. Neat idea. Minor thing, probably you could set the size of Tuple in TestTuple as that never changes in your test. public void testTupleIterator() { Tuple t = TupleFactory.getInstance().newTuple(); This can be changed to public void testTupleIterator() { Tuple t = TupleFactory.getInstance().newTuple(2000);
        Hide
        Julien Le Dem added a comment -

        Good idea Jon.

        Some minor comments:

        • chain the exception in the test:
          try {
              assertEquals("Element " + i, t.get(i++), o);
          } catch (ExecException e) {
              throw new RuntimeException(e);
          }
          
        • please add the @Override annotation to the iterator() implementations.
        • To facilitate future evolutions and default implementations you can create an AbstractTuple that all tuples would extend.
          For now it would have only the iterator() method implementation.
          possibly it would have isNull(), toDelimitedString(), etc
        Show
        Julien Le Dem added a comment - Good idea Jon. Some minor comments: chain the exception in the test: try { assertEquals( "Element " + i, t.get(i++), o); } catch (ExecException e) { throw new RuntimeException(e); } please add the @Override annotation to the iterator() implementations. To facilitate future evolutions and default implementations you can create an AbstractTuple that all tuples would extend. For now it would have only the iterator() method implementation. possibly it would have isNull(), toDelimitedString(), etc
        Hide
        Jonathan Coveney added a comment -

        Oh wow, not chaining the exception was pure typo. Lame.

        Thanks for the other comments. I went ahead and made an AbstractTuple and an AbstractTypeAwareTuple. Bam.

        Show
        Jonathan Coveney added a comment - Oh wow, not chaining the exception was pure typo. Lame. Thanks for the other comments. I went ahead and made an AbstractTuple and an AbstractTypeAwareTuple. Bam.
        Hide
        Julien Le Dem added a comment -

        Thanks Jonathan.
        Overall, this looks good.
        Some extra comments:

        • In javadoc you can use the following tag whenever you want to reuse the same doc as in the implemented method (for example isNull(int))
              /**
               * {@inheritDoc}
               */
          
        • I would not necessarily create AbstractTypeAwareTuple unless there is something to add in it
        • It looks like the toDelimitedString() in DefaultTuple was doing something different (nnull would be written differently) than the new default one in AbstractTuple. We may want to make sure we keep the same behavior in case someone is parsing it. You could add a test for this.
        • hashCode() could be in AbstractTuple as well as it just needs an object iterator (that you just provided!)
        Show
        Julien Le Dem added a comment - Thanks Jonathan. Overall, this looks good. Some extra comments: In javadoc you can use the following tag whenever you want to reuse the same doc as in the implemented method (for example isNull(int)) /** * {@inheritDoc} */ I would not necessarily create AbstractTypeAwareTuple unless there is something to add in it It looks like the toDelimitedString() in DefaultTuple was doing something different (nnull would be written differently) than the new default one in AbstractTuple. We may want to make sure we keep the same behavior in case someone is parsing it. You could add a test for this. hashCode() could be in AbstractTuple as well as it just needs an object iterator (that you just provided!)
        Hide
        Jonathan Coveney added a comment -
        • Thanks for the heads up on the javadoc
        • Yeah, I was thinking about pushing in hashCode as well, but I don't like the idea of pushing in hashCode if we don't push in compareTo as well... either we have default behavior for both, or neither, IMHO.
        • I will have DefaultTuple override it and do what it used to do, and leave AbstractTuple's as the general.
        • AbstractTypeAwareTuple let's you merge together AbstractTuple+TypeAwareTuple interface. It feels cleaner than forcing classes to do that themselves?
        Show
        Jonathan Coveney added a comment - Thanks for the heads up on the javadoc Yeah, I was thinking about pushing in hashCode as well, but I don't like the idea of pushing in hashCode if we don't push in compareTo as well... either we have default behavior for both, or neither, IMHO. I will have DefaultTuple override it and do what it used to do, and leave AbstractTuple's as the general. AbstractTypeAwareTuple let's you merge together AbstractTuple+TypeAwareTuple interface. It feels cleaner than forcing classes to do that themselves?
        Hide
        Julien Le Dem added a comment -
        • np
        • You are correct but your argument for hashCode() also applies to equals(). The contract is that o1.equals(o2) => o1.hashCode() == o2.hashCode() (not the other way around). compareTo() is separate but of course o1.compareTo(o2) == 0 <=> o1.equals(o2)
          Usually hashCode and equals are implemented together
          If the individual objects are not comparable it is hard to provide a default compareTo() implementation.
          Factor out what makes sense to you. Some warning about that in the javadoc?
        • Sounds good
        • I don't think factoring the implements statement is worth the extra AbstractTypeAwareTuple class. It doesn't really improve readability.
        Show
        Julien Le Dem added a comment - np You are correct but your argument for hashCode() also applies to equals(). The contract is that o1.equals(o2) => o1.hashCode() == o2.hashCode() (not the other way around). compareTo() is separate but of course o1.compareTo(o2) == 0 <=> o1.equals(o2) Usually hashCode and equals are implemented together If the individual objects are not comparable it is hard to provide a default compareTo() implementation. Factor out what makes sense to you. Some warning about that in the javadoc? Sounds good I don't think factoring the implements statement is worth the extra AbstractTypeAwareTuple class. It doesn't really improve readability.
        Hide
        Jonathan Coveney added a comment -

        I care not about the AbstractTypeAwareTuple, so I'll axe it. I knew thee well, AbstractTypeAwareTuple.

        As far as the hashCode thing, you are correct of course on the typical equals contract, but for Tuples, idiomatically they all use compareTo instead. I do not think that this is an issue. I just think that we should force people to be thoughtful about compareTo/hashCode and either force both to be overriden, or neither.

        Show
        Jonathan Coveney added a comment - I care not about the AbstractTypeAwareTuple, so I'll axe it. I knew thee well, AbstractTypeAwareTuple. As far as the hashCode thing, you are correct of course on the typical equals contract, but for Tuples, idiomatically they all use compareTo instead. I do not think that this is an issue. I just think that we should force people to be thoughtful about compareTo/hashCode and either force both to be overriden, or neither.
        Hide
        Jonathan Coveney added a comment -

        This removes AbstractTypeAwareTuple, though it doesn't change the compareTo/hashCode piece

        Show
        Jonathan Coveney added a comment - This removes AbstractTypeAwareTuple, though it doesn't change the compareTo/hashCode piece
        Hide
        Jonathan Coveney added a comment -

        A minor change in toDelimitedString that handles nulls consistently.

        Show
        Jonathan Coveney added a comment - A minor change in toDelimitedString that handles nulls consistently.
        Hide
        Julien Le Dem added a comment -

        +1

        Show
        Julien Le Dem added a comment - +1
        Hide
        Jonathan Coveney added a comment -

        rebased and committed

        Show
        Jonathan Coveney added a comment - rebased and committed

          People

          • Assignee:
            Jonathan Coveney
            Reporter:
            Jonathan Coveney
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development