Skip to content

[SPARK-23032][SQL][FOLLOW-UP]Add codegenStageId in comment #20419

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
kiszk wants to merge 5 commits into from
Closed

[SPARK-23032][SQL][FOLLOW-UP]Add codegenStageId in comment #20419

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6ee9106
initial commit
kiszk Jan 29, 2018
4a09eac
address review comment
kiszk Jan 29, 2018
f07dc2d
address review comment
kiszk Jan 31, 2018
cb7a16b
address review comment
kiszk Feb 5, 2018
0ad1e72
address review comment
kiszk Feb 13, 2018
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
13 changes: 10 additions & 3 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 @@ -1226,14 +1226,21 @@ class CodegenContext {

/**
* Register a comment and return the corresponding place holder
*
* @param placeholderId a string for a place holder

@rednaxelafx rednaxelafx Feb 13, 2018

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.

Nit: can we rephrase this ScalaDoc a bit, maybe like:

/**
 * ...
 * @param placeholderId an optionally specified identifier for the comment's placeholder. The caller should make sure this identifier is unique within the compilation unit. If this argument is not specified, a fresh identifier will be automatically created and used as the placeholder.
 * ...
 */

* @param force whether to force registering the comments
*/
def registerComment(text: => String): String = {
def registerComment(
text: => String,
placeholderId: String = "",
force: Boolean = false): 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)) {
val name = freshName("c")
if (force ||
SparkEnv.get != null && SparkEnv.get.conf.getBoolean("spark.sql.codegen.comments", false)) {

@cloud-fan cloud-fan Feb 13, 2018

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.

unrelated, but should this be a SQL conf?

@gatorsmile gatorsmile Feb 13, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Now, the question is whether we can use SQLConf.get in this case. If not, we might need to keep it like this

@kiszk kiszk Feb 13, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

IIUC, the answer seems to be No. See this discussion.

val name = if (placeholderId != "") placeholderId else freshName("c")
val comment = if (text.contains("\n") || text.contains("\r")) {
text.split("(\r\n)|\r|\n").mkString("/**\n * ", "\n * ", "\n */")
} else {
Expand Down
4 changes: 3 additions & 1 deletion sql/core/src/main/scala/org/apache/spark/sql/execution/WholeStageCodegenExec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,9 @@ case class WholeStageCodegenExec(child: SparkPlan)(val codegenStageId: Int)

${ctx.registerComment(
s"""Codegend pipeline for stage (id=$codegenStageId)
|${this.treeString.trim}""".stripMargin)}
|${this.treeString.trim}""".stripMargin,
"wsc_codegenPipeline")}
${ctx.registerComment(s"codegenStageId=$codegenStageId", "wsc_codegenStageId", true)}
final class $className extends ${classOf[BufferedRowIterator].getName} {

private Object[] references;
Expand Down