[GitHub] incubator-flink pull request: FIX-FLINK-888-swapValues() for Tuple...

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-flink pull request: FIX-FLINK-888-swapValues() for Tuple...

zentol
GitHub user tobwiens opened a pull request:

    https://github.com/apache/incubator-flink/pull/44

    FIX-FLINK-888-swapValues() for Tuple2

   

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/tobwiens/incubator-flink FIX-FLINK-888

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/incubator-flink/pull/44.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #44
   
----
commit 20657941cfc29045cad1a7d296cdd52dd61e5215
Author: TobiasWiens <[hidden email]>
Date:   2014-06-23T07:56:44Z

    FIX-FLINK-888-swapValues() for Tuple2

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-flink pull request: FIX-FLINK-888-swapValues() for Tuple...

zentol
Github user uce commented on the pull request:

    https://github.com/apache/incubator-flink/pull/44#issuecomment-47119600
 
    I suggest to create a new Tuple2 for the swap, because your current solution only works because of type erasure.
   
    The following would not work:
   
    ```java
    Tuple2<String, Integer> classUnderTestTuple2 = new Tuple2<String, Integer>(new String("Test case"), 25);
    classUnderTestTuple2.swapValues();
   
    String s1 = classUnderTestTuple2.f1; // does not work
    Integer i1 = classUnderTestTuple2.f0; // does not work
    ```
   
    So for swapValues() you would do something like `new Tuple2<T1, T0>(T1, T0)`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-flink pull request: FIX-FLINK-888-swapValues() for Tuple...

zentol
In reply to this post by zentol
Github user uce commented on a diff in the pull request:

    https://github.com/apache/incubator-flink/pull/44#discussion_r14194380
 
    --- Diff: stratosphere-java/src/test/java/eu/stratosphere/api/java/tuple/Tuple2Test.java ---
    @@ -0,0 +1,55 @@
    +/***********************************************************************************************************************
    + *
    + * Copyright (C) 2010-2013 by the Stratosphere project (http://stratosphere.eu)
    + *
    + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
    + * the License. You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
    + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
    + * specific language governing permissions and limitations under the License.
    + *
    + **********************************************************************************************************************/
    +
    +package eu.stratosphere.api.java.tuple;
    +
    +import junit.framework.Assert;
    +
    +import org.junit.Before;
    +import org.junit.Test;
    +
    +public class Tuple2Test {
    +
    + private Tuple2<String, Integer> classUnderTestTuple2;
    +
    + @Before
    + public void setUp() throws Exception {
    + classUnderTestTuple2 = new Tuple2<String, Integer>(new String("Test case"), 25);
    --- End diff --
   
    I would suggest to just move this tuple to the `testSwapValues()` method since you only use it there. I also think the name is rather verbose.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-flink pull request: FIX-FLINK-888-swapValues() for Tuple...

zentol
In reply to this post by zentol
Github user tobwiens commented on the pull request:

    https://github.com/apache/incubator-flink/pull/44#issuecomment-47127623
 
    When the VM casts dynamically the values the type specific equals method is called. That was the idea behind the test case. If the types are not correct that test will fail.
   
    You mean that I should swap an swapped Tuple2 and compare their values?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-flink pull request: FIX-FLINK-888-swapValues() for Tuple...

zentol
In reply to this post by zentol
Github user tobwiens commented on the pull request:

    https://github.com/apache/incubator-flink/pull/44#issuecomment-47194707
 
    Do you mean it like that?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-flink pull request: FIX-FLINK-888-swapValues() for Tuple...

zentol
In reply to this post by zentol
Github user uce commented on the pull request:

    https://github.com/apache/incubator-flink/pull/44#issuecomment-47204416
 
    Hey Tobias,
   
    no I meant something along those lines:
   
    ```java
    public Tuple2 swapValues() {
        return new Tuple2<T1, T0>(f1, f0);
    }
    ```
    What do you think?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-flink pull request: FIX-FLINK-888-swapValues() for Tuple...

zentol
In reply to this post by zentol
Github user tobwiens commented on the pull request:

    https://github.com/apache/incubator-flink/pull/44#issuecomment-47318464
 
    What is the benefit for the user then?
    That behavior can be implemented outside the class as well.  
   
    ```Tuple2<T1, T0> swapped = new Tuple2<T1, T0>(tuple.f1, tuple.f0);```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-flink pull request: FIX-FLINK-888-swapValues() for Tuple...

zentol
In reply to this post by zentol
Github user zentol commented on the pull request:

    https://github.com/apache/incubator-flink/pull/44#issuecomment-47318983
 
    ```
    Tupl2<T1, T0> swapped = tuple.swap();
    ```
    is alot easier to read though.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-flink pull request: FIX-FLINK-888-swapValues() for Tuple...

zentol
In reply to this post by zentol
Github user tobwiens commented on the pull request:

    https://github.com/apache/incubator-flink/pull/44#issuecomment-47320105
 
    The the other way can be implemented outside the class as well, because the the setField and getField using generic type T.
   
    I will do implement it how you said it. Thanks!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-flink pull request: FIX-FLINK-888-swapValues() for Tuple...

zentol
In reply to this post by zentol
Github user tobwiens commented on the pull request:

    https://github.com/apache/incubator-flink/pull/44#issuecomment-47326312
 
    What do you think of the new implementation?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-flink pull request: FIX-FLINK-888-swapValues() for Tuple...

zentol
In reply to this post by zentol
Github user uce commented on a diff in the pull request:

    https://github.com/apache/incubator-flink/pull/44#discussion_r14288928
 
    --- Diff: stratosphere-java/src/main/java/eu/stratosphere/api/java/tuple/Tuple2.java ---
    @@ -99,6 +99,15 @@ public void setFields(T0 value0, T1 value1) {
      this.f1 = value1;
      }
     
    + /**
    + * Returns a shallow copy of the Tuple2 with swapped values.
    + *
    + * @return A Tuple2<T1, T0> shallow copy. Value0 and value1 are swapped.
    + */
    + public Tuple2<T1, T0> swapValues() {
    + // Swap values
    --- End diff --
   
    comment doesn't add any value


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-flink pull request: FIX-FLINK-888-swapValues() for Tuple...

zentol
In reply to this post by zentol
Github user uce commented on a diff in the pull request:

    https://github.com/apache/incubator-flink/pull/44#discussion_r14288968
 
    --- Diff: stratosphere-java/src/main/java/eu/stratosphere/api/java/tuple/Tuple2.java ---
    @@ -99,6 +99,15 @@ public void setFields(T0 value0, T1 value1) {
      this.f1 = value1;
      }
     
    + /**
    + * Returns a shallow copy of the Tuple2 with swapped values.
    + *
    + * @return A Tuple2<T1, T0> shallow copy. Value0 and value1 are swapped.
    --- End diff --
   
    I think this comment is a bit ambiguous... I would say either remove the `@return` or just copy the `Shallow copy of the Tuple2 with swapped values.`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-flink pull request: FIX-FLINK-888-swapValues() for Tuple...

zentol
In reply to this post by zentol
Github user uce commented on a diff in the pull request:

    https://github.com/apache/incubator-flink/pull/44#discussion_r14288993
 
    --- Diff: stratosphere-java/src/test/java/eu/stratosphere/api/java/tuple/Tuple2Test.java ---
    @@ -0,0 +1,43 @@
    +/***********************************************************************************************************************
    + *
    + * Copyright (C) 2010-2013 by the Stratosphere project (http://stratosphere.eu)
    + *
    + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
    + * the License. You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
    + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
    + * specific language governing permissions and limitations under the License.
    + *
    + **********************************************************************************************************************/
    +
    +package eu.stratosphere.api.java.tuple;
    +
    +import junit.framework.Assert;
    +
    +import org.junit.Test;
    +
    +public class Tuple2Test {
    +
    +
    + @Test
    + public void testSwapValues() {
    + Tuple2<String, Integer> fullTuple2 = new Tuple2<String, Integer>(new String("Test case"), 25);
    + //Swapped tuple for comparison
    + Tuple2<Integer, String> swappedTuple2 = fullTuple2.swapValues();
    +
    + // Assert when not the same
    + // Use overloaded equals method to check for equality. Especially important for String.
    + if(!swappedTuple2.f1.equals(fullTuple2.getField(0))) {
    --- End diff --
   
    Can't you just use JUnit's assertEquals, i.e. `Assert.assertEquals(fullTuple2.f0, swappedTuple2.f1)`?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-flink pull request: FIX-FLINK-888-swapValues() for Tuple...

zentol
In reply to this post by zentol
Github user tobwiens commented on the pull request:

    https://github.com/apache/incubator-flink/pull/44#issuecomment-47337619
 
    I want to assert when they are not the same.
    And when I use: failNotEquals(java.lang.String message,java.lang.Object expected,java.lang.Object actual)
    It uses Object's equals method.
   
    I could write:
    // assert if NOT equal
    Assert.assertTrue(!swappedTuple2.f1.equals(fullTuple2.f0)) to make it more consistent and more obvious?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-flink pull request: FIX-FLINK-888-swapValues() for Tuple...

zentol
In reply to this post by zentol
Github user zentol commented on the pull request:

    https://github.com/apache/incubator-flink/pull/44#issuecomment-47366280
 
    if you use ```Assert.assertEquals(fullTuple2.f0, swappedTuple2.f1)``` the test will fail if they are not the same, just as your current code. you save six lines, and it's more obvious what you want to check.
   
    your test is supposed to make sure that the swapped values are equal, thus you should check exactly that.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-flink pull request: FIX-FLINK-888-swapValues() for Tuple...

zentol
In reply to this post by zentol
Github user tobwiens commented on the pull request:

    https://github.com/apache/incubator-flink/pull/44#issuecomment-47549619
 
    Obviously! Thank you!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---