Skip to content

[SPARK-18016][SQL][CATALYST] Code Generation: Constant Pool Limit - Class Splitting #18075

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.

Sign up for GitHub

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

Closed
bdrillard wants to merge 9 commits into from
Closed

[SPARK-18016][SQL][CATALYST] Code Generation: Constant Pool Limit - Class Splitting #18075

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
76e291b
class_splitting_only adding class splitting
May 23, 2017
b6bf6db
class_splitting_only addressing code review comments: typo, case stat…
May 24, 2017
28fc548
class_splitting_only refactoring classFunctions to use a simpler list
May 24, 2017
d30d097
class_splitting_only fixing error in pom scalatest-maven-plugin
May 24, 2017
442332b
class_splitting_only more consistent use of assoc notation
May 24, 2017
a1c93fb
class_splitting_only fixing classFunctions buffer append
May 25, 2017
1086bb3
class_splitting_only removing instances of this, adding nested class …
May 30, 2017
7fe5e4a
class_splitting_only using a map for functions since some functions m…
May 31, 2017
678b4ad
class_splitting_only addressing review comments
Jun 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
class_splitting_only fixing classFunctions buffer append
  • Loading branch information
ALeksander Eskilson committed May 31, 2017
commit a1c93fbc52a873fd8acdb7607388d56bc183a9bc
7 changes: 5 additions & 2 deletions ...lyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import scala.util.control.NonFatal

import com.google.common.cache.{CacheBuilder, CacheLoader}
import com.google.common.util.concurrent.{ExecutionError, UncheckedExecutionException}
import org.apache.commons.lang3.exception.ExceptionUtils
import org.codehaus.commons.compiler.CompileException
import org.codehaus.janino.{ByteArrayClassLoader, ClassBodyEvaluator, JaninoRuntimeException, SimpleCompiler}
import org.codehaus.janino.util.ClassFile
Expand Down Expand Up @@ -261,6 +260,10 @@ class CodegenContext {
*
* @param funcName the class-unqualified name of the function
* @param funcCode the body of the function
* @param inlineToOuterClass whether the given code must be inlined to the `OuterClass`. This
Copy link
Contributor

@cloud-fan cloud-fan Jun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you give an example? I'm not very clear when we need this

Copy link
Author

@bdrillard bdrillard Jun 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, see the portion of doConsume in the Limit class where the stopEarly function is registered, https://github.com/apache/spark/pull/18075/files#diff-379cccace8699ca00b76ff5631222adeR73

In this section of code, the registration of the function is separate from the caller code, so unlike other changes in this patch, we have no way of informing the caller code what the potentially class-qualified name of the function would be if it were inlined to a nested class. Instead, the caller code for the function (in WholeStageCodegenExec), makes a hard assumption that stopEarly will be visible globally, that is, in the outer class. The caller is divorced from the function producer across classes, so it's not clear how to make a generated function name visible, but the hint to inline to just inline the function to the outer class fixes that issue.

Copy link
Member

@viirya viirya Jun 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me, as the stopEarly in Limit is going to override the stopEarly in BufferedRowIterator, we can only put it in outer class.

Copy link
Contributor

@cloud-fan cloud-fan Jun 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, whole stage codegen is really tricky...

* can be necessary when a function is declared outside of the context
* it is eventually referenced and a returned qualified function name
* cannot otherwise be accessed.
* @return the name of the function, qualified by class if it will be inlined to a private,
Copy link
Member

@viirya viirya Jun 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto.

* nested sub-class
*/
Expand All @@ -287,7 +290,7 @@ class CodegenContext {
val name = classInfo._1

classSize.update(name, classSize(name) + funcCode.length)
classFunctions.update(name, classFunctions(name) += funcCode)
classFunctions(name).append(funcCode)

if (name.equals("OuterClass")) {
funcName
Expand Down