Pig
  1. Pig
  2. PIG-3211

Allow default Load/Store funcs to be configurable

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.12.0
    • Fix Version/s: 0.12.0
    • Component/s: None
    • Labels:
      None
    • Patch Info:
      Patch Available
    • Release Note:
      Hide
      The default storage is now configurable by the following new properties:
      - pig.default.load.func=<fully qualified class name of LoadFunc>
      - pig.default.store.func=<fully qualified class name of StoreFunc>
      Show
      The default storage is now configurable by the following new properties: - pig.default.load.func=<fully qualified class name of LoadFunc> - pig.default.store.func=<fully qualified class name of StoreFunc>

      Description

      PigStorage is used by default when a Load/StoreFunc is not specified. It would be useful to make this configurable.

      1. PIG-3211_2.patch
        11 kB
        Prashant Kommireddi
      2. PIG-3211.patch
        7 kB
        Prashant Kommireddi

        Activity

        Hide
        Prashant Kommireddi added a comment -

        Changes made to LogicalPlanBuilder, Utils and TestLogicalPlanBuilder. 1 thing I would appreciate feedback on was regarding the classloader to use which I am picking up from PigContext at the moment.

        2 new configs:

        pig.default.load.func - default class to load for LoadFunc
        pig.default.store.func - default class to load for StoreFunc

        I have added test cases for both load and store. Also, I noticed there's a lot of ugliness in TestLogicalPlanBuilder which I don't want to clean up in this patch but probably should be done at a later point.

        Show
        Prashant Kommireddi added a comment - Changes made to LogicalPlanBuilder, Utils and TestLogicalPlanBuilder. 1 thing I would appreciate feedback on was regarding the classloader to use which I am picking up from PigContext at the moment. 2 new configs: pig.default.load.func - default class to load for LoadFunc pig.default.store.func - default class to load for StoreFunc I have added test cases for both load and store. Also, I noticed there's a lot of ugliness in TestLogicalPlanBuilder which I don't want to clean up in this patch but probably should be done at a later point.
        Hide
        Prashant Kommireddi added a comment -

        Please note I have not added these props to pig.properties in this patch. I will incorporate that with any changes I might need to make with the review.

        Show
        Prashant Kommireddi added a comment - Please note I have not added these props to pig.properties in this patch. I will incorporate that with any changes I might need to make with the review.
        Hide
        Jonathan Coveney added a comment -

        I'll take a closer look in a little bit, though I will say that there is a PigConfiguration singleton I'd like to see catch on. Ideally, it should be the central place for configurations like this.

        Show
        Jonathan Coveney added a comment - I'll take a closer look in a little bit, though I will say that there is a PigConfiguration singleton I'd like to see catch on. Ideally, it should be the central place for configurations like this.
        Hide
        Cheolsoo Park added a comment -

        Prashant Kommireddi, I have two comments:

        1. I think we can simplify the code. Why don't we do this?
          buildLoadOp
          -            FuncSpec instantiatedFuncSpec =
          -                    funcSpec == null ?
          -                        new FuncSpec(PigStorage.class.getName()) :
          -                        funcSpec;
          -            loFunc = (LoadFunc)PigContext.instantiateFuncFromSpec(instantiatedFuncSpec);
          -            String fileNameKey = QueryParserUtils.constructFileNameSignature(filename, instantiatedFuncSpec) + "_" + (loadIndex++);
          +            String defaultLoadFuncName = pigContext.getProperties().getProperty("pig.default.load.func", PigStorage.class.getName());
          +            funcSpec = funcSpec == null ? new FuncSpec(defaultLoadFuncName): funcSpec;
          +            loFunc = (LoadFunc)PigContext.instantiateFuncFromSpec(funcSpec);
          +            String fileNameKey = QueryParserUtils.constructFileNameSignature(filename, funcSpec) + "_" + (loadIndex++);
          
          buildStoreOp
          -            FuncSpec instantiatedFuncSpec =
          -                    funcSpec == null ?
          -                            new FuncSpec(PigStorage.class.getName()):
          -                            funcSpec;
          -
          -            StoreFuncInterface stoFunc = (StoreFuncInterface)PigContext.instantiateFuncFromSpec(instantiatedFuncSpec);
          +            String defaultStoreFuncName = pigContext.getProperties().getProperty("pig.default.store.func", PigStorage.class.getName());
          +            funcSpec = funcSpec == null ? new FuncSpec(defaultStoreFuncName): funcSpec;
          +            StoreFuncInterface stoFunc = (StoreFuncInterface)PigContext.instantiateFuncFromSpec(funcSpec);
          

          I can confirm that your test cases pass with this, so I don't think we need the helper functions in Utils.java.

        2. In addition, we should change the following in QueryParserUtils.java:
          QueryParserUtils.java
               public static void attachStorePlan(String scope, LogicalPlan lp, String fileName, String func, 
                       Operator input, String alias, PigContext pigContext) throws FrontendException {
                   if( func == null ) {
          -            func = PigStorage.class.getName();
          +            func = pigContext.getProperties().getProperty("pig.default.store.func", PigStorage.class.getName());
                   }
          

        Let me know what you think.

        Show
        Cheolsoo Park added a comment - Prashant Kommireddi , I have two comments: I think we can simplify the code. Why don't we do this? buildLoadOp - FuncSpec instantiatedFuncSpec = - funcSpec == null ? - new FuncSpec(PigStorage.class.getName()) : - funcSpec; - loFunc = (LoadFunc)PigContext.instantiateFuncFromSpec(instantiatedFuncSpec); - String fileNameKey = QueryParserUtils.constructFileNameSignature(filename, instantiatedFuncSpec) + "_" + (loadIndex++); + String defaultLoadFuncName = pigContext.getProperties().getProperty( "pig. default .load.func" , PigStorage.class.getName()); + funcSpec = funcSpec == null ? new FuncSpec(defaultLoadFuncName): funcSpec; + loFunc = (LoadFunc)PigContext.instantiateFuncFromSpec(funcSpec); + String fileNameKey = QueryParserUtils.constructFileNameSignature(filename, funcSpec) + "_" + (loadIndex++); buildStoreOp - FuncSpec instantiatedFuncSpec = - funcSpec == null ? - new FuncSpec(PigStorage.class.getName()): - funcSpec; - - StoreFuncInterface stoFunc = (StoreFuncInterface)PigContext.instantiateFuncFromSpec(instantiatedFuncSpec); + String defaultStoreFuncName = pigContext.getProperties().getProperty( "pig. default .store.func" , PigStorage.class.getName()); + funcSpec = funcSpec == null ? new FuncSpec(defaultStoreFuncName): funcSpec; + StoreFuncInterface stoFunc = (StoreFuncInterface)PigContext.instantiateFuncFromSpec(funcSpec); I can confirm that your test cases pass with this, so I don't think we need the helper functions in Utils.java. In addition, we should change the following in QueryParserUtils.java: QueryParserUtils.java public static void attachStorePlan( String scope, LogicalPlan lp, String fileName, String func, Operator input, String alias, PigContext pigContext) throws FrontendException { if ( func == null ) { - func = PigStorage.class.getName(); + func = pigContext.getProperties().getProperty( "pig. default .store.func" , PigStorage.class.getName()); } Let me know what you think.
        Hide
        Prashant Kommireddi added a comment -

        Thanks Cheolsoo Park. I have updated the patch (may be simplified a bit further). I also took Jon's suggestion of using PigConfiguration to define the new properties. The same has been documented in pig.properties for users.

        Show
        Prashant Kommireddi added a comment - Thanks Cheolsoo Park . I have updated the patch (may be simplified a bit further). I also took Jon's suggestion of using PigConfiguration to define the new properties. The same has been documented in pig.properties for users.
        Hide
        Cheolsoo Park added a comment -

        +1. I will wait a day before committing just to see Jonathan has more suggestions. Thank you Prashant!

        Show
        Cheolsoo Park added a comment - +1. I will wait a day before committing just to see Jonathan has more suggestions. Thank you Prashant!
        Hide
        Jonathan Coveney added a comment -

        looks fine to me. go ahead and commit it cheolsoo

        Show
        Jonathan Coveney added a comment - looks fine to me. go ahead and commit it cheolsoo
        Hide
        Cheolsoo Park added a comment -

        OK, I will run unit test now.

        Show
        Cheolsoo Park added a comment - OK, I will run unit test now.
        Hide
        Cheolsoo Park added a comment -

        I committed to trunk. Thanks Prashant!

        Show
        Cheolsoo Park added a comment - I committed to trunk. Thanks Prashant!
        Hide
        Prashant Kommireddi added a comment -

        Thanks for the review/commit Cheolsoo.

        Show
        Prashant Kommireddi added a comment - Thanks for the review/commit Cheolsoo.

          People

          • Assignee:
            Prashant Kommireddi
            Reporter:
            Prashant Kommireddi
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development