Pig
  1. Pig
  2. PIG-154

Move DEFINE and STORE parsing into QueryParser from GruntParser

    Details

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

      Description

      Currently GruntParser handles the parsing of DEFINE and STORE commands, it would be better to parse these in the main QueryParser since it has better infrastructure for expressions etc. which are really crucial for parsing UDFs etc.

      1. PIG-154_0_20080319.patch
        12 kB
        Arun C Murthy
      2. PIG-154_1_20080319.patch
        12 kB
        Arun C Murthy

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Patch Available Patch Available
        1d 8h 28m 1 Arun C Murthy 20/Mar/08 01:42
        Patch Available Patch Available Resolved Resolved
        2d 59m 1 Olga Natkovich 22/Mar/08 02:41
        Resolved Resolved Closed Closed
        732d 19h 20m 1 Alan Gates 24/Mar/10 22:01
        Alan Gates made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Hide
        Arun C Murthy added a comment -

        Sorry I don't quite understand what you tried to explain. Is having LODefine in the logical plan due to the fact that we shortcut our processing step?

        Pi, Alan is referring to the fact that the newly introduced LODefine is essentially a dummy LogicalOperator which is never used at all... it was necessary since QueryParser.parse() currently returns a LogicalPlan (which contains a LogicalOperator). Alan is pointing out that future work on the Pig parsers should incorporate DEFINE like operators. Hope that helps ...

        Show
        Arun C Murthy added a comment - Sorry I don't quite understand what you tried to explain. Is having LODefine in the logical plan due to the fact that we shortcut our processing step? Pi, Alan is referring to the fact that the newly introduced LODefine is essentially a dummy LogicalOperator which is never used at all... it was necessary since QueryParser.parse() currently returns a LogicalPlan (which contains a LogicalOperator). Alan is pointing out that future work on the Pig parsers should incorporate DEFINE like operators. Hope that helps ...
        Hide
        Pi Song added a comment -

        Alan,

        Sorry I don't quite understand what you tried to explain. Is having LODefine in the logical plan due to the fact that we shortcut our processing step?

        Normal implementation:-

        [Code] ==parser==> [AST] ==code_generator==> [Intermediate Instructions] ==native_code_generator==> [native_executable]
        

        Pig implementation:-

        Compile Time
        [Code] ==parser==>  [Intermediate Instructions(Logical Plan)] ==native_code_generator(Physical Plan Compiler)==> [native_executable]
        
        Show
        Pi Song added a comment - Alan, Sorry I don't quite understand what you tried to explain. Is having LODefine in the logical plan due to the fact that we shortcut our processing step? Normal implementation:- [Code] ==parser==> [AST] ==code_generator==> [Intermediate Instructions] ==native_code_generator==> [native_executable] Pig implementation:- Compile Time [Code] ==parser==> [Intermediate Instructions(Logical Plan)] ==native_code_generator(Physical Plan Compiler)==> [native_executable]
        Olga Natkovich made changes -
        Resolution Fixed [ 1 ]
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Hide
        Olga Natkovich added a comment -

        patch committed

        Show
        Olga Natkovich added a comment - patch committed
        Hide
        Alan Gates added a comment -

        I'm unsure about this thing of adding LODefine. I know that the way things are set up now, it's hard to get around. In general, in the future the pig parser will need to be able to handle lines that don't generate a real logical plan (define, register, set, etc.). For now this is ok, but we need to remember that this isn't the direction we want to go.

        Everything else looks good.

        Show
        Alan Gates added a comment - I'm unsure about this thing of adding LODefine. I know that the way things are set up now, it's hard to get around. In general, in the future the pig parser will need to be able to handle lines that don't generate a real logical plan (define, register, set, etc.). For now this is ok, but we need to remember that this isn't the direction we want to go. Everything else looks good.
        Arun C Murthy made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Arun C Murthy made changes -
        Attachment PIG-154_1_20080319.patch [ 12378287 ]
        Hide
        Arun C Murthy added a comment -

        Updated patch which fixed a minor regression in STORE parsing...

        STORE accepts 'PigStorage' and 'PigStorage()' for the UDF while LOAD accepts only 'PigStorage()' and hence the regression. Fixed since.

        Show
        Arun C Murthy added a comment - Updated patch which fixed a minor regression in STORE parsing... STORE accepts 'PigStorage' and 'PigStorage()' for the UDF while LOAD accepts only 'PigStorage()' and hence the regression. Fixed since.
        Arun C Murthy made changes -
        Attachment PIG-154_0_20080319.patch [ 12378273 ]
        Hide
        Arun C Murthy added a comment -

        Minor change to earlier patch ...

        Forgot to add this note previously: this patch uses current infrastructure for parsing UDFs (used in LOAD/GROUP) in QueryParser. This regresses on some of the additional parsing done in PigScriptParser... I've opened PIG-163 to fix this once and for all.

        Show
        Arun C Murthy added a comment - Minor change to earlier patch ... Forgot to add this note previously: this patch uses current infrastructure for parsing UDFs (used in LOAD/GROUP) in QueryParser. This regresses on some of the additional parsing done in PigScriptParser... I've opened PIG-163 to fix this once and for all.
        Arun C Murthy made changes -
        Attachment PIG-154_0_20080319.patch [ 12378268 ]
        Arun C Murthy made changes -
        Field Original Value New Value
        Attachment PIG-154_0_20080319.patch [ 12378268 ]
        Hide
        Arun C Murthy added a comment -

        Here is a patch for review while I continue doing some large-scale tests... I've moved parsing of DEFINE and STORE to QueryParser from GruntParser and ensured it passes all unit tests.

        Show
        Arun C Murthy added a comment - Here is a patch for review while I continue doing some large-scale tests... I've moved parsing of DEFINE and STORE to QueryParser from GruntParser and ensured it passes all unit tests.
        Arun C Murthy created issue -

          People

          • Assignee:
            Arun C Murthy
            Reporter:
            Arun C Murthy
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development