Skip to content

[SPARK-15680][SQL] Disable comments in generated code in order to avoid perf. issues#13421

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
JoshRosen wants to merge 2 commits into from
Closed

[SPARK-15680][SQL] Disable comments in generated code in order to avoid perf. issues #13421

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b6a190
Use flag to disable comments in generated code.
JoshRosen May 31, 2016
db46241
Fix NPE in tests.
JoshRosen May 31, 2016
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
23 changes: 16 additions & 7 deletions ...lyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -24,6 +24,7 @@ import scala.language.existentials
import com.google.common.cache.{CacheBuilder, CacheLoader}
import org.codehaus.janino.ClassBodyEvaluator

import org.apache.spark.SparkEnv
import org.apache.spark.internal.Logging
import org.apache.spark.sql.catalyst.InternalRow
import org.apache.spark.sql.catalyst.expressions._
Expand DownExpand Up@@ -720,15 +721,23 @@ class CodegenContext {
/**
* Register a comment and return the corresponding place holder
*/
def registerComment(text: String): String = {
val name = freshName("c")
val comment = if (text.contains("\n") || text.contains("\r")) {
text.split("(\r\n)|\r|\n").mkString("/**\n * ", "\n * ", "\n */")
def registerComment(text: => String): String = {
// By default, disable comments in generated code because computing the comments themselves can
// be extremely expensive in certain cases, such as deeply-nested expressions which operate over
// inputs with wide schemas. For more details on the performance issues that motivated this
// flat, see SPARK-15680.
if (SparkEnv.get != null && SparkEnv.get.conf.getBoolean("spark.sql.codegen.comments", false)) {

@rxinrxinMay 31, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hm this is not a runtime config -- can we use a runtime sqlconf?

@rxinrxinMay 31, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

basically the change would be larger, but i think immutable configs like this make this feature pretty much dead.

@JoshRosenJoshRosenMay 31, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The problem with using a runtime SQLConf (actually a CatalystConf (to avoid a circular dependency)) is that we'd need to thread that configuration into the implementations of the CodeGenerator.generate method and that method has 60+ call sites, many of which do not have a readily-accessible configuration instance.

If we had some thread-local mechanism for implicitly obtaining these configurations then this would be easy, but for now I don't see a simple way to thread this configuration without changing 20+ files.

@daviesdaviesMay 31, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We already have a thread-local SQLContext (SQLContext.getActive()), it could be used here.

In BroadcastExchangeExec and prepare of subquery (in SparkPlan), we did not set the current SQLContext as active one, we should also fix that.

@JoshRosenJoshRosenMay 31, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Can we access SQLContext in the catalyst package, though?

@daviesdaviesJun 1, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's sad ...

val name = freshName("c")
val comment = if (text.contains("\n") || text.contains("\r")) {
text.split("(\r\n)|\r|\n").mkString("/**\n * ", "\n * ", "\n */")
} else {
s"// $text"
}
placeHolderToComments += (name -> comment)
s"/*$name*/"
} else {
s"// $text"
""
}
placeHolderToComments += (name -> comment)
s"/*$name*/"
}
}

Expand Down