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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
