[GitHub] [hive] zabetak commented on a diff in pull request #3538: HIVE-26488: Fix NPE in DDLSemanticAnalyzerFactory during compilation.

[GitBox <gi...@apache.org>]

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.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org