Skip to content
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

Closed
wants to merge 3 commits into from

Conversation

davies
Copy link
Contributor

@davies davies commented Jun 19, 2015

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

@SparkQA
Copy link

SparkQA commented Jun 19, 2015

Test build #35242 has finished for PR 6898 at commit 7f5ffbe.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jun 19, 2015

Test build #932 has finished for PR 6898 at commit 7f5ffbe.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

// 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)
Copy link
Contributor

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

@SparkQA
Copy link

SparkQA commented Jun 19, 2015

Test build #933 has finished for PR 6898 at commit 7f5ffbe.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@yhuai
Copy link
Contributor

yhuai commented Jun 19, 2015

Can we add a test in ReplSuite (REPL's tests already has sql core as a dependency)?

@yhuai
Copy link
Contributor

yhuai commented Jun 19, 2015

One question that may not related to this pr. If we define a udf in the repl (or user use --jars add a jar containing a udf) and later when we generate code at executor side that calls this udf, can we still find the class?

@davies
Copy link
Contributor Author

davies commented Jun 19, 2015

@yhuai The UDF will not be part of generated Java code, so Janino doesn't need to import it. UDF will fallback to call eval(), because UDF can not be presented in Java source.

@SparkQA
Copy link

SparkQA commented Jun 19, 2015

Test build #35273 has finished for PR 6898 at commit 4ff0457.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • abstract class GeneratedClass

@SparkQA
Copy link

SparkQA commented Jun 19, 2015

Test build #35276 has finished for PR 6898 at commit 24276d4.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • abstract class GeneratedClass

@liancheng
Copy link
Contributor

Should we also add a REPL test case which uses HiveContext to ensure the isolated classloader plays well with code generation? I know that HiveContext only exists on driver side while code generation happens on executor side, what I'm thinking is there might be chances that they shoot at each other in local mode where everything happens in a single JVM. This can be done in a separate PR since this is a hot fix.

Otherwise looks good, but I'm not super familiar with the whole code generation stuff though.

@davies
Copy link
Contributor Author

davies commented Jun 19, 2015

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

@asfgit asfgit closed this in e41e2fd Jun 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants