Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.1.0
    • Component/s: None
    • Labels:
      None

      Description

      Currently a large number of unit tests is failing on Windows. We need to fix that.

      1. cygpath.patch
        4 kB
        Daniel Dai
      2. PIG_243.patch
        50 kB
        Daniel Dai

        Issue Links

          Activity

          Hide
          olgan Olga Natkovich added a comment -

          Patch committed. Thanks Daniel for contributing.

          Show
          olgan Olga Natkovich added a comment - Patch committed. Thanks Daniel for contributing.
          Hide
          olgan Olga Natkovich added a comment -

          Thanks Daniel. Running tests now.

          Show
          olgan Olga Natkovich added a comment - Thanks Daniel. Running tests now.
          Hide
          daijy Daniel Dai added a comment -

          Fix the remaining 4 unit test error under cygwin

          Show
          daijy Daniel Dai added a comment - Fix the remaining 4 unit test error under cygwin
          Hide
          pi_song Pi Song added a comment -

          So far, we have ensured the data internalize/externalize bit. Is there anything else in the engine that can cause trouble on Windows?
          I couldn't think of any.

          Show
          pi_song Pi Song added a comment - So far, we have ensured the data internalize/externalize bit. Is there anything else in the engine that can cause trouble on Windows? I couldn't think of any.
          Hide
          olgan Olga Natkovich added a comment -

          Patch committed. Thanks Daniel for contributing!

          Show
          olgan Olga Natkovich added a comment - Patch committed. Thanks Daniel for contributing!
          Hide
          daijy Daniel Dai added a comment -

          You are right. We should change TextLoader as well. I have updated patch to include this. Thank you!

          Show
          daijy Daniel Dai added a comment - You are right. We should change TextLoader as well. I have updated patch to include this. Thank you!
          Hide
          pi_song Pi Song added a comment -

          +1

          BTW, there is another guy called TextLoader that needs '\r' removed as well.

          Show
          pi_song Pi Song added a comment - +1 BTW, there is another guy called TextLoader that needs '\r' removed as well.
          Hide
          daijy Daniel Dai added a comment -

          Thank you!

          Show
          daijy Daniel Dai added a comment - Thank you!
          Hide
          olgan Olga Natkovich added a comment -

          Yes, I think this is fine. I will test and commit the patch sometimes tomorrow or Monday.

          Show
          olgan Olga Natkovich added a comment - Yes, I think this is fine. I will test and commit the patch sometimes tomorrow or Monday.
          Hide
          daijy Daniel Dai added a comment - - edited

          I have tested reading one million records. Here is the result:

          • On Linux, where trimming not taking place:
            • Before patch: 10.53s
            • After patch: 10.60s
          • On Windows, where trimming branch taken:
            • Before patch: 13.65s
            • After patch: 13.69s

          Seems that the overhead is neglectable.

          Another solution is to change the testcase rather than core code to fix this test failure. However, I think move it to core is better. Failing to read a file in dos format (or need extra processing) under cygwin sounds not reasonable. How do you think?

          Show
          daijy Daniel Dai added a comment - - edited I have tested reading one million records. Here is the result: On Linux, where trimming not taking place: Before patch: 10.53s After patch: 10.60s On Windows, where trimming branch taken: Before patch: 13.65s After patch: 13.69s Seems that the overhead is neglectable. Another solution is to change the testcase rather than core code to fix this test failure. However, I think move it to core is better. Failing to read a file in dos format (or need extra processing) under cygwin sounds not reasonable. How do you think?
          Hide
          olgan Olga Natkovich added a comment -

          Thanks, Daniel. Changes look good. Once concern I have is with changes to PigStorage since it adds a little bit of overhead to each record processing. However, since I assume that String caches the length, the overhead should be minimal. Could you run a quick test that reads a million lines with and without the extra processing and see what the time difference is. Thanks.

          Show
          olgan Olga Natkovich added a comment - Thanks, Daniel. Changes look good. Once concern I have is with changes to PigStorage since it adds a little bit of overhead to each record processing. However, since I assume that String caches the length, the overhead should be minimal. Could you run a quick test that reads a million lines with and without the extra processing and see what the time difference is. Thanks.
          Hide
          daijy Daniel Dai added a comment - - edited

          Attached patch fix most all problems under cygwin. 4 errors remaining in TestStreaming for map-reduce testing under cygwin need further work. Patch is tested in both Linux and Cygwin. Here is a list of detected problem and solution:

          Problem Affected Testcase Solution
          Windows path delimit "\" interpreted as escape in java TestMapReduceResultRecycling, TestMapReduce, TestOrderBy, TestBuiltin, TestFilterOpNumeric, TestBinaryStorage, TestFilterOpString, TestEvalPipeline, TestPigFile, TestReversibleLoadStore, TestAlgebraicEval, TestLargeFile, TestCombiner, TestPi replace "\" with "\ \", see http://issues.apache.org/jira/browse/PIG-152
          Runtime.exec(String[]) fail for perl -e if perl statement include quote character for unknown reason TestParamSubPreproc, TestStreaming append a escape character \ to quote character " under Windows
          There is a tailing '\r' (0x0d) character for line read from text file under cygwin TestBuiltin, TestEvalPipeline, TestPigSplit, TestStreaming remove tailing '\r'
          Bug: forget to close output stream, following deletion operation fail TestBinaryStorage Fix bug
          Pig streaming put the current directory into PATH. Current directory is obtained using "System.getProperty("user.dir")", it is a Windows style path under cygwin. However, Cygwin only accept Unix style path in PATH environment variable. So put the Windows path obtained from "System.getProperty" directly into PATH will not work TestStreaming Change Windows path to Unix path under Cygwin. Using Cygwin utility "cygpath"

          Only two change are in core code. One is PigStorage.java (to fix tailing '\r'). The other is ExecutableManager (Add cygpath). All other changes are in testcase.

          Show
          daijy Daniel Dai added a comment - - edited Attached patch fix most all problems under cygwin. 4 errors remaining in TestStreaming for map-reduce testing under cygwin need further work. Patch is tested in both Linux and Cygwin. Here is a list of detected problem and solution: Problem Affected Testcase Solution Windows path delimit "\" interpreted as escape in java TestMapReduceResultRecycling, TestMapReduce, TestOrderBy, TestBuiltin, TestFilterOpNumeric, TestBinaryStorage, TestFilterOpString, TestEvalPipeline, TestPigFile, TestReversibleLoadStore, TestAlgebraicEval, TestLargeFile, TestCombiner, TestPi replace "\" with "\ \", see http://issues.apache.org/jira/browse/PIG-152 Runtime.exec(String[]) fail for perl -e if perl statement include quote character for unknown reason TestParamSubPreproc, TestStreaming append a escape character \ to quote character " under Windows There is a tailing '\r' (0x0d) character for line read from text file under cygwin TestBuiltin, TestEvalPipeline, TestPigSplit, TestStreaming remove tailing '\r' Bug: forget to close output stream, following deletion operation fail TestBinaryStorage Fix bug Pig streaming put the current directory into PATH. Current directory is obtained using "System.getProperty("user.dir")", it is a Windows style path under cygwin. However, Cygwin only accept Unix style path in PATH environment variable. So put the Windows path obtained from "System.getProperty" directly into PATH will not work TestStreaming Change Windows path to Unix path under Cygwin. Using Cygwin utility "cygpath" Only two change are in core code. One is PigStorage.java (to fix tailing '\r'). The other is ExecutableManager (Add cygpath). All other changes are in testcase.
          Hide
          pi_song Pi Song added a comment -

          Like Alan said, our developers don't use Windows so it's difficult to test. Is it possible to also run tests on windows as nightly build?

          Show
          pi_song Pi Song added a comment - Like Alan said, our developers don't use Windows so it's difficult to test. Is it possible to also run tests on windows as nightly build?

            People

            • Assignee:
              daijy Daniel Dai
              Reporter:
              olgan Olga Natkovich
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development