[GitHub] [flink] matriv commented on a diff in pull request #19410: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader

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

matriv commented on code in PR #19410:
URL: https://github.com/apache/flink/pull/19410#discussion_r846285936


##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/catalog/FunctionCatalog.java:
##########
@@ -621,7 +626,7 @@ public void registerTemporarySystemFunction(
     }
 
     @SuppressWarnings("unchecked")
-    private void validateAndPrepareFunction(CatalogFunction function)
+    private CatalogFunction validateAndPrepareFunction(CatalogFunction function)

Review Comment:
   Just a double check, have you checked all usages to make sure we use the returned value?



##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/utils/PlannerMocks.java:
##########
@@ -96,6 +102,7 @@ private PlannerMocks(
                 true,
                 ExpressionResolver.resolverFor(
                         tableConfig,
+                        Thread.currentThread().getContextClassLoader(),

Review Comment:
   Why not use `PlannerMocks.class.getClassLoader()` here?



##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/utils/EncodingUtils.java:
##########
@@ -139,6 +139,8 @@ public static String encodeObjectToString(Serializable obj) {
         }
     }
 
+    /** @see #loadClass(String, ClassLoader) */

Review Comment:
   please add `@deprecated`



##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/catalog/CatalogFunction.java:
##########
@@ -59,6 +59,7 @@
      *
      * @return whether the function is a generic function
      */
+    @Deprecated

Review Comment:
   Please add `@deprecated` section to the javadoc



##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/plan/rules/physical/stream/StreamPhysicalJoinRuleBase.scala:
##########
@@ -25,6 +25,11 @@ import org.apache.flink.table.planner.plan.nodes.{FlinkConventions, FlinkRelNode
 import org.apache.flink.table.planner.plan.utils.IntervalJoinUtil
 import org.apache.flink.table.planner.utils.ShortcutUtils.unwrapTableConfig
 
+import org.apache.flink.table.planner.plan.utils.{FlinkRelOptUtil, IntervalJoinUtil}

Review Comment:
   leftover?



##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/functions/UserDefinedFunctionHelper.java:
##########
@@ -244,17 +245,23 @@ public static UserDefinedFunction instantiateFunction(Class<?> functionClass) {
     }
 
     /** Prepares a {@link UserDefinedFunction} instance for usage in the API. */
-    public static void prepareInstance(ReadableConfig config, UserDefinedFunction function) {
+    public static <T extends UserDefinedFunction> T prepareInstance(

Review Comment:
   Maybe add to the javadoc, that the caller needs to use the returned value?



-- 
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: issues-unsubscribe@flink.apache.org

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