Uploaded image for project: 'Pig'
  1. Pig
  2. PIG-3087

Refactor TestLogicalPlanBuilder to be meaningful

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.17.0
    • Component/s: None
    • Labels:
    • Hadoop Flags:
      Reviewed

      Description

      I started doing this as part of another patch, but there are some bigger issues, and I don't have the time to dig in atm.

      That said, a lot of the tests as written don't test anything. I used more modern junit patterns, and discovered we had a lot of tests that weren't functioning properly. Making them function properly unveiled that the general buildLp pattern doesn't work properly anymore for many cases where it would throw an error in grunt, but for whatever reason no error is thrown in the tests.

      Any test with _1 is a test that previous failed, that now doesn't. Some, however, don't make sense so I think what really needs to be done is figure out which should be failing, which shouldn't, and then fix buildLp accordingly.

      I will attach my pass at it, but it is incomplete and needs work.

      1. PIG-3087.4.patch
        63 kB
        Adam Szita
      2. PIG-3087.3.patch
        63 kB
        Adam Szita
      3. PIG-3087.2.patch
        63 kB
        Adam Szita
      4. PIG-3087.1.patch
        62 kB
        Adam Szita
      5. PIG-3087-0.patch
        59 kB
        Jonathan Coveney

        Activity

        Hide
        szita Adam Szita added a comment -

        Thanks for the review

        Show
        szita Adam Szita added a comment - Thanks for the review
        Hide
        daijy Daniel Dai added a comment -

        Patch committed to trunk. Thanks Adam, Jonathan!

        Show
        daijy Daniel Dai added a comment - Patch committed to trunk. Thanks Adam, Jonathan!
        Hide
        szita Adam Szita added a comment -

        Indeed that's not needed there - apparently we'll end up in the catch clause either way: with the validation_on we get a ParserException, with validation_off we get the same, just wrapped into a FrontendException

        Show
        szita Adam Szita added a comment - Indeed that's not needed there - apparently we'll end up in the catch clause either way: with the validation_on we get a ParserException, with validation_off we get the same, just wrapped into a FrontendException
        Hide
        daijy Daniel Dai added a comment -

        Another question, why setting "pigServer.setValidateEachStatement(false);" in testQueryFail67? Doesn't seem necessary.

        Show
        daijy Daniel Dai added a comment - Another question, why setting "pigServer.setValidateEachStatement(false);" in testQueryFail67? Doesn't seem necessary.
        Hide
        jcoveney Jonathan Coveney added a comment -

        So cool to see this get some love! Adam, you are a prince!

        Show
        jcoveney Jonathan Coveney added a comment - So cool to see this get some love! Adam, you are a prince!
        Hide
        szita Adam Szita added a comment -

        Ah I see your point. Patch revised

        Show
        szita Adam Szita added a comment - Ah I see your point. Patch revised
        Hide
        daijy Daniel Dai added a comment -

        I think the original test is intended to test the fail path. It's better to change the query to trigger the validation exception:
        A = load 'a';
        B = group A by (*, $0);

        Show
        daijy Daniel Dai added a comment - I think the original test is intended to test the fail path. It's better to change the query to trigger the validation exception: A = load 'a'; B = group A by (*, $0);
        Hide
        szita Adam Szita added a comment -

        Thanks for taking a look.
        1. Fixed - see PIG-3087.2.patch
        2. testQuery22_a was testQuery22Fail before, indicating that is should fail and exception should be caught. This is not the case with that Pig script (at least with the trunk version), plan is built successfully without throwing any errors. So all-in-all this one is not expected to fail anymore. (We just didn't see this because this test case always went through, just not entering in the catch clause)

        Show
        szita Adam Szita added a comment - Thanks for taking a look. 1. Fixed - see PIG-3087 .2.patch 2. testQuery22_a was testQuery22Fail before, indicating that is should fail and exception should be caught. This is not the case with that Pig script (at least with the trunk version), plan is built successfully without throwing any errors. So all-in-all this one is not expected to fail anymore. (We just didn't see this because this test case always went through, just not entering in the catch clause)
        Hide
        daijy Daniel Dai added a comment - - edited

        The bug fix in ForEachUserSchemaVisitor looks good. I have comments on the test fix:
        1. ExecType.LOCAL->Util.getLocalTestMode()
        2. Seems now you are validate plan for most test, why we miss some validation exception (eg, testQuery22_a)

        Show
        daijy Daniel Dai added a comment - - edited The bug fix in ForEachUserSchemaVisitor looks good. I have comments on the test fix: 1. ExecType.LOCAL->Util.getLocalTestMode() 2. Seems now you are validate plan for most test, why we miss some validation exception (eg, testQuery22_a)
        Hide
        szita Adam Szita added a comment -

        Taken a look on this. It seems that quite some mysterious (failing in grunt if inputted manually, but passing in unit test mode) test cases are resulted from query validation being off.
        In the patch I attached (PIG-3087.1.patch) I have this enabled and also went through and fixed several test cases so that they pass (or fail with expected exception) properly.
        I also found a bug in ForEachUserSchemaVisitor during the process: it seems that it can't handle use cases where casting is in the latest (foreach) step of the plan - so I fixed this as well: testQuery90a tests this.

        Daniel Dai can you please take a look when you get a chance?

        Show
        szita Adam Szita added a comment - Taken a look on this. It seems that quite some mysterious (failing in grunt if inputted manually, but passing in unit test mode) test cases are resulted from query validation being off. In the patch I attached ( PIG-3087 .1.patch) I have this enabled and also went through and fixed several test cases so that they pass (or fail with expected exception) properly. I also found a bug in ForEachUserSchemaVisitor during the process: it seems that it can't handle use cases where casting is in the latest (foreach) step of the plan - so I fixed this as well: testQuery90a tests this. Daniel Dai can you please take a look when you get a chance?
        Hide
        jcoveney Jonathan Coveney added a comment -

        Please do!

        Show
        jcoveney Jonathan Coveney added a comment - Please do!
        Hide
        haogao Hao Gao added a comment -

        Can I try this?

        Show
        haogao Hao Gao added a comment - Can I try this?

          People

          • Assignee:
            szita Adam Szita
            Reporter:
            jcoveney Jonathan Coveney
          • Votes:
            1 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development