Skip to content

[SPARK-13135][SQL] Don't print expressions recursively in generated code #13192

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
wants to merge 1 commit into from
Closed

[SPARK-13135][SQL] Don't print expressions recursively in generated code #13192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 24 additions & 3 deletions ...lyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeFormatter.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ object CodeFormatter {
var lastLine: String = "dummy"
input.split('\n').foreach { l =>
val line = l.trim()
val skip = line == "" && (lastLine == "" || lastLine.endsWith("{"))
val skip = line == "" && (lastLine == "" || lastLine.endsWith("{") || lastLine.endsWith("*/"))
if (!skip) {
code.append(line)
code.append("\n")
Expand All @@ -49,6 +49,24 @@ object CodeFormatter {
}
code.result()
}

def stripOverlappingComments(codeAndComment: CodeAndComment): CodeAndComment = {
val code = new StringBuilder
val map = codeAndComment.comment
var lastLine: String = "dummy"
codeAndComment.body.split('\n').foreach { l =>
val line = l.trim()
val skip = lastLine.startsWith("/*") && lastLine.endsWith("*/") &&
Copy link
Contributor

@cloud-fan cloud-fan May 24, 2016

Choose a reason for hiding this comment

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

are we assuming the comment holder will always take an entire line?

line.startsWith("/*") && line.endsWith("*/") &&
map(lastLine).substring(3).contains(map(line).substring(3))
Copy link
Contributor

@davies davies May 24, 2016

Choose a reason for hiding this comment

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

Have you check that this actually work? I think we have placeholders here so will not find any duplicated comments to skip.

Copy link
Contributor

@davies davies May 24, 2016

Choose a reason for hiding this comment

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

Oh, it should work, I missed the map. Will it have performance issue?

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun May 24, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's okay for the performance.

  • This function is used for at every CodeAndComment creation once.
  • It scans codeAndComment.body once.
  • Map lookup occurs on each line at most twice. Also, it does not cost much in this case.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun May 24, 2016

Choose a reason for hiding this comment

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

Also, the skip condition is checking only consecutive comments lines.
If there is something to do more, please let me know, @davies .

Copy link
Contributor

@davies davies May 24, 2016

Choose a reason for hiding this comment

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

SGTM

if (!skip) {
code.append(line)
code.append("\n")
}
lastLine = line
}
new CodeAndComment(code.result().trim(), map)
}
}

private class CodeFormatter {
Expand Down Expand Up @@ -100,8 +118,11 @@ private class CodeFormatter {
indentString
}
code.append(f"/* ${currentLine}%03d */ ")
code.append(thisLineIndent)
code.append(line)
if (line.trim().length > 0) {
Copy link
Contributor

@cloud-fan cloud-fan May 24, 2016

Choose a reason for hiding this comment

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

Can you explain a bit more about this logic?

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun May 24, 2016

Choose a reason for hiding this comment

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

Sure! This if makes blank lines print only line numbers, e.g.,
Before

"/* 001 */ <space><space><space><space> ..."

After

"/* 001 */ "

As you see, the redundant spaces for indentation go away.

code.append(thisLineIndent)
if (inCommentBlock && line.startsWith("*") || line.startsWith("*/")) code.append(" ")
code.append(line)
}
code.append("\n")
indentLevel = newIndentLevel
indentString = " " * (indentSize * newIndentLevel)
Expand Down
3 changes: 2 additions & 1 deletion ...n/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateMutableProjection.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,8 @@ object GenerateMutableProjection extends CodeGenerator[Seq[Expression], MutableP
}
"""

val code = new CodeAndComment(codeBody, ctx.getPlaceHolderToComments())
val code = CodeFormatter.stripOverlappingComments(
new CodeAndComment(codeBody, ctx.getPlaceHolderToComments()))
logDebug(s"code for ${expressions.mkString(",")}:\n${CodeFormatter.format(code)}")

val c = CodeGenerator.compile(code)
Expand Down
3 changes: 2 additions & 1 deletion ...t/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateOrdering.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,8 @@ object GenerateOrdering extends CodeGenerator[Seq[SortOrder], Ordering[InternalR
}
}"""

val code = new CodeAndComment(codeBody, ctx.getPlaceHolderToComments())
val code = CodeFormatter.stripOverlappingComments(
new CodeAndComment(codeBody, ctx.getPlaceHolderToComments()))
logDebug(s"Generated Ordering by ${ordering.mkString(",")}:\n${CodeFormatter.format(code)}")

CodeGenerator.compile(code).generate(ctx.references.toArray).asInstanceOf[BaseOrdering]
Expand Down
3 changes: 2 additions & 1 deletion .../src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GeneratePredicate.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ object GeneratePredicate extends CodeGenerator[Expression, (InternalRow) => Bool
}
}"""

val code = new CodeAndComment(codeBody, ctx.getPlaceHolderToComments())
val code = CodeFormatter.stripOverlappingComments(
new CodeAndComment(codeBody, ctx.getPlaceHolderToComments()))
logDebug(s"Generated predicate '$predicate':\n${CodeFormatter.format(code)}")

val p = CodeGenerator.compile(code).generate(ctx.references.toArray).asInstanceOf[Predicate]
Expand Down
3 changes: 2 additions & 1 deletion ...main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateSafeProjection.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,8 @@ object GenerateSafeProjection extends CodeGenerator[Seq[Expression], Projection]
}
"""

val code = new CodeAndComment(codeBody, ctx.getPlaceHolderToComments())
val code = CodeFormatter.stripOverlappingComments(
new CodeAndComment(codeBody, ctx.getPlaceHolderToComments()))
logDebug(s"code for ${expressions.mkString(",")}:\n${CodeFormatter.format(code)}")

val c = CodeGenerator.compile(code)
Expand Down
3 changes: 2 additions & 1 deletion ...in/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateUnsafeProjection.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,8 @@ object GenerateUnsafeProjection extends CodeGenerator[Seq[Expression], UnsafePro
}
"""

val code = new CodeAndComment(codeBody, ctx.getPlaceHolderToComments())
val code = CodeFormatter.stripOverlappingComments(
new CodeAndComment(codeBody, ctx.getPlaceHolderToComments()))
logDebug(s"code for ${expressions.mkString(",")}:\n${CodeFormatter.format(code)}")

val c = CodeGenerator.compile(code)
Expand Down
2 changes: 1 addition & 1 deletion ...ain/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateUnsafeRowJoiner.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ object GenerateUnsafeRowJoiner extends CodeGenerator[(StructType, StructType), U
| }
|}
""".stripMargin
val code = new CodeAndComment(codeBody, Map.empty)
val code = CodeFormatter.stripOverlappingComments(new CodeAndComment(codeBody, Map.empty))
logDebug(s"SpecificUnsafeRowJoiner($schema1, $schema2):\n${CodeFormatter.format(code)}")

val c = CodeGenerator.compile(code)
Expand Down
43 changes: 43 additions & 0 deletions ...src/test/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeFormatterSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,22 @@ class CodeFormatterSuite extends SparkFunSuite {
}
}

test("removing overlapping comments") {
val code = new CodeAndComment(
"""/*project_c4*/
|/*project_c3*/
|/*project_c2*/
""".stripMargin,
Map(
"/*project_c4*/" -> "// (((input[0, bigint, false] + 1) + 2) + 3))",
Copy link
Contributor

@cloud-fan cloud-fan May 24, 2016

Choose a reason for hiding this comment

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

what if we have multi-line comment? Does it still work?

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun May 24, 2016

Choose a reason for hiding this comment

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

Hi, @cloud-fan .
This is only targeting for those overlapping expressions.
It's not for multi-line comments.
Could you give me some kind of generated overlapping multi-line comment?
If then, I can grasp your concerns more fast.

Copy link
Contributor

@cloud-fan cloud-fan May 24, 2016

Choose a reason for hiding this comment

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

seems no such case, then it's fine

"/*project_c3*/" -> "// ((input[0, bigint, false] + 1) + 2)",
"/*project_c2*/" -> "// (input[0, bigint, false] + 1)"
))

val reducedCode = CodeFormatter.stripOverlappingComments(code)
assert(reducedCode.body === "/*project_c4*/")
}

testCase("basic example") {
"""class A {
|blahblah;
Expand Down Expand Up @@ -147,4 +163,31 @@ class CodeFormatterSuite extends SparkFunSuite {
|/* 006 */ }
""".stripMargin
}

// scalastyle:off whitespace.end.of.line
testCase("reduce empty lines") {
CodeFormatter.stripExtraNewLines(
"""class A {
|
|
| /*** comment1 */
|
| class body;
|
|
| if (c) {duh;}
| else {boo;}
|}""".stripMargin)
}{
"""
|/* 001 */ class A {
|/* 002 */ /*** comment1 */
|/* 003 */ class body;
|/* 004 */
|/* 005 */ if (c) {duh;}
|/* 006 */ else {boo;}
|/* 007 */ }
""".stripMargin
}
// scalastyle:on whitespace.end.of.line
}
4 changes: 2 additions & 2 deletions 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 @@ -333,8 +333,8 @@ case class WholeStageCodegenExec(child: SparkPlan) extends UnaryExecNode with Co
""".trim

// try to compile, helpful for debug
val cleanedSource =
new CodeAndComment(CodeFormatter.stripExtraNewLines(source), ctx.getPlaceHolderToComments())
val cleanedSource = CodeFormatter.stripOverlappingComments(
new CodeAndComment(CodeFormatter.stripExtraNewLines(source), ctx.getPlaceHolderToComments()))

logDebug(s"\n${CodeFormatter.format(cleanedSource)}")
CodeGenerator.compile(cleanedSource)
Expand Down
3 changes: 2 additions & 1 deletion sql/core/src/main/scala/org/apache/spark/sql/execution/columnar/GenerateColumnAccessor.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,8 @@ object GenerateColumnAccessor extends CodeGenerator[Seq[DataType], ColumnarItera
}
}"""

val code = new CodeAndComment(codeBody, ctx.getPlaceHolderToComments())
val code = CodeFormatter.stripOverlappingComments(
new CodeAndComment(codeBody, ctx.getPlaceHolderToComments()))
logDebug(s"Generated ColumnarIterator:\n${CodeFormatter.format(code)}")

CodeGenerator.compile(code).generate(Array.empty).asInstanceOf[ColumnarIterator]
Expand Down