-
Notifications
You must be signed in to change notification settings - Fork 28.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-8461] [SQL] fix codegen with REPL class loader #6898
Conversation
Test build #35242 has finished for PR 6898 at commit
|
Test build #932 has finished for PR 6898 at commit
|
// class in java.lang. It's also slow, use the default JVM class loader. | ||
val currentThread = Thread.currentThread() | ||
val oldClassLoader = currentThread.getContextClassLoader | ||
currentThread.setContextClassLoader(getClass.getClassLoader) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we do not need to set setContextClassLoader. and we can set ClassBodyEvaluator's classLoader. code is:
val classBodyEval =new ClassBodyEvaluator()
classBodyEval.setParentClassLoader(getClass.getClassLoader)
classBodyEval.cook(code)
classBodyEval.getClazz()
some information: http://janino.net/javadoc/org/codehaus/janino/ClassBodyEvaluator.html,
https://github.com/codehaus/janino/blob/6e61d5b71b4e8682a3497cb1e33a005bee4ca0a2/janino/src/org/codehaus/janino/SimpleCompiler.java#L172
Test build #933 has finished for PR 6898 at commit
|
Can we add a test in |
One question that may not related to this pr. If we define a udf in the repl (or user use |
@yhuai The UDF will not be part of generated Java code, so Janino doesn't need to import it. UDF will fallback to call |
Test build #35273 has finished for PR 6898 at commit
|
Test build #35276 has finished for PR 6898 at commit
|
Should we also add a REPL test case which uses Otherwise looks good, but I'm not super familiar with the whole code generation stuff though. |
@liancheng Thanks for pointing this out, test always come with costs (time to run), so I'd like delay this until when we really need it. After this patch, Janino only use the default class loader, so it shouldn't be a problem. Merging this into master as a hotfix! |
The ExecutorClassLoader for REPL will cause Janino failed to find class for those in java.lang, so switch to use default class loader for Janino, which will also help performance.
cc @liancheng @yhuai