Pig
  1. Pig
  2. PIG-438

Handle realiasing of existing Alias (A=B;)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.2.0
    • Fix Version/s: 0.10.0, 0.11
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      We do not handle re-aliasing of an existing alias - this should be handled correctly.

      The following script should work:

      
      a = load 'studenttab10k';
      b = filter a by $1 > '25';
      c = b;
      
      -- use b
      d = cogroup b by $0, a by $0;
      e = foreach d generate flatten(b), flatten(a);
      dump e
      
      -- use c
      f = cogroup c by $0, a by $0;
      g = foreach f generate flatten(c), flatten(a);
      dump g;
      
      
      1. PIG-438.patch
        0.8 kB
        Jonathan Coveney
      2. PIG-438-2.patch
        7 kB
        Daniel Dai

        Activity

        Hide
        Jonathan Coveney added a comment -

        I actually have a patch that does this pretty trivially. It just takes B = A; and converts it to B = FOREACH A GENERATE *; The only issue is that this affects some error messages and whatnot, and I wasn't able to fix that. Attaching.

        Show
        Jonathan Coveney added a comment - I actually have a patch that does this pretty trivially. It just takes B = A; and converts it to B = FOREACH A GENERATE *; The only issue is that this affects some error messages and whatnot, and I wasn't able to fix that. Attaching.
        Hide
        Jonathan Coveney added a comment -

        It works to realias, but some tests fail and I'm not sure how to best fix them.

        Show
        Jonathan Coveney added a comment - It works to realias, but some tests fail and I'm not sure how to best fix them.
        Hide
        Russell Jurney added a comment -

        This is pretty cool. This would address some of my issues outlined in PIG-2511

        Show
        Russell Jurney added a comment - This is pretty cool. This would address some of my issues outlined in PIG-2511
        Hide
        Jonathan Coveney added a comment -

        For clarification, the tests that fail have nothing to do with correctness, but rather that some error messages get changed by the changes in the parser.

        Show
        Jonathan Coveney added a comment - For clarification, the tests that fail have nothing to do with correctness, but rather that some error messages get changed by the changes in the parser.
        Hide
        Jonathan Coveney added a comment -

        The only test that fails is:

        testMissingSemicolon in TestLogicalPlanBuilder

        The issue is that now that new_relation = old_relation; is valid, if you leave off the semicolon, it sees the error there, instead of elsewhere. Is it ok to change this accordingly, or should I try and finagle it so that the error will be the same? (no clue how I'd do that)

        I can add some tests as well, but that's the blocker for me.

        Show
        Jonathan Coveney added a comment - The only test that fails is: testMissingSemicolon in TestLogicalPlanBuilder The issue is that now that new_relation = old_relation; is valid, if you leave off the semicolon, it sees the error there, instead of elsewhere. Is it ok to change this accordingly, or should I try and finagle it so that the error will be the same? (no clue how I'd do that) I can add some tests as well, but that's the blocker for me.
        Hide
        Dmitriy V. Ryaboy added a comment -

        Jonathan, what's the error that is now being thrown in testMissingSemicolon? If it's a sensible message , we can change the test to that. If it's something that totally obscures the actual missing semicolon error, we should consider how to make errors for this kind of case more meaningful.

        Show
        Dmitriy V. Ryaboy added a comment - Jonathan, what's the error that is now being thrown in testMissingSemicolon? If it's a sensible message , we can change the test to that. If it's something that totally obscures the actual missing semicolon error, we should consider how to make errors for this kind of case more meaningful.
        Hide
        Jonathan Coveney added a comment -

        Ah, I thought I had included that.

        The old:

        mismatched input 'B' expecting SEMI_COLON
        

        The new:

        ERROR org.apache.pig.tools.grunt.Grunt - ERROR 1200: <line 1, column 4>  Syntax error, unexpected symbol at or near 'load'
        
        Show
        Jonathan Coveney added a comment - Ah, I thought I had included that. The old: mismatched input 'B' expecting SEMI_COLON The new: ERROR org.apache.pig.tools.grunt.Grunt - ERROR 1200: <line 1, column 4> Syntax error, unexpected symbol at or near 'load'
        Hide
        Daniel Dai added a comment -

        Attach a different fix.

        Show
        Daniel Dai added a comment - Attach a different fix.
        Hide
        Jonathan Coveney added a comment -

        Daniel: in general, is it a better practice to add operators instead of expressing them in terms of other operators?

        Show
        Jonathan Coveney added a comment - Daniel: in general, is it a better practice to add operators instead of expressing them in terms of other operators?
        Hide
        Daniel Dai added a comment -

        Not in general, but in this case, adding a foreach seems to be unnecessary, we only need to update the alias map.

        Show
        Daniel Dai added a comment - Not in general, but in this case, adding a foreach seems to be unnecessary, we only need to update the alias map.
        Hide
        Thejas M Nair added a comment -

        +1

        Show
        Thejas M Nair added a comment - +1
        Hide
        Daniel Dai added a comment -

        test-patch:
        [exec] -1 overall.
        [exec]
        [exec] +1 @author. The patch does not contain any @author tags.
        [exec]
        [exec] +1 tests included. The patch appears to include 9 new or modified tests.
        [exec]
        [exec] -1 javadoc. The javadoc tool appears to have generated 1 warning messages.
        [exec]
        [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
        [exec]
        [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
        [exec]
        [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.

        javadoc warning is not related.

        Patch committed to 0.10/trunk.

        Show
        Daniel Dai added a comment - test-patch: [exec] -1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 9 new or modified tests. [exec] [exec] -1 javadoc. The javadoc tool appears to have generated 1 warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. javadoc warning is not related. Patch committed to 0.10/trunk.

          People

          • Assignee:
            Jonathan Coveney
            Reporter:
            Pradeep Kamath
          • Votes:
            1 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development