-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-22226][SQL] splitExpression can create too many method calls in the outer class #19480
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
Changes from 3 commits
c2cc295
d3a5b87
e63264b
bdc2fdb
b9ffc52
831fc40
76b5489
20626b4
5f39500
c4601b4
61cc445
37506dc
95b0ad8
bce3616
c4ab587
0215139
41c0b2b
6a1eeca
be84c4d
a39098d
6e8cd00
36d8e2e
4952880
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,6 +77,20 @@ case class SubExprEliminationState(isNull: String, value: String) | |
| */ | ||
| case class SubExprCodes(codes: Seq[String], states: Map[Expression, SubExprEliminationState]) | ||
|
|
||
| /** | ||
| * The main information about a new added function. | ||
| * | ||
| * @param functionName String representing the name of the function | ||
| * @param subclassName Optional value which is empty if the function is added to | ||
| * the outer class, otherwise it contains the name of the | ||
| * inner class in which the function has been added. | ||
| * @param subclassInstance Optional value which is empty if the function is added to | ||
| * the superclass, otherwise it contains the name of the | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. superclass here too.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks! |
||
| * instance of the inner class in the outer class. | ||
| */ | ||
| private[codegen] case class NewFunction(functionName: String, subclassName: Option[String], | ||
| subclassInstance: Option[String]) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indent issues. |
||
|
|
||
| /** | ||
| * A context for codegen, tracking a list of objects that could be passed into generated Java | ||
| * function. | ||
|
|
@@ -277,13 +291,25 @@ class CodegenContext { | |
| funcName: String, | ||
| funcCode: String, | ||
| inlineToOuterClass: Boolean = false): String = { | ||
| val newFunction = addNewFunctionInternal(funcName, funcCode, inlineToOuterClass) | ||
| newFunction match { | ||
| case NewFunction(functionName, None, None) => functionName | ||
| case NewFunction(functionName, Some(_), Some(subclassInstance)) => | ||
| subclassInstance + "." + functionName | ||
| } | ||
| } | ||
|
|
||
| private[this] def addNewFunctionInternal( | ||
| funcName: String, | ||
| funcCode: String, | ||
| inlineToOuterClass: Boolean): NewFunction = { | ||
| // The number of named constants that can exist in the class is limited by the Constant Pool | ||
| // limit, 65,536. We cannot know how many constants will be inserted for a class, so we use a | ||
| // threshold of 1600k bytes to determine when a function should be inlined to a private, nested | ||
| // threshold of 1000k bytes to determine when a function should be inlined to a private, nested | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason to change this?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it for the possibly to be add method grouping all split methods? If yes, we can add the method into an inner class (see #19480 (comment)), so we don't need to change this threshold.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, without this change, I got the Constant Pool limit exception of the {{NestedClass}}. Actually I tried to address this issue in this PR (#19447), but without the other changes in thee current PR I wasn't able to create a UT for it.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you try #19480 (comment), so we may avoid changing this threshold?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't change that for the reason I explained there. |
||
| // sub-class. | ||
| val (className, classInstance) = if (inlineToOuterClass) { | ||
| outerClassName -> "" | ||
| } else if (currClassSize > 1600000) { | ||
| } else if (currClassSize > 1000000) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the reason we chose this magic number in the original PR? cc @bdrillard @kiszk There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gatorsmile it's a byte threshold, similar to the 1024 byte threshold set in In short, I tested different types of schemas with many columns to find what the value could be set to empirically. There's no particular harm in setting the value lower as is done here if it helps us avoid a known constant pool error case. Doing so would effectively reduce the number of expressions each nested class holds, and so also increase the number of nested classes in total.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, actually during several tries, I found that setting the value lower can somehow reduce the chance to hit constant pool limit exception in nested classes.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, this is the reason why I created this PR: this is an estimation, which might be good 99% of the times, but for those other 1% use cases it would be good to be able to tune it to prevent this error IMHO.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Create a variable in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can do that for this PR. But what do you think about introducing an internal configuration for this as I proposed in the other PR? If this should not be done, I can close the other PR: I think that the discussion here and there showed the reasons for its creation. If they are not enough, then I will close the PR. |
||
| val className = freshName("NestedClass") | ||
| val classInstance = freshName("nestedClassInstance") | ||
|
|
||
|
|
@@ -294,17 +320,23 @@ class CodegenContext { | |
| currClass() | ||
| } | ||
|
|
||
| classSize(className) += funcCode.length | ||
| classFunctions(className) += funcName -> funcCode | ||
| addNewFunctionToClass(funcName, funcCode, className) | ||
|
|
||
| if (className == outerClassName) { | ||
| funcName | ||
| NewFunction(funcName, None, None) | ||
| } else { | ||
|
|
||
| s"$classInstance.$funcName" | ||
| NewFunction(funcName, Some(className), Some(classInstance)) | ||
| } | ||
| } | ||
|
|
||
| private[this] def addNewFunctionToClass( | ||
| funcName: String, | ||
| funcCode: String, | ||
| className: String) = { | ||
| classSize(className) += funcCode.length | ||
| classFunctions(className) += funcName -> funcCode | ||
| } | ||
|
|
||
| /** | ||
| * Declares all function code. If the added functions are too many, split them into nested | ||
| * sub-classes to avoid hitting Java compiler constant pool limitation. | ||
|
|
@@ -798,10 +830,35 @@ class CodegenContext { | |
| | ${makeSplitFunction(body)} | ||
| |} | ||
| """.stripMargin | ||
| addNewFunction(name, code) | ||
| addNewFunctionInternal(name, code, inlineToOuterClass = false) | ||
| } | ||
|
|
||
| foldFunctions(functions.map(name => s"$name(${arguments.map(_._2).mkString(", ")})")) | ||
| val outerClassFunctions = functions | ||
| .filter(_.subclassName.isEmpty) | ||
| .map(_.functionName) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the calls to outerclass functions should not be an issue, even they could be many?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. exactly, because since they are defined there, they add no entry to the constant pool |
||
|
|
||
| val innerClassFunctions = functions | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might not be very intuitive for later readers. We'd better to add comments to explain it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will, thanks for the suggestion |
||
| .filter(_.subclassName.isDefined) | ||
| .foldLeft(Map.empty[(String, String), Seq[String]]) { case (acc, f) => | ||
| val key = (f.subclassName.get, f.subclassInstance.get) | ||
| acc.updated(key, acc.getOrElse(key, Seq.empty[String]) ++ Seq(f.functionName)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The order of function calls should be important and be kept. Does the map guarantee that updated map still preserves the insertion order?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. only up to 4 keys, thanks for the nice catch, I will fix this.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To improve the readability, let us avoid using |
||
| } | ||
| .map { case ((subclassName, subclassInstance), subclassFunctions) => | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you're right, even though this is very unlikely. We might also think of a threshold maybe. Which is the right approach according to you?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I think it can only happen at the first or last sub-class functions. Seems the functions might be only included in two (?) sub-classes. Most of functions will be wrapped in one function call. I'm not sure the proper threshold for this, maybe 5?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I will create a field to make this easy to be changed. |
||
| // Adding a new function to each subclass which contains | ||
| // the invocation of all the ones which have been added to | ||
| // that subclass | ||
| val code = s""" | ||
| |private $returnType $func($argString) { | ||
| | ${makeSplitFunction(foldFunctions(subclassFunctions.map(name => | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we don't need
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we do need for methods like
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh. I see. Nvm.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but this are still splits, just bigger. |
||
| s"$name(${arguments.map(_._2).mkString(", ")})")))} | ||
| |} | ||
| """.stripMargin | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. // Adding a new function to each inner class which contains the invocation of all the
// ones which have been added to that inner class. For example,
// private class NestedClass {
// private void apply_862(InternalRow i) { ... }
// private void apply_863(InternalRow i) { ... }
// ...
// private void apply(InternalRow i) {
// apply_862(i);
// apply_863(i);
// ...
// }
// }
val body = foldFunctions(orderedFunctions.map(name => s"$name($argInvocationString)"))
val code =
s"""
|private $returnType $funcName($argDefinitionString) {
| ${makeSplitFunction(body)}
|}
""".stripMargin |
||
| addNewFunctionToClass(func, code, subclassName) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The class to add codes into can be over the threshold too.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So shall we call
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no, this is made intentionally because if we add it to the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't we call them with instance?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we might, but I think this would not be the best option, since in this way we are "reusing" some ConstantPool entries (like
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry, I didn't get completely you last comments, might you elaborate it a bit more? Thanks.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh. I meant the increased inner classes/instances are added into the outerclass's Constant Pool entries. Outerclass should be more likely to be filled by various variables/methods...than inner classes. So I think to add into Constant Pool of inner classes should be less serious issue than outerclass.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is also convincing that nested classes should be less. So maybe this is not a serious issue, although I'm not sure this can be completely safe.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, but we are likely to recreate the same issue the PR is trying to solve in the last
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds reasonable to me. |
||
| s"$subclassInstance.$func" | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Build a separate private function for generating innerClassFunctions? Now, the function |
||
|
|
||
| foldFunctions((outerClassFunctions ++ innerClassFunctions).map( | ||
| name => s"$name(${arguments.map(_._2).mkString(", ")})")) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The same suggestion. Do not inline a complex expression. |
||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -201,6 +201,23 @@ class CodeGenerationSuite extends SparkFunSuite with ExpressionEvalHelper { | |
| } | ||
| } | ||
|
|
||
| test("SPARK-22226: group splitted expressions into one method per nested class") { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Besides the unit test, can you provide an end-to-end case that can trigger this issue too?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have a use case where I faced this problem. And I tried this patch on it. Unfortunately this contains a very complex business logic and I have not been able to reproduce it in a simple one. But if needed, I can try again.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of copying your customer codes, can you making a fake one?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mgaido91 I can reproduce the issue by following test case. You can check it: test("SPARK-22226: too much splitted expressions should not exceed constant pool limit") {
withSQLConf(
(SQLConf.WHOLESTAGE_CODEGEN_ENABLED.key, "false")) {
val colNumber = 1000
val baseDF = spark.range(10).toDF()
val newCols = (1 to colNumber).map { colIndex =>
expr(s"id + $colIndex").as(s"_$colIndex")
}
val input = baseDF.select(newCols: _*)
val aggs = (1 to colNumber).flatMap { colIndex =>
val colName = s"_$colIndex"
Seq(expr(s"stddev($colName)"),
expr(s"stddev_samp($colName)"),
expr(s"stddev_pop($colName)"),
expr(s"variance($colName)"),
expr(s"var_samp($colName)"),
expr(s"var_pop($colName)"),
expr(s"skewness($colName)"),
expr(s"kurtosis($colName)"))
}
input.agg(aggs.head, aggs.tail: _*).collect()
}
}
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thank you very much for your help @viirya ! In my use cases it seemed to be connected to the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was adding it to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @viirya I have a good and a bad news... Thanks to your suggestion I have been able to understand and reproduce the issue. Moreover, I found also another issue which is fixed by this problem and I am adding a UT for that too: in some cases, we might have a And with this PR the problem is fixed. The bad thing is that the UT you provided still fails, but with a different error: actually it is always a Constant Pool limit exceeded exception, but it is in a NestedClass. From my analysis, this is caused by another problem, ie. that we might reference too many fields of the superclass in the NestedClasses. This might be addressed maybe trying to tune the magic number which I brought to 1000k in this PR, but I am pretty sure that it will be also addressed by the ongoing PR for SPARK-18016, since he is trying to reduce the number of variables. Thus I consider this out of scope for this PR.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mgaido91 Do you meant "2: compact primitive declarations into arrays" in SPARK-18016?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mgaido91 Thanks for trying it. Yeah, those expressions like
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @viirya exactly, I meant that. Thank you for your suggestion. You have been very helpful to me. |
||
| val length = 10000 | ||
| val expressions = Seq.fill(length) { | ||
| ToUTCTimestamp( | ||
| Literal.create(Timestamp.valueOf("2017-10-10 00:00:00"), TimestampType), | ||
| Literal.create("PST", StringType)) | ||
| } | ||
| val plan = GenerateMutableProjection.generate(expressions) | ||
| val actual = plan(new GenericInternalRow(length)).toSeq(expressions.map(_.dataType)) | ||
| val expected = Seq.fill(length)( | ||
| DateTimeUtils.fromJavaTimestamp(Timestamp.valueOf("2017-10-10 07:00:00"))) | ||
|
|
||
| if (actual != expected) { | ||
| fail(s"Incorrect Evaluation: expressions: $expressions, actual: $actual, expected: $expected") | ||
| } | ||
| } | ||
|
|
||
| test("test generated safe and unsafe projection") { | ||
| val schema = new StructType(Array( | ||
| StructField("a", StringType, true), | ||
|
|
||
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 saw three ways to represent the same concept. subclass, inner class, nested classes.
How about renaming all of them to inner classes? Could you go over all the code changes in this PR to make them consistent?