Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.10.0
    • Fix Version/s: 0.12.0
    • Component/s: piggybank
    • Labels:
      None

      Description

      Close to DIFF function but SUBTRACT(bag1, bag2) will subtract elements of bag2 from bag1.

      1. PIG-2881.patch
        16 kB
        Joel Costigliola

        Activity

        Hide
        Joel Costigliola added a comment -

        The tests relies on fest assert https://github.com/alexruiz/fest-assert-2.x but can easily be rewritten with Hamcrest I guess.

        Show
        Joel Costigliola added a comment - The tests relies on fest assert https://github.com/alexruiz/fest-assert-2.x but can easily be rewritten with Hamcrest I guess.
        Hide
        Alan Gates added a comment -

        Patch looks fine, except I don't see a reason to introduce new libraries just for asserts. We can use the existing assert functionality in junit.

        Show
        Alan Gates added a comment - Patch looks fine, except I don't see a reason to introduce new libraries just for asserts. We can use the existing assert functionality in junit.
        Hide
        Joel Costigliola added a comment -

        I agree with you, I should have sent tests with basic JUnit assertions.
        Truth is I was just lazy to rewrite it but I have more time now, so I can send you a test version withour Fest assertions.
        You tell em.

        Show
        Joel Costigliola added a comment - I agree with you, I should have sent tests with basic JUnit assertions. Truth is I was just lazy to rewrite it but I have more time now, so I can send you a test version withour Fest assertions. You tell em.
        Hide
        Joel Costigliola added a comment -

        new version of test depending only on JUnit assertions.

        Show
        Joel Costigliola added a comment - new version of test depending only on JUnit assertions.
        Hide
        Dmitriy V. Ryaboy added a comment -

        Submitting so it shows up in the patch queue

        Show
        Dmitriy V. Ryaboy added a comment - Submitting so it shows up in the patch queue
        Hide
        Jonathan Coveney added a comment -

        Joel,

        This looks fine, but for the final oomph, can you do the following:

        • Put it in Pig w/the proper package (either org.apache.pig.builtin or the Piggybank)
        • Generate a diff against trunk

        This will make it so people can download and see exactly what is being applied. We like to avoid cases where many steps need to be taken to exactly replicate what is in trunk.

        Thanks for contributing
        Jon

        Show
        Jonathan Coveney added a comment - Joel, This looks fine, but for the final oomph, can you do the following: Put it in Pig w/the proper package (either org.apache.pig.builtin or the Piggybank) Generate a diff against trunk This will make it so people can download and see exactly what is being applied. We like to avoid cases where many steps need to be taken to exactly replicate what is in trunk. Thanks for contributing Jon
        Hide
        Joel Costigliola added a comment -

        Hi Jon,

        I'll try to do that this week end.

        Regards,

        Joel

        Show
        Joel Costigliola added a comment - Hi Jon, I'll try to do that this week end. Regards, Joel
        Hide
        Joel Costigliola added a comment -

        Jon,

        It turns out that the trunk (rev 1403547) did not pass the following tests on my machine

        [junit] Running org.apache.pig.test.TestLoad
        [junit] Tests run: 15, Failures: 9, Errors: 0, Time elapsed: 59.125 sec
        
        [junit] Running org.apache.pig.test.TestStore
        [junit] Tests run: 17, Failures: 2, Errors: 0, Time elapsed: 406.874 sec
        

        My environment is :

        • Apache Ant(TM) version 1.8.2 compiled on December 3 2011
        • Java version: 1.6.0_17, vendor: Sun Microsystems Inc.
        • Default locale: en_GB, platform encoding: UTF-8
        • OS name: "linux", version: "3.2.0-33-generic", arch: "amd64", family: "unix"

        I have improved SUBTRACT unit tests, run all the tests and did not have more failing tests than the one I mentioned.

        Question : do you want me to make a patch or wait until all tests pass on my machine ?

        Regards,

        Joel

        Show
        Joel Costigliola added a comment - Jon, It turns out that the trunk (rev 1403547) did not pass the following tests on my machine [junit] Running org.apache.pig.test.TestLoad [junit] Tests run: 15, Failures: 9, Errors: 0, Time elapsed: 59.125 sec [junit] Running org.apache.pig.test.TestStore [junit] Tests run: 17, Failures: 2, Errors: 0, Time elapsed: 406.874 sec My environment is : Apache Ant(TM) version 1.8.2 compiled on December 3 2011 Java version: 1.6.0_17, vendor: Sun Microsystems Inc. Default locale: en_GB, platform encoding: UTF-8 OS name: "linux", version: "3.2.0-33-generic", arch: "amd64", family: "unix" I have improved SUBTRACT unit tests, run all the tests and did not have more failing tests than the one I mentioned. Question : do you want me to make a patch or wait until all tests pass on my machine ? Regards, Joel
        Hide
        Cheolsoo Park added a comment -

        Hi Joel, what errors do you see in the test logs for your test failures?

        build/test/logs/TEST-org.apache.pig.test.TestLoad.txt
        build/test/logs/TEST-org.apache.pig.test.TestStore.txt

        Both tests use MiniCluster, so they may be failing due to environment issues. For example, did you set "umask 0022" in the shell where you're running tests?

        Show
        Cheolsoo Park added a comment - Hi Joel, what errors do you see in the test logs for your test failures? build/test/logs/TEST-org.apache.pig.test.TestLoad.txt build/test/logs/TEST-org.apache.pig.test.TestStore.txt Both tests use MiniCluster, so they may be failing due to environment issues. For example, did you set "umask 0022" in the shell where you're running tests?
        Hide
        Joel Costigliola added a comment -

        No I did not set umask 0022 because it was not mentioned to do so in the contributor guide : https://cwiki.apache.org/PIG/howtocontribute.html.

        TestStore failures :

        Testcase: testStoreRemoteRel took 0.028 sec
        	FAILED
        expected:<...> but was:<hdfs://joe-desktop:43074...>
        
        ...
        
        Testcase: testStoreRemoteRelScheme took 0.04 sec
        	FAILED
        expected:<...> but was:<hdfs://joe-desktop:43074...>
        

        TestLoad failures :

        Testcase: testLoadRemoteRel took 0.333 sec
        	FAILED
        expected:<[hdfs://joe-desktop:49682]/tmp/test> but was:<[]/tmp/test>
        junit.framework.AssertionFailedError: expected:<[hdfs://joe-desktop:49682]/tmp/test> but was:<[]/tmp/test>
        	at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:332)
        	at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:300)
        	at org.apache.pig.test.TestLoad.testLoadRemoteRel(TestLoad.java:122)
        
        Testcase: testLoadRemoteAbs took 0.074 sec
        Testcase: testLoadRemoteRelScheme took 0.03 sec
        	FAILED
        expected:<[hdfs://joe-desktop:49682]/tmp/test> but was:<[]/tmp/test>
        junit.framework.AssertionFailedError: expected:<[hdfs://joe-desktop:49682]/tmp/test> but was:<[]/tmp/test>
        	at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:332)
        	at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:300)
        	at org.apache.pig.test.TestLoad.testLoadRemoteRelScheme(TestLoad.java:139)
        
        Testcase: testLoadRemoteAbsScheme took 27.36 sec
        Testcase: testLoadRemoteAbsAuth took 0.083 sec
        	FAILED
        expected:<[hdfs://joe-desktop:49682]/test> but was:<[]/test>
        junit.framework.AssertionFailedError: expected:<[hdfs://joe-desktop:49682]/test> but was:<[]/test>
        	at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:332)
        	at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:300)
        	at org.apache.pig.test.TestLoad.testLoadRemoteAbsAuth(TestLoad.java:158)
        
        Testcase: testLoadRemoteNormalize took 0.041 sec
        Testcase: testGlobChars took 0.035 sec
        	FAILED
        expected:<[hdfs://joe-desktop:49682]/tmp/t?s*> but was:<[]/tmp/t?s*>
        junit.framework.AssertionFailedError: expected:<[hdfs://joe-desktop:49682]/tmp/t?s*> but was:<[]/tmp/t?s*>
        	at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:332)
        	at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:300)
        	at org.apache.pig.test.TestLoad.testGlobChars(TestLoad.java:174)
        
        Testcase: testCommaSeparatedString took 0.031 sec
        	FAILED
        expected:<[hdfs://joe-desktop:49682/tmp/usr/pig/a,hdfs://joe-desktop:49682]/tmp/usr/pig/b> but was:<[/tmp/usr/pig/a,]/tmp/usr/pig/b>
        junit.framework.AssertionFailedError: expected:<[hdfs://joe-desktop:49682/tmp/usr/pig/a,hdfs://joe-desktop:49682]/tmp/usr/pig/b> but was:<[/tmp/usr/pig/a,]/tmp/usr/pig/b>
        	at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:332)
        	at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:300)
        	at org.apache.pig.test.TestLoad.testCommaSeparatedString(TestLoad.java:182)
        
        Testcase: testCommaSeparatedString2 took 0.045 sec
        	FAILED
        expected:<[hdfs://joe-desktop:49682/tmp/t?s*,hdfs://joe-desktop:49682]/tmp/test> but was:<[/tmp/t?s*,]/tmp/test>
        junit.framework.AssertionFailedError: expected:<[hdfs://joe-desktop:49682/tmp/t?s*,hdfs://joe-desktop:49682]/tmp/test> but was:<[/tmp/t?s*,]/tmp/test>
        	at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:332)
        	at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:300)
        	at org.apache.pig.test.TestLoad.testCommaSeparatedString2(TestLoad.java:190)
        
        Testcase: testCommaSeparatedString3 took 23.653 sec
        Testcase: testCommaSeparatedString4 took 0.094 sec
        	FAILED
        expected:<[hdfs://joe-desktop:49682/tmp/usr/pig/{a,c},hdfs://joe-desktop:49682]/tmp/usr/pig/b> but was:<[/tmp/usr/pig/{a,c},]/tmp/usr/pig/b>
        junit.framework.AssertionFailedError: expected:<[hdfs://joe-desktop:49682/tmp/usr/pig/{a,c},hdfs://joe-desktop:49682]/tmp/usr/pig/b> but was:<[/tmp/usr/pig/{a,c},]/tmp/usr/pig/b>
        	at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:332)
        	at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:300)
        	at org.apache.pig.test.TestLoad.testCommaSeparatedString4(TestLoad.java:218)
        
        Testcase: testCommaSeparatedString5 took 0.035 sec
        	FAILED
        expected:</usr/pig/{a,c},[hdfs://joe-desktop:49682]/tmp/usr/pig/b> but was:</usr/pig/{a,c},[]/tmp/usr/pig/b>
        junit.framework.AssertionFailedError: expected:</usr/pig/{a,c},[hdfs://joe-desktop:49682]/tmp/usr/pig/b> but was:</usr/pig/{a,c},[]/tmp/usr/pig/b>
        	at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:332)
        	at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:300)
        	at org.apache.pig.test.TestLoad.testCommaSeparatedString5(TestLoad.java:226)
        
        Testcase: testCommaSeparatedString6 took 0.04 sec
        	FAILED
        expected:<[hdfs://joe-desktop:49682]/tmp/usr/pig/{a,c},/...> but was:<[]/tmp/usr/pig/{a,c},/...>
        junit.framework.AssertionFailedError: expected:<[hdfs://joe-desktop:49682]/tmp/usr/pig/{a,c},/...> but was:<[]/tmp/usr/pig/{a,c},/...>
        	at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:332)
        	at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:300)
        	at org.apache.pig.test.TestLoad.testCommaSeparatedString6(TestLoad.java:246)
        
        Testcase: testNonDfsLocation took 0.06 sec
        

        I can attach the log files if you want (note that TestStore.txt is 1.1MB large).

        Show
        Joel Costigliola added a comment - No I did not set umask 0022 because it was not mentioned to do so in the contributor guide : https://cwiki.apache.org/PIG/howtocontribute.html . TestStore failures : Testcase: testStoreRemoteRel took 0.028 sec FAILED expected:<...> but was:<hdfs://joe-desktop:43074...> ... Testcase: testStoreRemoteRelScheme took 0.04 sec FAILED expected:<...> but was:<hdfs://joe-desktop:43074...> TestLoad failures : Testcase: testLoadRemoteRel took 0.333 sec FAILED expected:<[hdfs://joe-desktop:49682]/tmp/test> but was:<[]/tmp/test> junit.framework.AssertionFailedError: expected:<[hdfs://joe-desktop:49682]/tmp/test> but was:<[]/tmp/test> at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:332) at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:300) at org.apache.pig.test.TestLoad.testLoadRemoteRel(TestLoad.java:122) Testcase: testLoadRemoteAbs took 0.074 sec Testcase: testLoadRemoteRelScheme took 0.03 sec FAILED expected:<[hdfs://joe-desktop:49682]/tmp/test> but was:<[]/tmp/test> junit.framework.AssertionFailedError: expected:<[hdfs://joe-desktop:49682]/tmp/test> but was:<[]/tmp/test> at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:332) at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:300) at org.apache.pig.test.TestLoad.testLoadRemoteRelScheme(TestLoad.java:139) Testcase: testLoadRemoteAbsScheme took 27.36 sec Testcase: testLoadRemoteAbsAuth took 0.083 sec FAILED expected:<[hdfs://joe-desktop:49682]/test> but was:<[]/test> junit.framework.AssertionFailedError: expected:<[hdfs://joe-desktop:49682]/test> but was:<[]/test> at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:332) at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:300) at org.apache.pig.test.TestLoad.testLoadRemoteAbsAuth(TestLoad.java:158) Testcase: testLoadRemoteNormalize took 0.041 sec Testcase: testGlobChars took 0.035 sec FAILED expected:<[hdfs://joe-desktop:49682]/tmp/t?s*> but was:<[]/tmp/t?s*> junit.framework.AssertionFailedError: expected:<[hdfs://joe-desktop:49682]/tmp/t?s*> but was:<[]/tmp/t?s*> at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:332) at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:300) at org.apache.pig.test.TestLoad.testGlobChars(TestLoad.java:174) Testcase: testCommaSeparatedString took 0.031 sec FAILED expected:<[hdfs://joe-desktop:49682/tmp/usr/pig/a,hdfs://joe-desktop:49682]/tmp/usr/pig/b> but was:<[/tmp/usr/pig/a,]/tmp/usr/pig/b> junit.framework.AssertionFailedError: expected:<[hdfs://joe-desktop:49682/tmp/usr/pig/a,hdfs://joe-desktop:49682]/tmp/usr/pig/b> but was:<[/tmp/usr/pig/a,]/tmp/usr/pig/b> at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:332) at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:300) at org.apache.pig.test.TestLoad.testCommaSeparatedString(TestLoad.java:182) Testcase: testCommaSeparatedString2 took 0.045 sec FAILED expected:<[hdfs://joe-desktop:49682/tmp/t?s*,hdfs://joe-desktop:49682]/tmp/test> but was:<[/tmp/t?s*,]/tmp/test> junit.framework.AssertionFailedError: expected:<[hdfs://joe-desktop:49682/tmp/t?s*,hdfs://joe-desktop:49682]/tmp/test> but was:<[/tmp/t?s*,]/tmp/test> at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:332) at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:300) at org.apache.pig.test.TestLoad.testCommaSeparatedString2(TestLoad.java:190) Testcase: testCommaSeparatedString3 took 23.653 sec Testcase: testCommaSeparatedString4 took 0.094 sec FAILED expected:<[hdfs://joe-desktop:49682/tmp/usr/pig/{a,c},hdfs://joe-desktop:49682]/tmp/usr/pig/b> but was:<[/tmp/usr/pig/{a,c},]/tmp/usr/pig/b> junit.framework.AssertionFailedError: expected:<[hdfs://joe-desktop:49682/tmp/usr/pig/{a,c},hdfs://joe-desktop:49682]/tmp/usr/pig/b> but was:<[/tmp/usr/pig/{a,c},]/tmp/usr/pig/b> at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:332) at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:300) at org.apache.pig.test.TestLoad.testCommaSeparatedString4(TestLoad.java:218) Testcase: testCommaSeparatedString5 took 0.035 sec FAILED expected:</usr/pig/{a,c},[hdfs://joe-desktop:49682]/tmp/usr/pig/b> but was:</usr/pig/{a,c},[]/tmp/usr/pig/b> junit.framework.AssertionFailedError: expected:</usr/pig/{a,c},[hdfs://joe-desktop:49682]/tmp/usr/pig/b> but was:</usr/pig/{a,c},[]/tmp/usr/pig/b> at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:332) at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:300) at org.apache.pig.test.TestLoad.testCommaSeparatedString5(TestLoad.java:226) Testcase: testCommaSeparatedString6 took 0.04 sec FAILED expected:<[hdfs://joe-desktop:49682]/tmp/usr/pig/{a,c},/...> but was:<[]/tmp/usr/pig/{a,c},/...> junit.framework.AssertionFailedError: expected:<[hdfs://joe-desktop:49682]/tmp/usr/pig/{a,c},/...> but was:<[]/tmp/usr/pig/{a,c},/...> at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:332) at org.apache.pig.test.TestLoad.checkLoadPath(TestLoad.java:300) at org.apache.pig.test.TestLoad.testCommaSeparatedString6(TestLoad.java:246) Testcase: testNonDfsLocation took 0.06 sec I can attach the log files if you want (note that TestStore.txt is 1.1MB large).
        Hide
        Cheolsoo Park added a comment -

        Yes, please attach the logs. Thanks!

        Show
        Cheolsoo Park added a comment - Yes, please attach the logs. Thanks!
        Hide
        Joel Costigliola added a comment -

        Logs attached, resulting from playing the test without umask set to any special value.

        Show
        Joel Costigliola added a comment - Logs attached, resulting from playing the test without umask set to any special value.
        Hide
        Cheolsoo Park added a comment -

        Hi Joel,

        I think that I know what's the problem. In fact, this is an issue with the test code as I see it. Let's look at one of error messages:

        expected:<[hdfs://joe-desktop:49682]/test> but was:<[]/test>
        

        If you look at the test code in TestLoad.java, hdfs://joe-desktop:49682/ is supposed to be replaced by / as follows:

        p.replaceAll("hdfs://[0-9a-zA-Z:\\.]*/", "/")
        

        But it isn't. Why? Because your hostname joe-desktop:49682 includes -, so it doesn't match the reg. exp. [0-9a-zA-Z:\\.]*.

        So either you can change your hostname removing -, or you can fix the reg. exp. so that your hostname can be matched. I would recommend to do the later.

        Thanks!

        Show
        Cheolsoo Park added a comment - Hi Joel, I think that I know what's the problem. In fact, this is an issue with the test code as I see it. Let's look at one of error messages: expected:<[hdfs: //joe-desktop:49682]/test> but was:<[]/test> If you look at the test code in TestLoad.java , hdfs://joe-desktop:49682/ is supposed to be replaced by / as follows: p.replaceAll( "hdfs: //[0-9a-zA-Z:\\.]*/" , "/" ) But it isn't. Why? Because your hostname joe-desktop:49682 includes - , so it doesn't match the reg. exp. [0-9a-zA-Z:\\.] * . So either you can change your hostname removing - , or you can fix the reg. exp. so that your hostname can be matched. I would recommend to do the later. Thanks!
        Hide
        Joel Costigliola added a comment -

        Hi Cheolsoo,

        All tests pass after the regex change, thanks.

        To be more precise, I have made a minor refactoring on the TestLoad to avoid duplication in checkLoadPath, here's what I've changed :

        if(noConversionExpected) {
            assertEquals(p, expected);
        } else  {
            String protocol = pc.getExecType() == ExecType.MAPREDUCE ? "hdfs" : "file";
            // regex : A word character, i.e. [a-zA-Z_0-9] or '-' followed by ':' then any characters 
            String regex = "[\\-\\w:\\.]";
            assertTrue(p.matches(".*" + protocol + "://" + regex + "*.*"));
            assertEquals(p.replaceAll(protocol + "://" + regex + "*/", "/"), expected);
        }
        

        If you are ok with that, tell I'll make the patch with SUBTRACT function and those changes.

        Joel

        Show
        Joel Costigliola added a comment - Hi Cheolsoo, All tests pass after the regex change, thanks. To be more precise, I have made a minor refactoring on the TestLoad to avoid duplication in checkLoadPath, here's what I've changed : if (noConversionExpected) { assertEquals(p, expected); } else { String protocol = pc.getExecType() == ExecType.MAPREDUCE ? "hdfs" : "file" ; // regex : A word character, i.e. [a-zA-Z_0-9] or '-' followed by ':' then any characters String regex = "[\\-\\w:\\.]" ; assertTrue(p.matches( ".*" + protocol + ": //" + regex + "*.*" )); assertEquals(p.replaceAll(protocol + ": //" + regex + "*/" , "/" ), expected); } If you are ok with that, tell I'll make the patch with SUBTRACT function and those changes. Joel
        Hide
        Cheolsoo Park added a comment -

        Hi Joel,

        That looks good. Thank you very much for fixing the tests! I assume that you would fix TestStore.java as well?

        One minor comments:

        • Please change the assertEquals to the following:
          assertEquals(expected, p.replaceAll(protocol + "://" + regex + "*/", "/"));
          
        • I noticed that you used 2 spaces to indent code for your original patch. Please use 4 spaces.

        I will review and commit it when you upload your patch.

        Show
        Cheolsoo Park added a comment - Hi Joel, That looks good. Thank you very much for fixing the tests! I assume that you would fix TestStore.java as well? One minor comments: Please change the assertEquals to the following: assertEquals(expected, p.replaceAll(protocol + ": //" + regex + "*/" , "/" )); I noticed that you used 2 spaces to indent code for your original patch. Please use 4 spaces. I will review and commit it when you upload your patch.
        Hide
        Joel Costigliola added a comment -

        Yes I did fix TestStore.

        I will switch assertEquals parameters and upload the patch.

        Show
        Joel Costigliola added a comment - Yes I did fix TestStore. I will switch assertEquals parameters and upload the patch.
        Hide
        Joel Costigliola added a comment -

        Patch attached

        Show
        Joel Costigliola added a comment - Patch attached
        Hide
        Cheolsoo Park added a comment -

        Hi Joel,

        Thanks for uploading the patch. It looks really good. Two minor comments:

        • Can you please use Unix style newline chars?
        • Can you please remove trailing white spaces?

        I am not supposed to modify your patch in any way, so please forgive me for asking you do extra work.

        A bigger question that I had was whether we should create a new suite (TestSUBTRACTFunc) or add new test cases to the existing TestBuiltin suite. Given that TestBuiltin is near 3k line long, I like to break this test suite into smaller pieces and make your patch be the 1st step toward that goal. But I need to know what other people think. I probably ask it on the dev mailing list. Please feel free to provide your opinion.

        Thanks!

        Show
        Cheolsoo Park added a comment - Hi Joel, Thanks for uploading the patch. It looks really good. Two minor comments: Can you please use Unix style newline chars? Can you please remove trailing white spaces? I am not supposed to modify your patch in any way, so please forgive me for asking you do extra work. A bigger question that I had was whether we should create a new suite (TestSUBTRACTFunc) or add new test cases to the existing TestBuiltin suite. Given that TestBuiltin is near 3k line long, I like to break this test suite into smaller pieces and make your patch be the 1st step toward that goal. But I need to know what other people think. I probably ask it on the dev mailing list. Please feel free to provide your opinion. Thanks!
        Hide
        Joel Costigliola added a comment -

        Ok, I'll do the final modifications to have a clean patch.

        I would definitely split the tests, because it will be easier to find/maintain them and it will also be possible to run only the tests you are working on instead of the complete builtin suite.

        Show
        Joel Costigliola added a comment - Ok, I'll do the final modifications to have a clean patch. I would definitely split the tests, because it will be easier to find/maintain them and it will also be possible to run only the tests you are working on instead of the complete builtin suite.
        Hide
        Joel Costigliola added a comment -

        Patch updated.

        Show
        Joel Costigliola added a comment - Patch updated.
        Hide
        Cheolsoo Park added a comment -

        Hi Joel,

        OK, I asked that on the dev list. We should keep the test as a separate file, but we should change its location and change its name.

        • Please move the test to o.a.pig.builtin.test
        • Please change the name to TestSUBTRACT.java.

        I saw your comment regarding the test name, but if we move it to o.a.pig.builtin.test, we won't have a confusion since the other TestSubtract.java is under o.a.pig.test. Agreed?

        Here are few other minor comments:

        1) There is a typo in your patch.

        TestSUBTRACTFunc.java
        import static org.junit.Assert.assertTrueS; // 'S' should be removed.
        

        2) The following import statement is not used. Please remove:

        TestSUBTRACTFunc.java
        import org.junit.Assert;
        

        3) Please update the doc. Please look for the 'Eval Function' section in src/docs/src/documentation/content/xdocs/func.xml. To build it, run:

        ant docs -Dforrest.home=<FORREST_HOME>
        

        This will generate ./src/docs/build/site/func.html.

        4) Lastly, it would be better if you could re-name each test case as "testBlah". I won't insist on this, so you can leave them as they are if you want to.

        Thanks!

        Show
        Cheolsoo Park added a comment - Hi Joel, OK, I asked that on the dev list. We should keep the test as a separate file, but we should change its location and change its name. Please move the test to o.a.pig.builtin.test Please change the name to TestSUBTRACT.java . I saw your comment regarding the test name, but if we move it to o.a.pig.builtin.test , we won't have a confusion since the other TestSubtract.java is under o.a.pig.test . Agreed? Here are few other minor comments: 1) There is a typo in your patch. TestSUBTRACTFunc.java import static org.junit.Assert.assertTrueS; // 'S' should be removed. 2) The following import statement is not used. Please remove: TestSUBTRACTFunc.java import org.junit.Assert; 3) Please update the doc. Please look for the 'Eval Function' section in src/docs/src/documentation/content/xdocs/func.xml . To build it, run: ant docs -Dforrest.home=<FORREST_HOME> This will generate ./src/docs/build/site/func.html . 4) Lastly, it would be better if you could re-name each test case as "testBlah". I won't insist on this, so you can leave them as they are if you want to. Thanks!
        Hide
        Joel Costigliola added a comment -

        I agree, one question though, there is no test directory under test/org/apache/pig/builtin/, should I create one ?

        Show
        Joel Costigliola added a comment - I agree, one question though, there is no test directory under test/org/apache/pig/builtin/, should I create one ?
        Hide
        Cheolsoo Park added a comment -

        I am sorry that I wasn't clear. I think that we can put the test under ./test/org/apache/pig/builtin/. Right now I see one file (TestFunctionWrapperEvalFunc.java) in there. Since o.a.pig.builtin is the package that we're adding a new file, that seems like the right place to me.

        Show
        Cheolsoo Park added a comment - I am sorry that I wasn't clear. I think that we can put the test under ./test/org/apache/pig/builtin/ . Right now I see one file ( TestFunctionWrapperEvalFunc.java ) in there. Since o.a.pig.builtin is the package that we're adding a new file, that seems like the right place to me.
        Hide
        Joel Costigliola added a comment -

        Ok, I have moved TestSUBTRACT to ./test/org/apache/pig/builtin/

        I have updated func.xml but I can't the site, forrest always throw me this OutOfMemoryError

             [exec] * [16/28]   [0/0]     10.501s 0b      basic.pdf
             [exec] Exception in thread "main" java.lang.OutOfMemoryError: Java heap space
             [exec] 	at org.apache.fop.layoutmgr.BlockStackingLayoutManager.wrapPositionElement(BlockStackingLayoutManager.java:1499)
             [exec] 	at org.apache.fop.layoutmgr.BlockStackingLayoutManager.wrapPositionElements(BlockStackingLayoutManager.java:1487)
             [exec] 	at org.apache.fop.layoutmgr.BlockStackingLayoutManager.wrapPositionElements(BlockStackingLayoutManager.java:1471)
             [exec] 	at org.apache.fop.layoutmgr.FlowLayoutManager.getNextKnuthElements(FlowLayoutManager.java:117)
             [exec] 	at org.apache.fop.layoutmgr.PageBreaker.getNextKnuthElements(PageBreaker.java:145)
             [exec] 	at org.apache.fop.layoutmgr.AbstractBreaker.getNextBlockList(AbstractBreaker.java:552)
             [exec] 	at org.apache.fop.layoutmgr.PageBreaker.getNextBlockList(PageBreaker.java:137)
             [exec] 	at org.apache.fop.layoutmgr.AbstractBreaker.doLayout(AbstractBreaker.java:302)
             [exec] 	at org.apache.fop.layoutmgr.AbstractBreaker.doLayout(AbstractBreaker.java:264)
             [exec] 	at org.apache.fop.layoutmgr.PageSequenceLayoutManager.activateLayout(PageSequenceLayoutManager.java:106)
             [exec] 	at org.apache.fop.area.AreaTreeHandler.endPageSequence(AreaTreeHandler.java:234)
             [exec] 	at org.apache.fop.fo.pagination.PageSequence.endOfNode(PageSequence.java:123)
             [exec] 	at org.apache.fop.fo.FOTreeBuilder$MainFOHandler.endElement(FOTreeBuilder.java:340)
             [exec] 	at org.apache.fop.fo.FOTreeBuilder.endElement(FOTreeBuilder.java:169)
             [exec] 	at org.apache.cocoon.xml.AbstractXMLPipe.endElement(AbstractXMLPipe.java:112)
             [exec] 	at org.apache.cocoon.xml.AbstractXMLPipe.endElement(AbstractXMLPipe.java:112)
             [exec] 	at org.apache.cocoon.xml.xlink.XLinkPipe.endElement(XLinkPipe.java:212)
             [exec] 	at org.apache.cocoon.xml.AbstractXMLPipe.endElement(AbstractXMLPipe.java:112)
             [exec] 	at org.apache.cocoon.transformation.I18nTransformer.endElement(I18nTransformer.java:1193)
             [exec] 	at org.apache.cocoon.components.EnvironmentChanger.endElement(EnvironmentStack.java:148)
             [exec] 	at org.apache.xml.serializer.ToXMLSAXHandler.endElement(ToXMLSAXHandler.java:263)
             [exec] 	at org.apache.xalan.templates.ElemLiteralResult.execute(ElemLiteralResult.java:1401)
             [exec] 	at org.apache.xalan.transformer.TransformerImpl.executeChildTemplates(TransformerImpl.java:2400)
             [exec] 	at org.apache.xalan.templates.ElemLiteralResult.execute(ElemLiteralResult.java:1376)
             [exec] 	at org.apache.xalan.transformer.TransformerImpl.executeChildTemplates(TransformerImpl.java:2400)
             [exec] 	at org.apache.xalan.transformer.TransformerImpl.applyTemplateToNode(TransformerImpl.java:2270)
             [exec] 	at org.apache.xalan.transformer.TransformerImpl.transformNode(TransformerImpl.java:1356)
             [exec] 	at org.apache.xalan.transformer.TransformerImpl.run(TransformerImpl.java:3447)
             [exec] 	at org.apache.xalan.transformer.TransformerHandlerImpl.endDocument(TransformerHandlerImpl.java:408)
             [exec] 	at org.apache.cocoon.xml.AbstractXMLPipe.endDocument(AbstractXMLPipe.java:56)
             [exec] 	at org.apache.cocoon.transformation.TraxTransformer.endDocument(TraxTransformer.java:586)
             [exec] 	at org.apache.cocoon.xml.AbstractXMLPipe.endDocument(AbstractXMLPipe.java:56)
        

        I haven't been able to set Xmx memory options in forrest yet, can you help me on that ?
        I have to say that the doc process is painful

        Show
        Joel Costigliola added a comment - Ok, I have moved TestSUBTRACT to ./test/org/apache/pig/builtin/ I have updated func.xml but I can't the site, forrest always throw me this OutOfMemoryError [exec] * [16/28] [0/0] 10.501s 0b basic.pdf [exec] Exception in thread "main" java.lang.OutOfMemoryError: Java heap space [exec] at org.apache.fop.layoutmgr.BlockStackingLayoutManager.wrapPositionElement(BlockStackingLayoutManager.java:1499) [exec] at org.apache.fop.layoutmgr.BlockStackingLayoutManager.wrapPositionElements(BlockStackingLayoutManager.java:1487) [exec] at org.apache.fop.layoutmgr.BlockStackingLayoutManager.wrapPositionElements(BlockStackingLayoutManager.java:1471) [exec] at org.apache.fop.layoutmgr.FlowLayoutManager.getNextKnuthElements(FlowLayoutManager.java:117) [exec] at org.apache.fop.layoutmgr.PageBreaker.getNextKnuthElements(PageBreaker.java:145) [exec] at org.apache.fop.layoutmgr.AbstractBreaker.getNextBlockList(AbstractBreaker.java:552) [exec] at org.apache.fop.layoutmgr.PageBreaker.getNextBlockList(PageBreaker.java:137) [exec] at org.apache.fop.layoutmgr.AbstractBreaker.doLayout(AbstractBreaker.java:302) [exec] at org.apache.fop.layoutmgr.AbstractBreaker.doLayout(AbstractBreaker.java:264) [exec] at org.apache.fop.layoutmgr.PageSequenceLayoutManager.activateLayout(PageSequenceLayoutManager.java:106) [exec] at org.apache.fop.area.AreaTreeHandler.endPageSequence(AreaTreeHandler.java:234) [exec] at org.apache.fop.fo.pagination.PageSequence.endOfNode(PageSequence.java:123) [exec] at org.apache.fop.fo.FOTreeBuilder$MainFOHandler.endElement(FOTreeBuilder.java:340) [exec] at org.apache.fop.fo.FOTreeBuilder.endElement(FOTreeBuilder.java:169) [exec] at org.apache.cocoon.xml.AbstractXMLPipe.endElement(AbstractXMLPipe.java:112) [exec] at org.apache.cocoon.xml.AbstractXMLPipe.endElement(AbstractXMLPipe.java:112) [exec] at org.apache.cocoon.xml.xlink.XLinkPipe.endElement(XLinkPipe.java:212) [exec] at org.apache.cocoon.xml.AbstractXMLPipe.endElement(AbstractXMLPipe.java:112) [exec] at org.apache.cocoon.transformation.I18nTransformer.endElement(I18nTransformer.java:1193) [exec] at org.apache.cocoon.components.EnvironmentChanger.endElement(EnvironmentStack.java:148) [exec] at org.apache.xml.serializer.ToXMLSAXHandler.endElement(ToXMLSAXHandler.java:263) [exec] at org.apache.xalan.templates.ElemLiteralResult.execute(ElemLiteralResult.java:1401) [exec] at org.apache.xalan.transformer.TransformerImpl.executeChildTemplates(TransformerImpl.java:2400) [exec] at org.apache.xalan.templates.ElemLiteralResult.execute(ElemLiteralResult.java:1376) [exec] at org.apache.xalan.transformer.TransformerImpl.executeChildTemplates(TransformerImpl.java:2400) [exec] at org.apache.xalan.transformer.TransformerImpl.applyTemplateToNode(TransformerImpl.java:2270) [exec] at org.apache.xalan.transformer.TransformerImpl.transformNode(TransformerImpl.java:1356) [exec] at org.apache.xalan.transformer.TransformerImpl.run(TransformerImpl.java:3447) [exec] at org.apache.xalan.transformer.TransformerHandlerImpl.endDocument(TransformerHandlerImpl.java:408) [exec] at org.apache.cocoon.xml.AbstractXMLPipe.endDocument(AbstractXMLPipe.java:56) [exec] at org.apache.cocoon.transformation.TraxTransformer.endDocument(TraxTransformer.java:586) [exec] at org.apache.cocoon.xml.AbstractXMLPipe.endDocument(AbstractXMLPipe.java:56) I haven't been able to set Xmx memory options in forrest yet, can you help me on that ? I have to say that the doc process is painful
        Hide
        Joel Costigliola added a comment -

        Ok, finally found how to set up memory options in forrest (by modifying forrest.build.xml directly which is not that clean).

        Anyway, I have attached the new patch, tell me if it's ok.

        Show
        Joel Costigliola added a comment - Ok, finally found how to set up memory options in forrest (by modifying forrest.build.xml directly which is not that clean). Anyway, I have attached the new patch, tell me if it's ok.
        Hide
        Cheolsoo Park added a comment -

        Hi Joel,

        +1.

        Thanks you so much for your patience. The patch looks great! All tests pass too.

        I have a super minor comment on the doc. I think that the functions in the doc are listed in alphabetical order, but your SUBTRACT comes before IsEmpty. I guess that you added it there because SUBTRACT is similar to DIFF? Would it make more sense to follow the order? If you could upload a new patch that fixes the order, that would be great.

        Thanks!

        p.s. You don't have to clean old attached files. In fact, it's better to keep them since they show how this jira has developed and make it easier for others to understand history.

        Show
        Cheolsoo Park added a comment - Hi Joel, +1. Thanks you so much for your patience. The patch looks great! All tests pass too. I have a super minor comment on the doc. I think that the functions in the doc are listed in alphabetical order, but your SUBTRACT comes before IsEmpty. I guess that you added it there because SUBTRACT is similar to DIFF? Would it make more sense to follow the order? If you could upload a new patch that fixes the order, that would be great. Thanks! p.s. You don't have to clean old attached files. In fact, it's better to keep them since they show how this jira has developed and make it easier for others to understand history.
        Hide
        Joel Costigliola added a comment -

        Sorry for cleaning old attached files, I won't do that next time.

        I'll submit a new patch with doc functions in correct order.

        Show
        Joel Costigliola added a comment - Sorry for cleaning old attached files, I won't do that next time. I'll submit a new patch with doc functions in correct order.
        Hide
        Joel Costigliola added a comment -

        Hope it's the final patch

        Show
        Joel Costigliola added a comment - Hope it's the final patch
        Hide
        Cheolsoo Park added a comment -

        Looks good to me. I will wait today just to see if anyone has any concerns. If I don't hear anything, I will commit it.

        Thanks a lot!

        Show
        Cheolsoo Park added a comment - Looks good to me. I will wait today just to see if anyone has any concerns. If I don't hear anything, I will commit it. Thanks a lot!
        Hide
        Cheolsoo Park added a comment -

        Committed to trunk. Thanks Joel!

        Please someone add jocosti to the contributor list. I can't assign this jira to him.

        Show
        Cheolsoo Park added a comment - Committed to trunk. Thanks Joel! Please someone add jocosti to the contributor list. I can't assign this jira to him.
        Hide
        Joel Costigliola added a comment -

        Thanks Cheolsoo, with your help, we finally made it !

        Show
        Joel Costigliola added a comment - Thanks Cheolsoo, with your help, we finally made it !

          People

          • Assignee:
            Joel Costigliola
            Reporter:
            Joel Costigliola
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development