You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/05/30 10:03:09 UTC

[GitHub] [flink] lsyldliu opened a new pull request, #19845: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader

lsyldliu opened a new pull request, #19845:
URL: https://github.com/apache/flink/pull/19845

   This PR solves the issue of allowing the user to provide its own classloader for UDFs and at other places where it's more appropriate than the current Thread context classloader.
   
   The PR also contains a couple of hotfixes (mergeable separately):
   
   Deprecate CatalogFunction#isGeneric
   Remove CodeGeneratorContext#apply which makes the code unnecessarily more complicated
   Fix singleton config in EnvironmentSettings, which among various issues, it's also causing in this case to select the wrong user class loader


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

[GitHub] [flink] wuchong commented on pull request #19845: [FLINK-15635][table] EnvironmentSettings supports to accept the user ClassLoader

Posted by GitBox <gi...@apache.org>.
wuchong commented on PR #19845:
URL: https://github.com/apache/flink/pull/19845#issuecomment-1149484853

   Merging...


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

[GitHub] [flink] lsyldliu commented on pull request #19845: [FLINK-15635][table] EnvironmentSettings supports to accept the user ClassLoader

Posted by GitBox <gi...@apache.org>.
lsyldliu commented on PR #19845:
URL: https://github.com/apache/flink/pull/19845#issuecomment-1143311384

   > Hi @lsyldliu , it seems that the spotless is failed. Could you fix it?
   Thanks for reminder, I have fixed it.
   


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

[GitHub] [flink] wuchong commented on a diff in pull request #19845: [FLINK-15635][table] EnvironmentSettings supports to accept the user ClassLoader

Posted by GitBox <gi...@apache.org>.
wuchong commented on code in PR #19845:
URL: https://github.com/apache/flink/pull/19845#discussion_r886303859


##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala:
##########
@@ -1763,6 +1763,11 @@ object ScalarOperatorGens {
     }
   }
 
+  /**
+   * Note that this cast context is going to use the thread context classloader. This is fine when
+   * the context will be used to generate casting for primitive types, but it might create problems
+   * when dealing with user provided types.

Review Comment:
   This sounds problematic? Why not use `userClassloader` here? 



##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/CodeGeneratorContext.scala:
##########
@@ -701,11 +701,7 @@ class CodeGeneratorContext(val tableConfig: ReadableConfig) {
       fieldTerm: String,
       fieldTypeTerm: String): Unit = {
     val idx = references.length
-    // make a deep copy of the object
-    val byteArray = InstantiationUtil.serializeObject(obj)
-    val objCopy: AnyRef =
-      InstantiationUtil.deserializeObject(byteArray, Thread.currentThread().getContextClassLoader)
-    references += objCopy
+    references += obj

Review Comment:
   Hi @slinkydeveloper ,  I think the `CodeGeneratorContext` doesn't know whether the `obj` would be mutated or not, so it would be safer to clone the `obj` here. Do you have any concerns to copy the object?



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

[GitHub] [flink] wuchong commented on pull request #19845: [FLINK-15635][table] EnvironmentSettings supports to accept the user ClassLoader

Posted by GitBox <gi...@apache.org>.
wuchong commented on PR #19845:
URL: https://github.com/apache/flink/pull/19845#issuecomment-1144506307

   Besides, there are many cases are failed, could you take a look?
   


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

[GitHub] [flink] wuchong commented on pull request #19845: [FLINK-15635][table] EnvironmentSettings supports to accept the user ClassLoader

Posted by GitBox <gi...@apache.org>.
wuchong commented on PR #19845:
URL: https://github.com/apache/flink/pull/19845#issuecomment-1149410600

   @lsyldliu could you help to have a look at the test? It seems {{SplitAggregateITCase.testMinMaxWithRetraction}} is failed. 


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

[GitHub] [flink] lsyldliu commented on pull request #19845: [FLINK-15635][table] EnvironmentSettings supports to accept the user ClassLoader

Posted by GitBox <gi...@apache.org>.
lsyldliu commented on PR #19845:
URL: https://github.com/apache/flink/pull/19845#issuecomment-1149422004

   @flinkbot rerun


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

[GitHub] [flink] wuchong merged pull request #19845: [FLINK-15635][table] EnvironmentSettings supports to accept the user ClassLoader

Posted by GitBox <gi...@apache.org>.
wuchong merged PR #19845:
URL: https://github.com/apache/flink/pull/19845


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

[GitHub] [flink] flinkbot commented on pull request #19845: [FLINK-15635][table] Now EnvironmentSettings accepts the user ClassLoader

Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #19845:
URL: https://github.com/apache/flink/pull/19845#issuecomment-1140960701

   <!--
   Meta data
   {
     "version" : 1,
     "metaDataEntries" : [ {
       "hash" : "52061c1eaa198d76f2ef44a8043cc9175708edd6",
       "status" : "UNKNOWN",
       "url" : "TBD",
       "triggerID" : "52061c1eaa198d76f2ef44a8043cc9175708edd6",
       "triggerType" : "PUSH"
     } ]
   }-->
   ## CI report:
   
   * 52061c1eaa198d76f2ef44a8043cc9175708edd6 UNKNOWN
   
   <details>
   <summary>Bot commands</summary>
     The @flinkbot bot supports the following commands:
   
    - `@flinkbot run azure` re-run the last Azure build
   </details>


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

[GitHub] [flink] wuchong commented on pull request #19845: [FLINK-15635][table] EnvironmentSettings supports to accept the user ClassLoader

Posted by GitBox <gi...@apache.org>.
wuchong commented on PR #19845:
URL: https://github.com/apache/flink/pull/19845#issuecomment-1148754236

   There are too many commits. I will squash them into 2 commits. 


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

[GitHub] [flink] wuchong commented on pull request #19845: [FLINK-15635][table] EnvironmentSettings supports to accept the user ClassLoader

Posted by GitBox <gi...@apache.org>.
wuchong commented on PR #19845:
URL: https://github.com/apache/flink/pull/19845#issuecomment-1142045906

   Hi @lsyldliu , it seems that the spotless is failed. Could you fix it? 


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