Hive
  1. Hive
  2. HIVE-1546

Ability to plug custom Semantic Analyzers for Hive Grammar

    Details

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

      Description

      It will be useful if Semantic Analysis phase is made pluggable such that other projects can do custom analysis of hive queries before doing metastore operations on them.

      1. hooks2.patch
        31 kB
        Ashutosh Chauhan
      2. hooks.patch
        31 kB
        Ashutosh Chauhan
      3. Howl_Semantic_Analysis.txt
        13 kB
        Ashutosh Chauhan
      4. hive-1546-4.patch
        24 kB
        Ashutosh Chauhan
      5. hive-1546-3.patch
        24 kB
        Ashutosh Chauhan
      6. hive-1546_2.patch
        28 kB
        Ashutosh Chauhan
      7. hive-1546.patch
        18 kB
        Ashutosh Chauhan

        Activity

        Hide
        Ashutosh Chauhan added a comment -

        This will involve a bit of refactoring and adding interface points which other systems can latch on.

        Show
        Ashutosh Chauhan added a comment - This will involve a bit of refactoring and adding interface points which other systems can latch on.
        Hide
        John Sichi added a comment -

        (Note for other Hive committers: this JIRA originated with some back-channel discussions between me and Pradeep about how to make it possible for Howl to reuse Hive CLI functionality.)

        Show
        John Sichi added a comment - (Note for other Hive committers: this JIRA originated with some back-channel discussions between me and Pradeep about how to make it possible for Howl to reuse Hive CLI functionality.)
        Hide
        Ashutosh Chauhan added a comment -

        Attached patch adds the capability to Hive so that custom semantic analysis of query is possible before it is handed over to Hive. Plus there are few other miscellaneous refactoring around it. Changes include:

        • Addition of SemanticAnalyzerFactoryInterface. If conf has a particular variable specified, a custom analyzer will be loaded and used, otherwise existing Hive Semantic Analyzer will be used. So, default behavior is preserved.
        • Changed visibility of few methods in DDLSemanticAnalyzer and SemanticAnalyzer from private to protected as I wanted to override them in my custom analyzer.
        • Changed file format specification in grammar, so that it can optionally take two more parameters (InputDriver and OutputDriver) in addition to InputFormat and OutputFormat. These are optional, so preserves the default behavior.
        • In file format specification, currently SequenceFile, TextFile and RCFile are supported through keyword. Expanded that production so to accept an identifier so that its possible to provide support for more file formats without needing to change Hive grammar every time. Currently, that token results in exception since there are none, but when we add support for other file formats that could be changed. This preserves current behavior.

        Note that there are no new test cases since its mostly code restructuring and doesnt add/modify current behavior, thus passing existing tests should suffice.
        I should point out most of these changes are driven by Howl and would like to thank John for suggesting the initial approach for these changes.

        Show
        Ashutosh Chauhan added a comment - Attached patch adds the capability to Hive so that custom semantic analysis of query is possible before it is handed over to Hive. Plus there are few other miscellaneous refactoring around it. Changes include: Addition of SemanticAnalyzerFactoryInterface. If conf has a particular variable specified, a custom analyzer will be loaded and used, otherwise existing Hive Semantic Analyzer will be used. So, default behavior is preserved. Changed visibility of few methods in DDLSemanticAnalyzer and SemanticAnalyzer from private to protected as I wanted to override them in my custom analyzer. Changed file format specification in grammar, so that it can optionally take two more parameters (InputDriver and OutputDriver) in addition to InputFormat and OutputFormat. These are optional, so preserves the default behavior. In file format specification, currently SequenceFile, TextFile and RCFile are supported through keyword. Expanded that production so to accept an identifier so that its possible to provide support for more file formats without needing to change Hive grammar every time. Currently, that token results in exception since there are none, but when we add support for other file formats that could be changed. This preserves current behavior. Note that there are no new test cases since its mostly code restructuring and doesnt add/modify current behavior, thus passing existing tests should suffice. I should point out most of these changes are driven by Howl and would like to thank John for suggesting the initial approach for these changes.
        Hide
        Ashutosh Chauhan added a comment -

        Btw, can someone assign this jira to me and add me to the list of contributors so that in future I can do that myself.

        Show
        Ashutosh Chauhan added a comment - Btw, can someone assign this jira to me and add me to the list of contributors so that in future I can do that myself.
        Hide
        John Sichi added a comment -

        Thanks Ashutosh, I've reassigned this one to you and will get back to you with some review comments.

        Show
        John Sichi added a comment - Thanks Ashutosh, I've reassigned this one to you and will get back to you with some review comments.
        Hide
        John Sichi added a comment -

        I have published some review comments here:

        http://review.cloudera.org/r/713/

        In addition:

        • We need a test for loading a variation on the default semantic analyzer in order to exercise the pluggable configuration. You can create a subclass of the default analyzer (under ql/src/test/org/apache/hadoop/hive/ql/parse) to inject some mock behavior change.
        • We need a test for the new INPUTDRIVER/OUTPUTDRIVER clauses. Otherwise, people will wonder why they're in the grammar but not referenced elsewhere in Hive; the test will give you a chance to explain the Howl dependency.
        • We need a negative test for unrecognized generic format.

        Let me know if you need help with how to script new positive+negative test cases.

        (BTW, your patch file header had a glitch in it; we usually use svn diff format.)

        Show
        John Sichi added a comment - I have published some review comments here: http://review.cloudera.org/r/713/ In addition: We need a test for loading a variation on the default semantic analyzer in order to exercise the pluggable configuration. You can create a subclass of the default analyzer (under ql/src/test/org/apache/hadoop/hive/ql/parse) to inject some mock behavior change. We need a test for the new INPUTDRIVER/OUTPUTDRIVER clauses. Otherwise, people will wonder why they're in the grammar but not referenced elsewhere in Hive; the test will give you a chance to explain the Howl dependency. We need a negative test for unrecognized generic format. Let me know if you need help with how to script new positive+negative test cases. (BTW, your patch file header had a glitch in it; we usually use svn diff format.)
        Hide
        HBase Review Board added a comment -

        Message from: "John Sichi" <jsichi@facebook.com>

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        http://review.cloudera.org/r/713/
        -----------------------------------------------------------

        Review request for Hive Developers.

        Summary
        -------

        review by JVS

        This addresses bug HIVE-1546.
        http://issues.apache.org/jira/browse/HIVE-1546

        Diffs


        http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java 988318
        http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java 988318
        http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java 988318
        http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 988318
        http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ExplainSemanticAnalyzer.java 988318
        http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 988318
        http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 988318
        http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactory.java 988318
        http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactoryInterface.java PRE-CREATION

        Diff: http://review.cloudera.org/r/713/diff

        Testing
        -------

        Thanks,

        John

        Show
        HBase Review Board added a comment - Message from: "John Sichi" <jsichi@facebook.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/713/ ----------------------------------------------------------- Review request for Hive Developers. Summary ------- review by JVS This addresses bug HIVE-1546 . http://issues.apache.org/jira/browse/HIVE-1546 Diffs http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java 988318 http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java 988318 http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java 988318 http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 988318 http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ExplainSemanticAnalyzer.java 988318 http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 988318 http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 988318 http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactory.java 988318 http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactoryInterface.java PRE-CREATION Diff: http://review.cloudera.org/r/713/diff Testing ------- Thanks, John
        Hide
        HBase Review Board added a comment -

        Message from: "John Sichi" <jsichi@facebook.com>

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        http://review.cloudera.org/r/713/#review1008
        -----------------------------------------------------------

        http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java
        <http://review.cloudera.org/r/713/#comment3232>

        Can you break this out into several statements for readability?

        http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java
        <http://review.cloudera.org/r/713/#comment3233>

        This does not belong in HiveUtils. Can't we just leave it where it is (in SemanticAnalyzerFactory) and add a public accessor method for it so that extensions can reuse it?

        http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java
        <http://review.cloudera.org/r/713/#comment3234>

        Why does this return a String []? The callers don't use it.

        http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactoryInterface.java
        <http://review.cloudera.org/r/713/#comment3231>

        Putting "Interface" into the interface name is clunky.

        Let's call the interface HiveSemanticAnalyzerFactory instead (matching the convention for other plugin interfaces such as HiveStorageHandler and HiveIndexHandler).

        In addition, I think we should introduce a new interface HiveSemanticAnalyzer and have the factory produce that, since factories should generally produce interfaces rather than classes (e.g. so that dynamic proxies can be used).

        http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactoryInterface.java
        <http://review.cloudera.org/r/713/#comment3224>

        This configuration parameter needs to be defined in HiveConf.java following the pattern already in use there.

        Also, it needs a new entry in conf/hive-default.xml

        http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactoryInterface.java
        <http://review.cloudera.org/r/713/#comment3228>

        Our convention for existing Hive plugins is to make them extend org.apache.hadoop.conf.Configurable and pass in the configuration only once via setConf (rather than to individual methods such as get).

        Inside a Configurable, you can get back to a HiveConf with:

        new HiveConf(getConf(), YourClass.class)

        • John
        Show
        HBase Review Board added a comment - Message from: "John Sichi" <jsichi@facebook.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/713/#review1008 ----------------------------------------------------------- http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java < http://review.cloudera.org/r/713/#comment3232 > Can you break this out into several statements for readability? http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java < http://review.cloudera.org/r/713/#comment3233 > This does not belong in HiveUtils. Can't we just leave it where it is (in SemanticAnalyzerFactory) and add a public accessor method for it so that extensions can reuse it? http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java < http://review.cloudera.org/r/713/#comment3234 > Why does this return a String []? The callers don't use it. http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactoryInterface.java < http://review.cloudera.org/r/713/#comment3231 > Putting "Interface" into the interface name is clunky. Let's call the interface HiveSemanticAnalyzerFactory instead (matching the convention for other plugin interfaces such as HiveStorageHandler and HiveIndexHandler). In addition, I think we should introduce a new interface HiveSemanticAnalyzer and have the factory produce that, since factories should generally produce interfaces rather than classes (e.g. so that dynamic proxies can be used). http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactoryInterface.java < http://review.cloudera.org/r/713/#comment3224 > This configuration parameter needs to be defined in HiveConf.java following the pattern already in use there. Also, it needs a new entry in conf/hive-default.xml http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactoryInterface.java < http://review.cloudera.org/r/713/#comment3228 > Our convention for existing Hive plugins is to make them extend org.apache.hadoop.conf.Configurable and pass in the configuration only once via setConf (rather than to individual methods such as get). Inside a Configurable, you can get back to a HiveConf with: new HiveConf(getConf(), YourClass.class) John
        Hide
        Ashutosh Chauhan added a comment -

        Attaching a new patch.

        In addition, I think we should introduce a new interface HiveSemanticAnalyzer and have the factory produce that, since factories should generally produce interfaces rather than classes (e.g. so that dynamic proxies can be used).

        This got me thinking and I have redesigned the patch, so now we have factory producing factory enabling dynamic proxying (abstractions and extensibility !) Your other comments got affected by it because of it but I have taken care of them.

        Signature for method handleGenricFileFormat() was String[] because description for task and task itself is created in analyzeCreateTable() and analyzeTableFileFormat() so that method cant be void, as it needs to return whatever information it gathered to its calling method. Since at present we have no such file format, return value is ignored by calling methods. However I changed it to Map<String,String> because that seems more useful then String[].

        Added test-cases for all three cases you suggested.

        Show
        Ashutosh Chauhan added a comment - Attaching a new patch. In addition, I think we should introduce a new interface HiveSemanticAnalyzer and have the factory produce that, since factories should generally produce interfaces rather than classes (e.g. so that dynamic proxies can be used). This got me thinking and I have redesigned the patch, so now we have factory producing factory enabling dynamic proxying (abstractions and extensibility !) Your other comments got affected by it because of it but I have taken care of them. Signature for method handleGenricFileFormat() was String[] because description for task and task itself is created in analyzeCreateTable() and analyzeTableFileFormat() so that method cant be void, as it needs to return whatever information it gathered to its calling method. Since at present we have no such file format, return value is ignored by calling methods. However I changed it to Map<String,String> because that seems more useful then String[]. Added test-cases for all three cases you suggested.
        Hide
        John Sichi added a comment -

        Thanks, I took a look. But factory producing factory seems like overkill here.

        What I was thinking in my previous comment was as follows:

        • only one level of factory
        • define new interface HiveSemanticAnalyzer (not an abstract class): copy signatures of interface methods from public methods on BaseSemanticAnalyzer; add Javadoc (I can help with that if any of them are non-obvious)
        • when callers get new analyzer instance from factory, they refer to it via the HiveSemanticAnalyzer interface only

        If that doesn't work for some reason, then we can just use your original pattern where the factory returns a class (BaseSemanticAnalyzer) instead of an interface.

        Regarding handleGenericFileFormat: I still don't understand. Your code in Hive is ignoring the return value (and the only implementation doesn't return anything, it just throws). So, either

        (a) you're planning to do something inside of Howl with it; in that case, the Hive method is just a hook for you to intercept, and it should return void

        or

        (b) you're planning to add some more code inside of Hive which actually does something with the return value (e.g. sets the serde+inputformat+outputformat); in that case, you need to keep working on the patch to make this happen

        Show
        John Sichi added a comment - Thanks, I took a look. But factory producing factory seems like overkill here. What I was thinking in my previous comment was as follows: only one level of factory define new interface HiveSemanticAnalyzer (not an abstract class): copy signatures of interface methods from public methods on BaseSemanticAnalyzer; add Javadoc (I can help with that if any of them are non-obvious) when callers get new analyzer instance from factory, they refer to it via the HiveSemanticAnalyzer interface only If that doesn't work for some reason, then we can just use your original pattern where the factory returns a class (BaseSemanticAnalyzer) instead of an interface. Regarding handleGenericFileFormat: I still don't understand. Your code in Hive is ignoring the return value (and the only implementation doesn't return anything, it just throws). So, either (a) you're planning to do something inside of Howl with it; in that case, the Hive method is just a hook for you to intercept, and it should return void or (b) you're planning to add some more code inside of Hive which actually does something with the return value (e.g. sets the serde+inputformat+outputformat); in that case, you need to keep working on the patch to make this happen
        Hide
        Ashutosh Chauhan added a comment -

        What I was thinking in my previous comment was as follows:

        • only one level of factory
        • define new interface HiveSemanticAnalyzer (not an abstract class): copy signatures of interface methods from public methods on BaseSemanticAnalyzer; add Javadoc (I can help with that if any of them are non-obvious)
        • when callers get new analyzer instance from factory, they refer to it via the HiveSemanticAnalyzer interface only

        Are you envisioning something like this in Driver.java

        HiveSemanticAnalyzer sem = HiveSemanticAnalyzerFactory.get(conf);
        
        // Do semantic analysis and plan generation
        sem.analyze(tree, ctx);
        
        // validate the plan
        sem.validate();
        
        plan = new QueryPlan(command, sem);
        
        // get the output schema
        schema = getSchema(sem, conf);
        

        Note where

        {sem}

        is used. If so, HiveSemanticAnalyzer interface needs to look like this:

        public interface HiveSemanticAnalyzer extends Configurable{
        
          public void analyze(ASTNode ast, Context ctx) throws SemanticException;
        
          public void validate() throws SemanticException;
        
          public List<FieldSchema> getResultSchema();
        
          public FetchTask getFetchTask();
        
          public List<Task<? extends Serializable>> getRootTasks();
        
          public HashSet<ReadEntity> getInputs();
        
          public HashSet<WriteEntity> getOutputs();
        
          public LineageInfo getLineageInfo();
        
          public HashMap<String, String> getIdToTableNameMap();
        }
        
        

        This to me look like an awkward interface, which has to expose internal details of the very class it is trying to hide.
        But even if we make an effort to improve on it and try to come up with a better interface, this will mean that I need to touch upon lot of critical code paths in BaseSemanticAnalyzer, SemanticAnalyzer and DDLSemanticAnalyzer which I am not sure I know enough of them to make changes. If this is not what you were envisioning then I have missed something in what you were suggesting.

        Assuming we dont go this route, I liked the approach in my second patch better then from my first patch. I think it provides better abstractions. But if you have different opinion, I am fine with the first one as well.

        Regarding handleGenericFileFormat, its the point (a). It cant be void, because in Howl where we override this it needs to return information to its caller which will work on that information. handleGenericFileFormat() can only make use of the token and provide back some information. Caller works on that information and modifies the task it has created which usually happens to be of type DDLWork. And even Hive will need to go through this same code flow if and when it decides to add more FileFormats.

        Show
        Ashutosh Chauhan added a comment - What I was thinking in my previous comment was as follows: only one level of factory define new interface HiveSemanticAnalyzer (not an abstract class): copy signatures of interface methods from public methods on BaseSemanticAnalyzer; add Javadoc (I can help with that if any of them are non-obvious) when callers get new analyzer instance from factory, they refer to it via the HiveSemanticAnalyzer interface only Are you envisioning something like this in Driver.java HiveSemanticAnalyzer sem = HiveSemanticAnalyzerFactory.get(conf); // Do semantic analysis and plan generation sem.analyze(tree, ctx); // validate the plan sem.validate(); plan = new QueryPlan(command, sem); // get the output schema schema = getSchema(sem, conf); Note where {sem} is used. If so, HiveSemanticAnalyzer interface needs to look like this: public interface HiveSemanticAnalyzer extends Configurable{ public void analyze(ASTNode ast, Context ctx) throws SemanticException; public void validate() throws SemanticException; public List<FieldSchema> getResultSchema(); public FetchTask getFetchTask(); public List<Task<? extends Serializable>> getRootTasks(); public HashSet<ReadEntity> getInputs(); public HashSet<WriteEntity> getOutputs(); public LineageInfo getLineageInfo(); public HashMap< String , String > getIdToTableNameMap(); } This to me look like an awkward interface, which has to expose internal details of the very class it is trying to hide. But even if we make an effort to improve on it and try to come up with a better interface, this will mean that I need to touch upon lot of critical code paths in BaseSemanticAnalyzer, SemanticAnalyzer and DDLSemanticAnalyzer which I am not sure I know enough of them to make changes. If this is not what you were envisioning then I have missed something in what you were suggesting. Assuming we dont go this route, I liked the approach in my second patch better then from my first patch. I think it provides better abstractions. But if you have different opinion, I am fine with the first one as well. Regarding handleGenericFileFormat, its the point (a). It cant be void, because in Howl where we override this it needs to return information to its caller which will work on that information. handleGenericFileFormat() can only make use of the token and provide back some information. Caller works on that information and modifies the task it has created which usually happens to be of type DDLWork. And even Hive will need to go through this same code flow if and when it decides to add more FileFormats.
        Hide
        John Sichi added a comment -

        Yes, that is what I was envisioning. I think the interface as you've specified it looks close to the abstract functionality of the semantic analyzer, which is what we want (although we should use interfaces such as Set in preference to concrete classes such as HashSet, something which is currently crufty throughout Hive).

        I agree that this could be too involved for your first patch, and we would probably need to evolve the HiveSemanticAnalyzer interface anyway. So, if you're not comfortable going there, let's scale it back to the approach in the first patch (with HiveSemanticAnalyzerFactory returning BaseSemanticAnalyzer).

        handleGenericFileFormat: if it's only a hook for Howl, then you can have a separate method inside of Howl which returns whatever you want, but wrap it with a void method which overrides a void one in Hive (and discards the return values). Or, if the idea is to have Hive use this too, then go ahead and add Javadoc specifying exactly what the return map is supposed to contain, and then convert the existing SEQUENCEFILE/TEXTFILE/RCFILE cases so they go through the generic path. (But still keep them as reserved words rather than literal strings for backwards compatibility.)

        Show
        John Sichi added a comment - Yes, that is what I was envisioning. I think the interface as you've specified it looks close to the abstract functionality of the semantic analyzer, which is what we want (although we should use interfaces such as Set in preference to concrete classes such as HashSet, something which is currently crufty throughout Hive). I agree that this could be too involved for your first patch, and we would probably need to evolve the HiveSemanticAnalyzer interface anyway. So, if you're not comfortable going there, let's scale it back to the approach in the first patch (with HiveSemanticAnalyzerFactory returning BaseSemanticAnalyzer). handleGenericFileFormat: if it's only a hook for Howl, then you can have a separate method inside of Howl which returns whatever you want, but wrap it with a void method which overrides a void one in Hive (and discards the return values). Or, if the idea is to have Hive use this too, then go ahead and add Javadoc specifying exactly what the return map is supposed to contain, and then convert the existing SEQUENCEFILE/TEXTFILE/RCFILE cases so they go through the generic path. (But still keep them as reserved words rather than literal strings for backwards compatibility.)
        Hide
        John Sichi added a comment -

        More specifically, for the last suggestion, where we currently have

              case HiveParser.TOK_TBLRCFILE:
                inputFormat = RCFILE_INPUT;
                outputFormat = RCFILE_OUTPUT;
                shared.serde = COLUMNAR_SERDE;
                storageFormat = true;
        

        instead do

              case HiveParser.TOK_TBLRCFILE:
                  processGenericFileFormat("RCFILE");
                  break;
              case HiveParser.TOK_FILEFORMAT_GENERIC:
                  processGenericFileFormat(child.getChild(0).getText());
                  break;
        
        ...
        
        void processGenericFileFormat(String formatName) {
            Map<String, String> props = handleGenericFileFormat(formatName);
            inputFormat = props.get(Constants.FILE_INPUT_FORMAT);
            outputFormat = props.get(Constants.FILE_OUTPUT_FORMAT);
            shared.serde = props.get(Constants.META_TABLE_SERDE);
           ...
        }
        

        Then in Hive's version of handleGenericFileFormat, make it return the right info for SEQUENCEFILE/TEXTFILE/RCFILE.

        This is just a sketch, not the real code, but I hope it makes sense.

        Show
        John Sichi added a comment - More specifically, for the last suggestion, where we currently have case HiveParser.TOK_TBLRCFILE: inputFormat = RCFILE_INPUT; outputFormat = RCFILE_OUTPUT; shared.serde = COLUMNAR_SERDE; storageFormat = true; instead do case HiveParser.TOK_TBLRCFILE: processGenericFileFormat("RCFILE"); break; case HiveParser.TOK_FILEFORMAT_GENERIC: processGenericFileFormat(child.getChild(0).getText()); break; ... void processGenericFileFormat(String formatName) { Map<String, String> props = handleGenericFileFormat(formatName); inputFormat = props.get(Constants.FILE_INPUT_FORMAT); outputFormat = props.get(Constants.FILE_OUTPUT_FORMAT); shared.serde = props.get(Constants.META_TABLE_SERDE); ... } Then in Hive's version of handleGenericFileFormat, make it return the right info for SEQUENCEFILE/TEXTFILE/RCFILE. This is just a sketch, not the real code, but I hope it makes sense.
        Hide
        Ashutosh Chauhan added a comment -

        Thanks for quick feedback, John.
        New patch with the approach in first patch incorporating review comments that followed it. For method signature, I took the first suggestion you had and made it void and figured out a way how to deal with it in Howl. Test cases added.

        Show
        Ashutosh Chauhan added a comment - Thanks for quick feedback, John. New patch with the approach in first patch incorporating review comments that followed it. For method signature, I took the first suggestion you had and made it void and figured out a way how to deal with it in Howl. Test cases added.
        Hide
        Ashutosh Chauhan added a comment -

        Missed changes in one of the file. Reattaching the patch.

        Show
        Ashutosh Chauhan added a comment - Missed changes in one of the file. Reattaching the patch.
        Hide
        HBase Review Board added a comment -

        Message from: "Carl Steinbach" <carl@cloudera.com>

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        http://review.cloudera.org/r/713/
        -----------------------------------------------------------

        (Updated 2010-08-30 15:42:15.768310)

        Review request for Hive Developers.

        Changes
        -------

        Updated diff with https://issues.apache.org/jira/secure/attachment/12453466/hive-1546-3.patch

        Summary
        -------

        review by JVS

        This addresses bug HIVE-1546.
        http://issues.apache.org/jira/browse/HIVE-1546

        Diffs (updated)


        trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 990934
        trunk/conf/hive-default.xml 990934
        trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java 990934
        trunk/ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java 990934
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java 990934
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 990934
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ExplainSemanticAnalyzer.java 990934
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 990934
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/HiveSemanticAnalyzerFactory.java PRE-CREATION
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 990934
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactory.java 990934
        trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/DummySemanticAnalyzerFactory.java PRE-CREATION
        trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestSemanticAnalyzerLoading.java PRE-CREATION
        trunk/ql/src/test/queries/clientnegative/genericFileFormat.q PRE-CREATION
        trunk/ql/src/test/queries/clientpositive/inoutdriver.q PRE-CREATION
        trunk/ql/src/test/results/clientnegative/genericFileFormat.q.out PRE-CREATION
        trunk/ql/src/test/results/clientpositive/inoutdriver.q.out PRE-CREATION

        Diff: http://review.cloudera.org/r/713/diff

        Testing
        -------

        Thanks,

        John

        Show
        HBase Review Board added a comment - Message from: "Carl Steinbach" <carl@cloudera.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/713/ ----------------------------------------------------------- (Updated 2010-08-30 15:42:15.768310) Review request for Hive Developers. Changes ------- Updated diff with https://issues.apache.org/jira/secure/attachment/12453466/hive-1546-3.patch Summary ------- review by JVS This addresses bug HIVE-1546 . http://issues.apache.org/jira/browse/HIVE-1546 Diffs (updated) trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 990934 trunk/conf/hive-default.xml 990934 trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java 990934 trunk/ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java 990934 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java 990934 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 990934 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ExplainSemanticAnalyzer.java 990934 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 990934 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/HiveSemanticAnalyzerFactory.java PRE-CREATION trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 990934 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactory.java 990934 trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/DummySemanticAnalyzerFactory.java PRE-CREATION trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestSemanticAnalyzerLoading.java PRE-CREATION trunk/ql/src/test/queries/clientnegative/genericFileFormat.q PRE-CREATION trunk/ql/src/test/queries/clientpositive/inoutdriver.q PRE-CREATION trunk/ql/src/test/results/clientnegative/genericFileFormat.q.out PRE-CREATION trunk/ql/src/test/results/clientpositive/inoutdriver.q.out PRE-CREATION Diff: http://review.cloudera.org/r/713/diff Testing ------- Thanks, John
        Hide
        John Sichi added a comment -

        +1. Will commit when tests pass.

        Show
        John Sichi added a comment - +1. Will commit when tests pass.
        Hide
        Ashutosh Chauhan added a comment -

        After some recent changes, It seems its necessary to do

        conf.set(HiveConf.ConfVars.HIVE_SUPPORT_CONCURRENCY.varname, "false");
        

        in the test-case, otherwise test-case just hangs. Added it in my test-case. No other changes in the patch (as compared to previous patch) otherwise.

        Show
        Ashutosh Chauhan added a comment - After some recent changes, It seems its necessary to do conf.set(HiveConf.ConfVars.HIVE_SUPPORT_CONCURRENCY.varname, " false " ); in the test-case, otherwise test-case just hangs. Added it in my test-case. No other changes in the patch (as compared to previous patch) otherwise.
        Hide
        Namit Jain added a comment -

        We need to debug this.
        For tests, we should be able to turn concurrency on.

        Which test is hanging ?

        Show
        Namit Jain added a comment - We need to debug this. For tests, we should be able to turn concurrency on. Which test is hanging ?
        Hide
        Ashutosh Chauhan added a comment -

        It is the new test that I wrote for this patch TestSemanticAnalyzerLoading.java If I dont have this line my test hangs but if I have it, testcase completes successfully. How should I go about debugging it? Do I need to bring up zookeeper externally or something?

        Show
        Ashutosh Chauhan added a comment - It is the new test that I wrote for this patch TestSemanticAnalyzerLoading.java If I dont have this line my test hangs but if I have it, testcase completes successfully. How should I go about debugging it? Do I need to bring up zookeeper externally or something?
        Hide
        Carl Steinbach added a comment -

        @Ashutosh: Can you provide some background on what you hope to accomplish with this? What is the motivating use case, i.e. what custom SemanticAnalyzer do you plan to write?

        Also, how are the new INPUTDRIVER and OUTPUTDRIVER properties used? By adding these to the Hive grammar it seems like we may be providing a mechanism for defining tables in the MetaStore that Hive can't read or write to. If that's the case what are your plans for adding this support to Hive?

        Show
        Carl Steinbach added a comment - @Ashutosh: Can you provide some background on what you hope to accomplish with this? What is the motivating use case, i.e. what custom SemanticAnalyzer do you plan to write? Also, how are the new INPUTDRIVER and OUTPUTDRIVER properties used? By adding these to the Hive grammar it seems like we may be providing a mechanism for defining tables in the MetaStore that Hive can't read or write to. If that's the case what are your plans for adding this support to Hive?
        Hide
        Carl Steinbach added a comment -

        If the main motivation for this ticket is the ability to produce a crippled version of the HiveCLI that is only capable of executing DDL, then I think we should consider simpler approaches that don't involve making SemanticAnalyzer a public API.

        SemanticAnalyzer is in serious need of refactoring. Making this API public will severely restrict our ability to do this work in the future.

        Show
        Carl Steinbach added a comment - If the main motivation for this ticket is the ability to produce a crippled version of the HiveCLI that is only capable of executing DDL, then I think we should consider simpler approaches that don't involve making SemanticAnalyzer a public API. SemanticAnalyzer is in serious need of refactoring. Making this API public will severely restrict our ability to do this work in the future.
        Hide
        John Sichi added a comment -

        Hi Carl,

        As mentioned above, this patch originated with some of the early discussions on Howl before we had mailing lists set up.

        Show
        John Sichi added a comment - Hi Carl, As mentioned above, this patch originated with some of the early discussions on Howl before we had mailing lists set up.
        Hide
        Carl Steinbach added a comment -

        @ John: Good to know, but what's the motivation for this change? Was is it covered in the back-channel discussions you mentioned above? And is making SemanticAnalyzer a public API really a good idea?

        Show
        Carl Steinbach added a comment - @ John: Good to know, but what's the motivation for this change? Was is it covered in the back-channel discussions you mentioned above? And is making SemanticAnalyzer a public API really a good idea?
        Hide
        Ashutosh Chauhan added a comment -

        @Carl,

        • Ya, the main motivating use case is to provide an alternate DDL CLI tool (hopefully not crippled smiles). Reason for that is to enforce certain use-cases on DDL commands in Howl CLI. More details on that are here: http://wiki.apache.org/pig/Howl/HowlCliFuncSpec If you have questions why we are making such decisions in Howl, I will encourage you to post it on howl-dev list and we can discuss it there. howldev@yahoogroups.com
        • I dont understand what do you mean by making "SemanticAnalyzer a public API". This patch is just letting other tools to do some semantic analysis of the query and then use Hive to do further processing (if tool chooses to do so). Important point here is other tools. This in no way enforcing any changes to any Hive behavior. Hive can continue to have its own semantic analyzer and do any sort of semantic analysis of the query. Hive is making no guarantees to any tool.
        • Hive doesnt care about INPUTDRIVER and OUTPUTDRIVER and neither this patch is asking it to. I dont see any way that its providing any mechanism for defining tables in MetaStore that Hive cant read or write to.

        @John,
        Do you want to make me to any further changes or are we good to go?

        Show
        Ashutosh Chauhan added a comment - @Carl, Ya, the main motivating use case is to provide an alternate DDL CLI tool (hopefully not crippled smiles ). Reason for that is to enforce certain use-cases on DDL commands in Howl CLI. More details on that are here: http://wiki.apache.org/pig/Howl/HowlCliFuncSpec If you have questions why we are making such decisions in Howl, I will encourage you to post it on howl-dev list and we can discuss it there. howldev@yahoogroups.com I dont understand what do you mean by making "SemanticAnalyzer a public API". This patch is just letting other tools to do some semantic analysis of the query and then use Hive to do further processing (if tool chooses to do so). Important point here is other tools . This in no way enforcing any changes to any Hive behavior. Hive can continue to have its own semantic analyzer and do any sort of semantic analysis of the query. Hive is making no guarantees to any tool. Hive doesnt care about INPUTDRIVER and OUTPUTDRIVER and neither this patch is asking it to. I dont see any way that its providing any mechanism for defining tables in MetaStore that Hive cant read or write to. @John, Do you want to make me to any further changes or are we good to go?
        Hide
        John Sichi added a comment -

        @Carl: I understand your concern, but this seemed like the least intrusive approach as opposed to continually patching Hive to refine what Howl's CLI wants to support at a given point in time (which really has nothing to do with Hive). The override approach allows that behavior to be factored completely out into Howl. A number of our existing extensibility interfaces (e.g. StorageHandler) already have similar issues regarding impact from continual refactoring, so I expect an across-the-board SPI stabilization effort to be required in the future (with corresponding migrations from old to new). This will need to be part of that effort.

        @Ashutosh: I hit the hang you mentioned, so I can retry tests with your latest patch. But let's resolve the approach with Carl first. In particular, can we get agreement from the Howl team that even though we're introducing this dependency now, we will not let its existence hinder future semantic analyzer refactoring within Hive? As long as we all stay in frequent communication, we can make that work.

        @Both: one possible refinement would be to limit the public interface to just validation (as opposed to full semantic analysis). In that case, we would have HiveStmtValidatorFactory producing HiveStmtValidator with just a single method validate(). This would also remove the unpleasantness of having a factory returning a base class rather than an interface. However, if CLI is going to need to do more than just validation, then this isn't good enough.

        Show
        John Sichi added a comment - @Carl: I understand your concern, but this seemed like the least intrusive approach as opposed to continually patching Hive to refine what Howl's CLI wants to support at a given point in time (which really has nothing to do with Hive). The override approach allows that behavior to be factored completely out into Howl. A number of our existing extensibility interfaces (e.g. StorageHandler) already have similar issues regarding impact from continual refactoring, so I expect an across-the-board SPI stabilization effort to be required in the future (with corresponding migrations from old to new). This will need to be part of that effort. @Ashutosh: I hit the hang you mentioned, so I can retry tests with your latest patch. But let's resolve the approach with Carl first. In particular, can we get agreement from the Howl team that even though we're introducing this dependency now, we will not let its existence hinder future semantic analyzer refactoring within Hive? As long as we all stay in frequent communication, we can make that work. @Both: one possible refinement would be to limit the public interface to just validation (as opposed to full semantic analysis). In that case, we would have HiveStmtValidatorFactory producing HiveStmtValidator with just a single method validate(). This would also remove the unpleasantness of having a factory returning a base class rather than an interface. However, if CLI is going to need to do more than just validation, then this isn't good enough.
        Hide
        John Sichi added a comment -

        For the last sentence, I meant "If Howl's CLI customized behavior is going to need to influence more than just validation"

        Show
        John Sichi added a comment - For the last sentence, I meant "If Howl's CLI customized behavior is going to need to influence more than just validation"
        Hide
        Carl Steinbach added a comment -

        Can we get agreement from the Howl team that even though we're introducing this dependency now, we will not let its existence hinder future semantic analyzer refactoring within Hive?

        What about other projects that use this feature? How do we get them to agree to this, or how do we prevent them from using it? The new configuration property is documented in hive-default.xml, which implies that it's open to everyone.

        one possible refinement would be to limit the public interface to just validation (as opposed to full semantic analysis). In that case, we would have HiveStmtValidatorFactory producing HiveStmtValidator with just a single method validate().

        This reduces the scope of the dependency, but doesn't eliminate it. Plugins would presumably depend on the structure of the AST that they are trying to validate, which in turn would limit our ability to refactor the grammar or to replace ANTLR with another parser generator.

        Show
        Carl Steinbach added a comment - Can we get agreement from the Howl team that even though we're introducing this dependency now, we will not let its existence hinder future semantic analyzer refactoring within Hive? What about other projects that use this feature? How do we get them to agree to this, or how do we prevent them from using it? The new configuration property is documented in hive-default.xml, which implies that it's open to everyone. one possible refinement would be to limit the public interface to just validation (as opposed to full semantic analysis). In that case, we would have HiveStmtValidatorFactory producing HiveStmtValidator with just a single method validate(). This reduces the scope of the dependency, but doesn't eliminate it. Plugins would presumably depend on the structure of the AST that they are trying to validate, which in turn would limit our ability to refactor the grammar or to replace ANTLR with another parser generator.
        Hide
        John Sichi added a comment -

        New dependencies: we don't prevent anyone from using it, but we can Javadoc it as unstable. We can work out the language now in an updated patch since there's currently no Javadoc on the factory interface.

        Dependencies on AST/ANTLR: it does make such changes more expensive in terms of impact analysis and migration, but it doesn't really prevent us in any way, does it?

        Given that we've agreed at the high level on the approach of creating Howl as a wrapper around Hive (reusing as much as possible of what's already there), can you suggest an alternative mechanism that addresses the requirements while minimizing the injection of Howl behavior directly into Hive itself? If it were something generic like a bitmask of allowed operations, I could kind of see it, but the validation logic is more involved than that (and may become even more so over time). I wasn't able to come up with anything clean on that front myself, which is why I suggested the factoring approach to Pradeep originally. Apologies for not getting stuff aired out sooner.

        Show
        John Sichi added a comment - New dependencies: we don't prevent anyone from using it, but we can Javadoc it as unstable. We can work out the language now in an updated patch since there's currently no Javadoc on the factory interface. Dependencies on AST/ANTLR: it does make such changes more expensive in terms of impact analysis and migration, but it doesn't really prevent us in any way, does it? Given that we've agreed at the high level on the approach of creating Howl as a wrapper around Hive (reusing as much as possible of what's already there), can you suggest an alternative mechanism that addresses the requirements while minimizing the injection of Howl behavior directly into Hive itself? If it were something generic like a bitmask of allowed operations, I could kind of see it, but the validation logic is more involved than that (and may become even more so over time). I wasn't able to come up with anything clean on that front myself, which is why I suggested the factoring approach to Pradeep originally. Apologies for not getting stuff aired out sooner.
        Hide
        Carl Steinbach added a comment -

        we've agreed at the high level on the approach of creating Howl as a wrapper around Hive

        I thought Howl was supposed to be a wrapper around (and replacement for) the Hive metastore, not all of Hive.

        I think there are clear advantages to Hive and Howl sharing the same metastore code as long as they access this facility through the public API, but can't say the same for the two projects using the same CLI code if it means allowing external projects to depend on loosely defined set of internal APIs. What benefits are we hoping to achieve by having Howl and Hive share the same CLI code, especially if Howl is only interested in a small part of it? What are the drawbacks of instead encouraging the Howl project to copy the CLI code and maintain their own version?

        Show
        Carl Steinbach added a comment - we've agreed at the high level on the approach of creating Howl as a wrapper around Hive I thought Howl was supposed to be a wrapper around (and replacement for) the Hive metastore, not all of Hive. I think there are clear advantages to Hive and Howl sharing the same metastore code as long as they access this facility through the public API, but can't say the same for the two projects using the same CLI code if it means allowing external projects to depend on loosely defined set of internal APIs. What benefits are we hoping to achieve by having Howl and Hive share the same CLI code, especially if Howl is only interested in a small part of it? What are the drawbacks of instead encouraging the Howl project to copy the CLI code and maintain their own version?
        Hide
        John Sichi added a comment -

        It's the usual tradeoffs on copy-and-paste vs factoring. There's a significant amount of DDL processing code which can be shared, and that will continue to grow as we add new features (e.g. GRANT/REVOKE) which are applicable to both.

        Show
        John Sichi added a comment - It's the usual tradeoffs on copy-and-paste vs factoring. There's a significant amount of DDL processing code which can be shared, and that will continue to grow as we add new features (e.g. GRANT/REVOKE) which are applicable to both.
        Hide
        Carl Steinbach added a comment -

        What do you think of this option: we check the Howl SemanticAnalyzer into the Hive source tree and provide a config option that optionally enables it? This gives Howl the features they need without making the SemanticAnalyzer API public.

        Show
        Carl Steinbach added a comment - What do you think of this option: we check the Howl SemanticAnalyzer into the Hive source tree and provide a config option that optionally enables it? This gives Howl the features they need without making the SemanticAnalyzer API public.
        Hide
        John Sichi added a comment -

        That's fine with me if it doesn't drag in unrelated dependencies. I would vote for contrib, with the plugin mechanism remaining the same as Ashutosh has defined it, but with the config parameter explicitly defining it as intended for internal use only for now.

        Ashutosh, could you run this proposal by the Howl team and see if that is acceptable?

        Show
        John Sichi added a comment - That's fine with me if it doesn't drag in unrelated dependencies. I would vote for contrib, with the plugin mechanism remaining the same as Ashutosh has defined it, but with the config parameter explicitly defining it as intended for internal use only for now. Ashutosh, could you run this proposal by the Howl team and see if that is acceptable?
        Hide
        Carl Steinbach added a comment -

        I'm +1 on the approach outlined by John.

        Show
        Carl Steinbach added a comment - I'm +1 on the approach outlined by John.
        Hide
        Alan Gates added a comment -

        Using the definitions given in HADOOP-5073, can we call this interface limited private and evolving? We (the Howl team) know it will continue to change, and we understand Hive's desire not to make this a public API. But checking Howl code into Hive just muddles things and makes our build and release process harder.

        Show
        Alan Gates added a comment - Using the definitions given in HADOOP-5073 , can we call this interface limited private and evolving? We (the Howl team) know it will continue to change, and we understand Hive's desire not to make this a public API. But checking Howl code into Hive just muddles things and makes our build and release process harder.
        Hide
        Namit Jain added a comment -

        Would it be possible to do it via a hook ?

        Do you want to allow a subset of operations ? The hook is not very advanced right now, and you cannot change the query plan etc.
        But, it might be good enough for disallowing a class of statements. We can add more parameters to the hook if need be.

        That way, the change will be completely outside hive, and we will be able to use the existing client, but with a limited functionality.

        Show
        Namit Jain added a comment - Would it be possible to do it via a hook ? Do you want to allow a subset of operations ? The hook is not very advanced right now, and you cannot change the query plan etc. But, it might be good enough for disallowing a class of statements. We can add more parameters to the hook if need be. That way, the change will be completely outside hive, and we will be able to use the existing client, but with a limited functionality.
        Hide
        Carl Steinbach added a comment -

        I gather from Ashutosh's latest patch that you want to do the following:

        • Provide your own implementation of HiveSemanticAnalyzerFactory.
        • Subclass SemanticAnalyzer
        • Subclass DDLSemanticAnalzyer

        I looked at the public and protected members in these classes and think
        that at a minimum we would have to mark the following classes as limited
        private and evolving:

        • HiveSemanticAnalyzerFactory
        • BaseSemanticAnalyzer
        • SemanticAnalyzer
        • DDLSemanticAnalyzer
        • ASTNode
        • HiveParser (i.e. Hive's ANTLR grammar)
        • SemanticAnalyzer Context (org.apache.hadoop.hive.ql.Context)
        • Task and FetchTask
        • QB
        • QBParseInfo
        • QBMetaData
        • QBJoinTree
        • CreateTableDesc

        So anytime we touch one of these classes we would need to coordinate with the Howl folks to make sure we aren't breaking one of their plugins? I don't think this is a good tradeoff if the main benefit we can expect is a simpler build and release process for Howl.

        Show
        Carl Steinbach added a comment - I gather from Ashutosh's latest patch that you want to do the following: Provide your own implementation of HiveSemanticAnalyzerFactory. Subclass SemanticAnalyzer Subclass DDLSemanticAnalzyer I looked at the public and protected members in these classes and think that at a minimum we would have to mark the following classes as limited private and evolving: HiveSemanticAnalyzerFactory BaseSemanticAnalyzer SemanticAnalyzer DDLSemanticAnalyzer ASTNode HiveParser (i.e. Hive's ANTLR grammar) SemanticAnalyzer Context (org.apache.hadoop.hive.ql.Context) Task and FetchTask QB QBParseInfo QBMetaData QBJoinTree CreateTableDesc So anytime we touch one of these classes we would need to coordinate with the Howl folks to make sure we aren't breaking one of their plugins? I don't think this is a good tradeoff if the main benefit we can expect is a simpler build and release process for Howl.
        Hide
        Namit Jain added a comment -

        Sorry on jumping on this late.

        I quickly reviewed http://wiki.apache.org/pig/Howl/HowlCliFuncSpec, and it seems like most of the functionality is already
        present in hive. So, we need a way to restrict other types of statements - is that a fair statement ?

        If there is a slight change needed in hive (for some howl behavior), we can add it to hive ?
        Why do we need a brand new client ?

        Show
        Namit Jain added a comment - Sorry on jumping on this late. I quickly reviewed http://wiki.apache.org/pig/Howl/HowlCliFuncSpec , and it seems like most of the functionality is already present in hive. So, we need a way to restrict other types of statements - is that a fair statement ? If there is a slight change needed in hive (for some howl behavior), we can add it to hive ? Why do we need a brand new client ?
        Hide
        John Sichi added a comment -

        @Namit: the option of putting Howl directly into Hive was previously proposed but dropped for the same reasons Alan mentioned above. Regarding hooks, could you point me to the hook you're referring to? I don't believe Pre/Post have enough information currently, do they?

        @Carl: I don't think Howl cares about the query processing stuff like

        {Task and FetchTask,QB,QBParseInfo,QBMetaData,QBJoinTree}

        . For the others, it's not any time we touch them; it's only when we make breaking changes. And since Howl is also open source, it's not like these are opaque dependencies. We would need to do the same impact analysis if we used the contrib approach, right? I don't see a big difference between the two except with contrib we get the convenience of immediate compilation errors to tell us something broke. A continuous integration setup for Howl would take us close to that.

        @All: maybe we should set up a f2f meeting to hash this out?

        Show
        John Sichi added a comment - @Namit: the option of putting Howl directly into Hive was previously proposed but dropped for the same reasons Alan mentioned above. Regarding hooks, could you point me to the hook you're referring to? I don't believe Pre/Post have enough information currently, do they? @Carl: I don't think Howl cares about the query processing stuff like {Task and FetchTask,QB,QBParseInfo,QBMetaData,QBJoinTree} . For the others, it's not any time we touch them; it's only when we make breaking changes. And since Howl is also open source, it's not like these are opaque dependencies. We would need to do the same impact analysis if we used the contrib approach, right? I don't see a big difference between the two except with contrib we get the convenience of immediate compilation errors to tell us something broke. A continuous integration setup for Howl would take us close to that. @All: maybe we should set up a f2f meeting to hash this out?
        Hide
        Namit Jain added a comment -

        I agree it would be good to have a meeting.

        btw, the hooks I was referrring to and Pre/Post Execution Statement level hooks, with the following signature:

        public interface PostExecute

        { /** * The run command that is called just before the execution of the query. * * @param sess * The session state. * @param inputs * The set of input tables and partitions. * @param outputs * The set of output tables, partitions, local and hdfs directories. * @param lInfo * The column level lineage information. * @param ugi * The user group security information. */ void run(SessionState sess, Set<ReadEntity> inputs, Set<WriteEntity> outputs, LineageInfo lInfo, UserGroupInformation ugi) throws Exception; }

        Looking at the spec., it looks like a subset of DDLs need to be supported, which can be easily accomplished via the hook.
        If need be, we can pass more info. in the hook - there was a plan to add job level hook also, which is not checked in,
        but via which the configuration etc. can be changed.

        Show
        Namit Jain added a comment - I agree it would be good to have a meeting. btw, the hooks I was referrring to and Pre/Post Execution Statement level hooks, with the following signature: public interface PostExecute { /** * The run command that is called just before the execution of the query. * * @param sess * The session state. * @param inputs * The set of input tables and partitions. * @param outputs * The set of output tables, partitions, local and hdfs directories. * @param lInfo * The column level lineage information. * @param ugi * The user group security information. */ void run(SessionState sess, Set<ReadEntity> inputs, Set<WriteEntity> outputs, LineageInfo lInfo, UserGroupInformation ugi) throws Exception; } Looking at the spec., it looks like a subset of DDLs need to be supported, which can be easily accomplished via the hook. If need be, we can pass more info. in the hook - there was a plan to add job level hook also, which is not checked in, but via which the configuration etc. can be changed.
        Hide
        Ashutosh Chauhan added a comment -

        @Namit,
        As far as I understand, Hooks were designed to run before or after the query executes, not while query is getting executing. By executing a query I mean (generating AST, doing semantic analysis of that AST which includes necessary metadata operations, building a query plan and running the plan). If that was how hook were intended to be used I am not clear how they will be used in this scenario. In Howl its not just a list of metadata operations that we want to allow or disallow, there is a bit more semantic analysis that we do which includes doing metadata operation on metadata repository. Its not really before or after query is executed, it is one of the parts of query processing itself. You also hinted that hooks can be modified and more information can be provided in those methods. If thats what we do then I think we are loosing semantics on hooks which is something which is fired before and after the query executes, as hook will now be running while query is executing. Its entirely possible that I have misunderstood hooks and their intended usage. You obviously know more then so feel free to correct me.
        I am also attaching what semantic analysis that we are doing for everyone's reference for better context.

        Show
        Ashutosh Chauhan added a comment - @Namit, As far as I understand, Hooks were designed to run before or after the query executes, not while query is getting executing. By executing a query I mean (generating AST, doing semantic analysis of that AST which includes necessary metadata operations, building a query plan and running the plan). If that was how hook were intended to be used I am not clear how they will be used in this scenario. In Howl its not just a list of metadata operations that we want to allow or disallow, there is a bit more semantic analysis that we do which includes doing metadata operation on metadata repository. Its not really before or after query is executed, it is one of the parts of query processing itself. You also hinted that hooks can be modified and more information can be provided in those methods. If thats what we do then I think we are loosing semantics on hooks which is something which is fired before and after the query executes, as hook will now be running while query is executing. Its entirely possible that I have misunderstood hooks and their intended usage. You obviously know more then so feel free to correct me. I am also attaching what semantic analysis that we are doing for everyone's reference for better context.
        Hide
        Ashutosh Chauhan added a comment -

        @Namit,

        Continuing from discussion yesterday w.r.t new interface/hooks that you were suggesting. As far as I understood you have two primary concerns:

        • You do not want to have config variable enabled SemanticAnalyzer factory producing different Analyzers. Rather, you want to have only one HiveSemanticAnalyzer.
        • You also dont want others to extend SemanticAnalyzer, DDLSemanticAnalyzer and other Hive classes.

        As a solution to this, there will be new hooks introduced. These pre and post hooks will be called by Hive at different times during Semantic Analysis phase. To me, it seems there are few potential problems:

        • Hive will need to call these hooks from each and every method of its Analyzer which opens up whole lot more integration points to external tools (like Howl) and burden on those Analyzers then the current approach of factory enabled instantiation and extending of those classes.
        • Either there will be a one generic hook or many different hooks. If there is only one generic hook, then passed in information also need to be generic and as a result it will be a root of AST Node. If so, in any particular hook I am using I need to traverse the whole AST to get the node I am interested in.
        • If the hooks are more specific, then more specific information can be passed in which is more useful, but then you will have long list of such hooks.
        • Even if these concerns are taken care of, it seems that all of Howl's requirements may not get satisfied (like getting hold of DDLTask and modifying it, doing metadata operations etc.) I can elaborate on them further once I can see what you are proposing.

        Clearly from my description, you can see that I didnt get what you were envisioning and how is it better then current design. I must be missing something. Would you like to propose what you were suggesting? The attached patch and Howl classes on this jira should be sufficient to spec out Howl's requirements, if you need them.

        Show
        Ashutosh Chauhan added a comment - @Namit, Continuing from discussion yesterday w.r.t new interface/hooks that you were suggesting. As far as I understood you have two primary concerns: You do not want to have config variable enabled SemanticAnalyzer factory producing different Analyzers. Rather, you want to have only one HiveSemanticAnalyzer. You also dont want others to extend SemanticAnalyzer, DDLSemanticAnalyzer and other Hive classes. As a solution to this, there will be new hooks introduced. These pre and post hooks will be called by Hive at different times during Semantic Analysis phase. To me, it seems there are few potential problems: Hive will need to call these hooks from each and every method of its Analyzer which opens up whole lot more integration points to external tools (like Howl) and burden on those Analyzers then the current approach of factory enabled instantiation and extending of those classes. Either there will be a one generic hook or many different hooks. If there is only one generic hook, then passed in information also need to be generic and as a result it will be a root of AST Node. If so, in any particular hook I am using I need to traverse the whole AST to get the node I am interested in. If the hooks are more specific, then more specific information can be passed in which is more useful, but then you will have long list of such hooks. Even if these concerns are taken care of, it seems that all of Howl's requirements may not get satisfied (like getting hold of DDLTask and modifying it, doing metadata operations etc.) I can elaborate on them further once I can see what you are proposing. Clearly from my description, you can see that I didnt get what you were envisioning and how is it better then current design. I must be missing something. Would you like to propose what you were suggesting? The attached patch and Howl classes on this jira should be sufficient to spec out Howl's requirements, if you need them.
        Hide
        John Sichi added a comment -

        @Ashutosh: I will get back to you soon with a proposal. Based on what I see in your Howl_Semantic_Analysis.txt code, I think the interface can be very small.

        Show
        John Sichi added a comment - @Ashutosh: I will get back to you soon with a proposal. Based on what I see in your Howl_Semantic_Analysis.txt code, I think the interface can be very small.
        Hide
        John Sichi added a comment -

        I think we can start off with this interface:

        /**
         * HiveSemanticAnalyzerHook allows Hive to be extended with custom
         * logic for semantic analysis of QL statements.  This interface
         * and any Hive internals it exposes are currently 
         * "limited private and evolving" (unless otherwise stated elsewhere)
         * and intended mainly for use by the Howl project.
         *
         *<p>
         *
         * Note that the lifetime of an instantiated hook object is scoped to
         * the analysis of a single statement; hook instances are never reused.
         */
        public interface HiveSemanticAnalyzerHook {
          /**
           * Invoked before Hive performs its own semantic analysis on
           * a statement.  The implementation may inspect the statement AST and
           * prevent its execution by throwing a SemanticException.
           * Optionally, it may also augment/rewrite the AST, but must produce
           * a form equivalent to one which could have
           * been returned directly from Hive's own parser.
           *
           * @param context context information for semantic analysis
           *
           * @param ast AST being analyzed and optionally rewritten
           *
           * @return replacement AST (typically the same as the original AST unless the
           * entire tree had to be replaced; must not be null)
           */
          public ASTNode preAnalyze(
            HiveSemanticAnalyzerHookContext context,
            ASTNode ast) throws SemanticException;
        
          /**
           * Invoked after Hive performs its own semantic analysis on a
           * statement (including optimization).  
           * Hive calls postAnalyze on the same hook object
           * as preAnalyze, so the hook can maintain state across the calls.
           *
           * @param context context information for semantic analysis
           *
           * @param rootTasks root tasks produced by semantic analysis;
           * the hook is free to modify this list or its contents
           */
          public void postAnalyze(
            HiveSemanticAnalyzerHookContext context,
            List<Task<? extends Serializable>> rootTasks) throws SemanticException;
        }
        

        Plus companion context interface to be passed in from Hive:

        /**
         * Context information provided by Hive to implementations of HiveSemanticAnalyzerHook.
         */
        public interface HiveSemanticAnalyzerHookContext {
          /**
           * @return the Hive db instance; hook implementations can use this for purposes such as getting configuration information or making metastore calls
           */
          public Hive getHive();
        }
        

        The reason for the context is so that later, if we need to make more information
        available to the hook, we just add new getters to the context object
        (rather than adding new parameters to methods such as preAnalyze,
        which would break existing hook implementations).

        Unlike pre/post exec hooks, we will only allow one hook to be
        configured (rather than a list). If someone really wants to run
        more than one, they can write a list hook implementation which delegates
        to multiple. The conf variable used to load the hook class will be
        hive.semantic.analyzer.hook since it's no longer a factory.

        We also need an insulation class:

        public abstract class AbstractSemanticAnalyzerHook 
          implements HiveSemanticAnalyzerHook {
        
          public ASTNode preAnalyze(
            HiveSemanticAnalyzerHookContext context,
            ASTNode ast) throws SemanticException
          {
            return ast;
          }
        
          public void postAnalyze(
            HiveSemanticAnalyzerHookContext context,
            List<Task<? extends Serializable>> rootTasks) throws SemanticException
          {
          }
        }
        

        Hook implementations should extend this to avoid breaking when we add
        new methods to the HiveSemanticAnalyzerHook interface later.

        By using the hook approach, we limit the dependency exposure: only ASTNodes,
        Tasks, and the org.apache.hadoop.hive.ql.metadata.Hive class can be accessed
        for now. If we decide to open it up more later, that will be via an
        agreed-upon decision in a new patch (rather than via ad hoc dependency
        creep).

        Instead of invoking the hook from many different places inside of
        SemanticAnalyzer, we'll start with just invoking it only pre- and post-
        the call to sem.analyze. More invocation points
        (e.g. pre-optimization) can be added later on an as-needed basis.

        Howl's table property additions can be saved during the preAnalyze
        call and then applied to the Task during the postAnalyze call (since
        the hook is allowed to maintain state); this stays very close to the
        approach in the current patch. Another way to do this is to
        not use a postAnalyze at all, and instead just rewrite the AST to
        splice in the additional TBLPROPERTIES up front. Both ways are a
        little messy, but either way should be fine.

        For handleGenericFileFormat, I think we can deal with it by having
        Howl edit the AST to delete the STORED AS clause entirely during
        preAnalyze (in cases where it accepts the storage format). That way
        Hive can continue to reject it unconditionally, and we can avoid
        adding a third interface method.

        (Likewise, Howl should delete INPUTDRIVER and OUTPUTDRIVER, and Hive
        should reject them when it sees them, since they aren't currently
        meaningful within Hive by itself.)

        The only other SemanticAnalyzer dependency I see is getColumns, and
        that can be dealt with by moving it to become a static utility method
        on ParseUtils.

        Let me know if I missed any other cases that need to be dealt with.

        Show
        John Sichi added a comment - I think we can start off with this interface: /** * HiveSemanticAnalyzerHook allows Hive to be extended with custom * logic for semantic analysis of QL statements. This interface * and any Hive internals it exposes are currently * "limited private and evolving" (unless otherwise stated elsewhere) * and intended mainly for use by the Howl project. * *<p> * * Note that the lifetime of an instantiated hook object is scoped to * the analysis of a single statement; hook instances are never reused. */ public interface HiveSemanticAnalyzerHook { /** * Invoked before Hive performs its own semantic analysis on * a statement. The implementation may inspect the statement AST and * prevent its execution by throwing a SemanticException. * Optionally, it may also augment/rewrite the AST, but must produce * a form equivalent to one which could have * been returned directly from Hive's own parser. * * @param context context information for semantic analysis * * @param ast AST being analyzed and optionally rewritten * * @return replacement AST (typically the same as the original AST unless the * entire tree had to be replaced; must not be null) */ public ASTNode preAnalyze( HiveSemanticAnalyzerHookContext context, ASTNode ast) throws SemanticException; /** * Invoked after Hive performs its own semantic analysis on a * statement (including optimization). * Hive calls postAnalyze on the same hook object * as preAnalyze, so the hook can maintain state across the calls. * * @param context context information for semantic analysis * * @param rootTasks root tasks produced by semantic analysis; * the hook is free to modify this list or its contents */ public void postAnalyze( HiveSemanticAnalyzerHookContext context, List<Task<? extends Serializable>> rootTasks) throws SemanticException; } Plus companion context interface to be passed in from Hive: /** * Context information provided by Hive to implementations of HiveSemanticAnalyzerHook. */ public interface HiveSemanticAnalyzerHookContext { /** * @return the Hive db instance; hook implementations can use this for purposes such as getting configuration information or making metastore calls */ public Hive getHive(); } The reason for the context is so that later, if we need to make more information available to the hook, we just add new getters to the context object (rather than adding new parameters to methods such as preAnalyze, which would break existing hook implementations). Unlike pre/post exec hooks, we will only allow one hook to be configured (rather than a list). If someone really wants to run more than one, they can write a list hook implementation which delegates to multiple. The conf variable used to load the hook class will be hive.semantic.analyzer.hook since it's no longer a factory. We also need an insulation class: public abstract class AbstractSemanticAnalyzerHook implements HiveSemanticAnalyzerHook { public ASTNode preAnalyze( HiveSemanticAnalyzerHookContext context, ASTNode ast) throws SemanticException { return ast; } public void postAnalyze( HiveSemanticAnalyzerHookContext context, List<Task<? extends Serializable>> rootTasks) throws SemanticException { } } Hook implementations should extend this to avoid breaking when we add new methods to the HiveSemanticAnalyzerHook interface later. By using the hook approach, we limit the dependency exposure: only ASTNodes, Tasks, and the org.apache.hadoop.hive.ql.metadata.Hive class can be accessed for now. If we decide to open it up more later, that will be via an agreed-upon decision in a new patch (rather than via ad hoc dependency creep). Instead of invoking the hook from many different places inside of SemanticAnalyzer, we'll start with just invoking it only pre- and post- the call to sem.analyze. More invocation points (e.g. pre-optimization) can be added later on an as-needed basis. Howl's table property additions can be saved during the preAnalyze call and then applied to the Task during the postAnalyze call (since the hook is allowed to maintain state); this stays very close to the approach in the current patch. Another way to do this is to not use a postAnalyze at all, and instead just rewrite the AST to splice in the additional TBLPROPERTIES up front. Both ways are a little messy, but either way should be fine. For handleGenericFileFormat, I think we can deal with it by having Howl edit the AST to delete the STORED AS clause entirely during preAnalyze (in cases where it accepts the storage format). That way Hive can continue to reject it unconditionally, and we can avoid adding a third interface method. (Likewise, Howl should delete INPUTDRIVER and OUTPUTDRIVER, and Hive should reject them when it sees them, since they aren't currently meaningful within Hive by itself.) The only other SemanticAnalyzer dependency I see is getColumns, and that can be dealt with by moving it to become a static utility method on ParseUtils. Let me know if I missed any other cases that need to be dealt with.
        Hide
        Ashutosh Chauhan added a comment -

        @John,
        I took a look at the proposed new interface. I didn't go full in-depth into it yet to ascertain whether this works for Howl as desired or not, but I got the high level idea. It is a major redesign from the current patch. It will require some amount of design and dev work. This also implies all our testing (both unit and integration) which we have done on this patch till now is redundant and we need to redo that again as well.

        Show
        Ashutosh Chauhan added a comment - @John, I took a look at the proposed new interface. I didn't go full in-depth into it yet to ascertain whether this works for Howl as desired or not, but I got the high level idea. It is a major redesign from the current patch. It will require some amount of design and dev work. This also implies all our testing (both unit and integration) which we have done on this patch till now is redundant and we need to redo that again as well.
        Hide
        John Sichi added a comment -

        For the Hive part, it will be good if you can keep the same coverage as you have in the new unit tests from your last patch--the same cases can be validated regardless of hook vs factory.

        Show
        John Sichi added a comment - For the Hive part, it will be good if you can keep the same coverage as you have in the new unit tests from your last patch--the same cases can be validated regardless of hook vs factory.
        Hide
        Ashutosh Chauhan added a comment -

        @John,

        Neat work with hook interfaces. I was able to use the proposed one as it is without any modifications. Attaching the patch for incorporating these interfaces based on hooks in Hive.

        Show
        Ashutosh Chauhan added a comment - @John, Neat work with hook interfaces. I was able to use the proposed one as it is without any modifications. Attaching the patch for incorporating these interfaces based on hooks in Hive.
        Hide
        John Sichi added a comment -

        Great. I'm actually out on vacation until the end of next week, so if you want to get this one committed before then, ping Namit (njain at facebook dot com) and get him to review. Otherwise, I'll push it through once I get back.

        Show
        John Sichi added a comment - Great. I'm actually out on vacation until the end of next week, so if you want to get this one committed before then, ping Namit (njain at facebook dot com) and get him to review. Otherwise, I'll push it through once I get back.
        Hide
        Ashutosh Chauhan added a comment -

        Hey Namit,

        Would you like to review it?

        Show
        Ashutosh Chauhan added a comment - Hey Namit, Would you like to review it?
        Hide
        Namit Jain added a comment -

        I will take a look in more detail, but overall it looks good. I had the following comments:

        1. Instead of TestSemanticAnalyzerHookLoading.java, add tests in test/queries/clientpositive and test/queries/clientnegative
        2. Do you want to set the value of hive.semantic.analyzer.hook to a dummy value in data/conf/hive-site.xml for the unit tests ?
        Can something meaningful be printed here, which can be used for comparing ?

        Show
        Namit Jain added a comment - I will take a look in more detail, but overall it looks good. I had the following comments: 1. Instead of TestSemanticAnalyzerHookLoading.java, add tests in test/queries/clientpositive and test/queries/clientnegative 2. Do you want to set the value of hive.semantic.analyzer.hook to a dummy value in data/conf/hive-site.xml for the unit tests ? Can something meaningful be printed here, which can be used for comparing ?
        Hide
        Ashutosh Chauhan added a comment -

        I did it in junit form because John suggested it that way in his earlier comment:

        • We need a test for loading a variation on the default semantic analyzer in order to exercise the pluggable configuration. You can create a subclass of the default analyzer (under ql/src/test/org/apache/hadoop/hive/ql/parse) to inject some mock behavior change.

        I also feel junit test is better suited for this kind of behavioral testing of code paths (which exercises interface points) rather then forcing through string comparison ways of test/queries/* which are more end-to-end tests for hive. Further if we add dummy hook name in data/conf/hive-site.xml then that dummy hook will get loaded and all the subsequent tests will have it too. Do we want it that way?

        Show
        Ashutosh Chauhan added a comment - I did it in junit form because John suggested it that way in his earlier comment: We need a test for loading a variation on the default semantic analyzer in order to exercise the pluggable configuration. You can create a subclass of the default analyzer (under ql/src/test/org/apache/hadoop/hive/ql/parse) to inject some mock behavior change. I also feel junit test is better suited for this kind of behavioral testing of code paths (which exercises interface points) rather then forcing through string comparison ways of test/queries/* which are more end-to-end tests for hive. Further if we add dummy hook name in data/conf/hive-site.xml then that dummy hook will get loaded and all the subsequent tests will have it too. Do we want it that way?
        Hide
        Ashutosh Chauhan added a comment -

        @Namit,

        Did you get a chance to take a look? I want to move forward and eager to contribute more to Hive, rather then stuck on this forever : )

        Show
        Ashutosh Chauhan added a comment - @Namit, Did you get a chance to take a look? I want to move forward and eager to contribute more to Hive, rather then stuck on this forever : )
        Hide
        Namit Jain added a comment -

        +1

        Otherwise, the changes are good - the patch is not applying cleanly.

        Can you refresh and regenerate the patch ? I will take care of it

        Show
        Namit Jain added a comment - +1 Otherwise, the changes are good - the patch is not applying cleanly. Can you refresh and regenerate the patch ? I will take care of it
        Hide
        Ashutosh Chauhan added a comment -

        Patch did apply cleanly for me. Anyways, I regenerated it on latest trunk (Revision: 1005669)
        Hopefully, this one will make it in time.

        Show
        Ashutosh Chauhan added a comment - Patch did apply cleanly for me. Anyways, I regenerated it on latest trunk (Revision: 1005669) Hopefully, this one will make it in time.
        Hide
        Namit Jain added a comment -

        Committed. Thanks Ashutosh

        Show
        Namit Jain added a comment - Committed. Thanks Ashutosh

          People

          • Assignee:
            Ashutosh Chauhan
            Reporter:
            Ashutosh Chauhan
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development