[jira] [Work logged] (HIVE-26488) Fix NPE in DDLSemanticAnalyzerFactory during compilation

["ASF GitHub Bot (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/HIVE-26488?focusedWorklogId=808221&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-808221 ]

ASF GitHub Bot logged work on HIVE-26488:
-----------------------------------------

                Author: ASF GitHub Bot
            Created on: 13/Sep/22 11:34
            Start Date: 13/Sep/22 11:34
    Worklog Time Spent: 10m 
      Work Description: zabetak commented on code in PR #3538:
URL: https://github.com/apache/hive/pull/3538#discussion_r969499557


##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/DDLSemanticAnalyzerFactory.java:
##########
@@ -65,10 +68,12 @@ public interface DDLSemanticAnalyzerCategory {
       new HashMap<>();
 
   static {
-    Set<Class<? extends BaseSemanticAnalyzer>> analyzerClasses1 =
-        new Reflections(DDL_ROOT).getSubTypesOf(BaseSemanticAnalyzer.class);
-    Set<Class<? extends CalcitePlanner>> analyzerClasses2 =
-        new Reflections(DDL_ROOT).getSubTypesOf(CalcitePlanner.class);
+    Set<Class<? extends BaseSemanticAnalyzer>> analyzerClasses1 = new Reflections(
+        new ConfigurationBuilder()
+            .setUrls(ClasspathHelper.forPackage(DDL_ROOT)).filterInputsBy(new FilterBuilder().includePackage(DDL_ROOT)).setExpandSuperTypes(false)).getSubTypesOf(BaseSemanticAnalyzer.class);
+    Set<Class<? extends CalcitePlanner>> analyzerClasses2 = new Reflections(
+        new ConfigurationBuilder().filterInputsBy(new FilterBuilder().includePackage(DDL_ROOT))
+            .setUrls(ClasspathHelper.forPackage(DDL_ROOT)).setExpandSuperTypes(false)).getSubTypesOf(CalcitePlanner.class);
     Set<Class<? extends BaseSemanticAnalyzer>> analyzerClasses = Sets.union(analyzerClasses1, analyzerClasses2);
     for (Class<? extends BaseSemanticAnalyzer> analyzerClass : analyzerClasses) {

Review Comment:
   I think that if you specify `.setUrls(ClasspathHelper.forPackage(DDL_ROOT))` then `filterInputsBy(new FilterBuilder().includePackage(DDL_ROOT))` is actually redundant.
   
   `setExpandSuperTypes(false)` actually restores the behavior of `0.9.10` version but I think we don't need it. Actually we could take advantage of this new feature of Reflections library to get rid of the separate call to `getSubTypesOf(CalcitePlanner.class)` followed by `Sets.union` etc.
   
   Basically I think it suffices to just keep the snippet below and remove the rest.
   ```java
   Set<Class<? extends BaseSemanticAnalyzer>> analyzerClasses1 =
           new Reflections(DDL_ROOT).getSubTypesOf(BaseSemanticAnalyzer.class);
   ```
   As explained in the JIRA this code will also include `SemanticAnalyzer` class and possibly other classes that are not annotated with `DDLType` so we need to find a way to avoid the NPE. 
   
   I think it is better to introduce a null check below (`if (ddlType == null) continue`) instead of assuming that every class returned by the Reflections library is gonna be annotated appropriately; the Reflection APIs that we are currently using cannot guarantee that.
   
   We could possibly use other Reflections APIs to get only annotated types such as:
   ```java
   new Reflections(DDL_ROOT).getTypesAnnotatedWith(DDLType.class);
   ```
   but this requires a bigger refactoring so I would suggest to keep it for follow-up JIRA.



##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/DDLTask.java:
##########
@@ -45,8 +46,11 @@ public final class DDLTask extends Task<DDLWork> implements Serializable {
       new HashMap<>();
 
   static {
-    Set<Class<? extends DDLOperation>> operationClasses =
-        new Reflections("org.apache.hadoop.hive.ql.ddl").getSubTypesOf(DDLOperation.class);
+    String DDL_ROOT = "org.apache.hadoop.hive.ql.ddl";
+    Set<Class<? extends DDLOperation>> operationClasses = new Reflections(
+        new ConfigurationBuilder().setUrls(ClasspathHelper.forPackage(DDL_ROOT))
+            .filterInputsBy(new FilterBuilder().includePackage(DDL_ROOT)).setExpandSuperTypes(false))
+        .getSubTypesOf(DDLOperation.class);

Review Comment:
   Same comments here maybe we shall leave the code as is.





Issue Time Tracking
-------------------

    Worklog Id:     (was: 808221)
    Time Spent: 1h 20m  (was: 1h 10m)

> Fix NPE in DDLSemanticAnalyzerFactory during compilation
> --------------------------------------------------------
>
>                 Key: HIVE-26488
>                 URL: https://issues.apache.org/jira/browse/HIVE-26488
>             Project: Hive
>          Issue Type: Sub-task
>            Reporter: Ayush Saxena
>            Assignee: Ayush Saxena
>            Priority: Major
>              Labels: pull-request-available
>          Time Spent: 1h 20m
>  Remaining Estimate: 0h
>
> *Exception Trace:*
> {noformat}
> java.lang.ExceptionInInitializerError
> 	at org.apache.hadoop.hive.ql.parse.SemanticAnalyzerFactory.getInternal(SemanticAnalyzerFactory.java:62)
> 	at org.apache.hadoop.hive.ql.parse.SemanticAnalyzerFactory.get(SemanticAnalyzerFactory.java:41)
> 	at org.apache.hadoop.hive.ql.Compiler.analyze(Compiler.java:209)
> 	at org.apache.hadoop.hive.ql.Compiler.compile(Compiler.java:106)
> 	at org.apache.hadoop.hive.ql.Driver.compile(Driver.java:507)
> 	at org.apache.hadoop.hive.ql.Driver.compileInternal(Driver.java:459)
> 	at org.apache.hadoop.hive.ql.Driver.compileAndRespond(Driver.java:424)
> 	at org.apache.hadoop.hive.ql.Driver.compileAndRespond(Driver.java:418)
> {noformat}
> *Cause:*
> {noformat}
> Caused by: java.lang.NullPointerException
> 	at org.apache.hadoop.hive.ql.ddl.DDLSemanticAnalyzerFactory.<clinit>(DDLSemanticAnalyzerFactory.java:84)
> 	... 40 more
> {noformat}



--
This message was sent by Atlassian Jira
(v8.20.10#820010)