[jira] [Updated] (HIVE-2779) Improve hooks run in Driver

["Phabricator (Updated) (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/HIVE-2779?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Phabricator updated HIVE-2779:
------------------------------

    Attachment: HIVE-2779.D1599.1.patch

kevinwilfong requested code review of "HIVE-2779 [jira] Improve hooks run in Driver".
Reviewers: JIRA

  https://issues.apache.org/jira/browse/HIVE-2779

  Merged the various methods in Driver to get hooks into a single method.  I took care to ensure that the checks that were occurring for SemanticAnalyzer hooks, but not for other hooks, as they are not necessarily applicable.

  While doing this, I also mentioned in the comments on the method that the hooks are returned in the same order they were specified, and this was intentional.

  I also updated the HiveSemanticAnalyzerHookContext to include the inputs and outputs from the SemanticAnalyzer before it is passed to any hooks which run after analysis.  I wrote it in a way such that it should not be difficult to add more should we need them.

  There are some small improvements that can be made to the hooks which are run in the Driver:

  1) The code to get hooks has been clearly just been copy+pasted for each of Pre/Post/OnFailure/SemanticAnalyzer hooks.  This code should be consolidated into a single method.

  2) There is a lot more information available to SemanticAnalyzer hooks which ran after semantic analysis than to those that run before, such as inputs and outputs.  We should make some of this information available to those hooks, preferably through HiveSemanticAnalyzerHookContext, so that existing hooks aren't broken.

  3) Currently, possibly unintentionally, hooks are initialized and run in the order they appear in the comma separated list that is the value of the configuration variable.  This is a useful property, we should add comments indicating this is desired and add a unit test to enforce it.

TEST PLAN
  EMPTY

REVISION DETAIL
  https://reviews.facebook.net/D1599

AFFECTED FILES
  ql/src/test/results/clientnegative/bad_exec_hooks.q.out
  ql/src/test/results/clientpositive/hook_order.q.out
  ql/src/test/org/apache/hadoop/hive/ql/hooks/VerifyHooksRunInOrder.java
  ql/src/test/queries/clientpositive/hook_order.q
  ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java
  ql/src/java/org/apache/hadoop/hive/ql/parse/HiveSemanticAnalyzerHook.java
  ql/src/java/org/apache/hadoop/hive/ql/parse/HiveSemanticAnalyzerHookContext.java
  ql/src/java/org/apache/hadoop/hive/ql/parse/HiveSemanticAnalyzerHookContextImpl.java
  ql/src/java/org/apache/hadoop/hive/ql/Driver.java

MANAGE HERALD DIFFERENTIAL RULES
  https://reviews.facebook.net/herald/view/differential/

WHY DID I GET THIS EMAIL?
  https://reviews.facebook.net/herald/transcript/3399/

Tip: use the X-Herald-Rules header to filter Herald messages in your client.

                
> Improve hooks run in Driver
> ---------------------------
>
>                 Key: HIVE-2779
>                 URL: https://issues.apache.org/jira/browse/HIVE-2779
>             Project: Hive
>          Issue Type: Improvement
>            Reporter: Kevin Wilfong
>            Assignee: Kevin Wilfong
>         Attachments: HIVE-2779.D1599.1.patch
>
>
> There are some small improvements that can be made to the hooks which are run in the Driver:
> 1) The code to get hooks has been clearly just been copy+pasted for each of Pre/Post/OnFailure/SemanticAnalyzer hooks.  This code should be consolidated into a single method.
> 2) There is a lot more information available to SemanticAnalyzer hooks which ran after semantic analysis than to those that run before, such as inputs and outputs.  We should make some of this information available to those hooks, preferably through HiveSemanticAnalyzerHookContext, so that existing hooks aren't broken.
> 3) Currently, possibly unintentionally, hooks are initialized and run in the order they appear in the comma separated list that is the value of the configuration variable.  This is a useful property, we should add comments indicating this is desired and add a unit test to enforce it.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira