[GitHub] [flink] wuchong commented on a diff in pull request #19561: [FLINK-26414][hive] Hive dialect supports macro

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

wuchong commented on code in PR #19561:
URL: https://github.com/apache/flink/pull/19561#discussion_r914397777


##########
flink-connectors/flink-connector-hive/src/main/java/org/apache/flink/table/functions/hive/HiveFunctionWrapper.java:
##########
@@ -36,23 +38,48 @@
     public static final long serialVersionUID = 393313529306818205L;
 
     private final String className;
+    // a field to hold the string serialized for the UDF.
+    // we sometimes need to hold it in case of some serializable UDF will contain
+    // additional information such as Hive's GenericUDFMacro and if we construct the UDF directly by
+    // getUDFClass#newInstance, the information will be missed.
+    private String udfSerializedString;
 
     private transient UDFType instance = null;
 
     public HiveFunctionWrapper(String className) {
         this.className = className;
     }
 
+    /**
+     * Create a HiveFunctionWrapper with a UDF instance. In this constructor, the instance will be
+     * serialized to string and held on in the HiveFunctionWrapper.
+     */
+    public HiveFunctionWrapper(String className, UDFType serializableInstance) {
+        this(className);
+        Preconditions.checkArgument(
+                serializableInstance instanceof Serializable,
+                String.format(
+                        "The UDF %s should be an instance of Serializable.",
+                        serializableInstance.getClass().getName()));

Review Comment:
   We should check the class name is equal to the class of `serializableInstance`.



##########
flink-connectors/flink-connector-hive/src/main/java/org/apache/flink/table/planner/delegation/hive/parse/HiveParserDDLSemanticAnalyzer.java:
##########
@@ -506,6 +533,109 @@ private Operation convertCreateFunction(HiveParserASTNode ast) {
         }
     }
 
+    private Operation convertCreateMacro(HiveParserASTNode ast) throws SemanticException {
+        String macroName = ast.getChild(0).getText();
+        if (FunctionUtils.isQualifiedFunctionName(macroName)) {
+            throw new SemanticException("The name of the temporary macro can't contain `.`.");

Review Comment:
   Please also print the wrong function name. For example: 
   
   `CREATE TEMPORARY MACRO doesn't allow "." character in the macro name, but the name is "%s"`. 
   
   Besides, is there a test for the exception? 



##########
flink-connectors/flink-connector-hive/src/main/java/org/apache/flink/table/functions/hive/HiveFunctionWrapper.java:
##########
@@ -88,4 +115,20 @@ public String getClassName() {
     public Class<UDFType> getUDFClass() throws ClassNotFoundException {
         return (Class<UDFType>) Thread.currentThread().getContextClassLoader().loadClass(className);
     }
+
+    /**
+     * Deserialize UDF used the udfSerializedString held on.
+     *
+     * @return the UDF deserialized
+     */
+    private UDFType deserializeUDF() {
+        try {
+            return (UDFType)
+                    SerializationUtilities.deserializeObject(
+                            udfSerializedString, (Class<Serializable>) getUDFClass());

Review Comment:
   This is problematic in the case the hive connector is dynamically loaded, i.e. in the user classloader instead of the current class loader. We should use the user classloader to load classes. Could you create a JIRA issue for this? 



-- 
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