Skip to content

[SPARK-20871][SQL] limit logging of Janino code #18658

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
pjfanning wants to merge 6 commits into from
Closed

[SPARK-20871][SQL] limit logging of Janino code #18658

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
803166c
[SPARK-20871] only log Janino code at debug level
Jul 17, 2017
52b20f3
[SPARK-20871][SQL] log Janino code at info level (but include a max o…
Jul 17, 2017
2997ded
Explicitly reference the maxLines param
Jul 21, 2017
252f8ea
[SPARK-20871][SQL] have just one format function with a default appli…
Jul 21, 2017
c42dc46
[SPARK-20871][SQL] add SQLConf internal property: spark.sql.codegen.l…
Jul 22, 2017
a795db2
[SPARK-20871][SQL] add check on SQLConf internal property: spark.sql.…
Jul 23, 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
12 changes: 10 additions & 2 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 @@ -28,14 +28,22 @@ import java.util.regex.Matcher
object CodeFormatter {
val commentHolder = """\/\*(.+?)\*\/""".r

def format(code: CodeAndComment): String = {
def format(code: CodeAndComment): String = format(code, -1)

def format(code: CodeAndComment, maxLines: Int): String = {
Copy link
Member

@srowen srowen Jul 21, 2017

Choose a reason for hiding this comment

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

Can maxLines just be an optional argument rather than declare two methods? this is an internal method so its signature is OK to change.

Copy link
Member Author

@pjfanning pjfanning Jul 21, 2017

Choose a reason for hiding this comment

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

@srowen I can make that change. Is there any need to scaladoc the maxLines param? Or is the function definition easy enough to follow?

def format(code: CodeAndComment, maxLines: Int = -1): String

val formatter = new CodeFormatter
code.body.split("\n").foreach { line =>
val lines = code.body.split("\n")
val needToTruncate = maxLines >= 0 && lines.length > maxLines
val filteredLines = if (needToTruncate) lines.take(maxLines) else lines
filteredLines.foreach { line =>
val commentReplaced = commentHolder.replaceAllIn(
line.trim,
m => code.comment.get(m.group(1)).map(Matcher.quoteReplacement).getOrElse(m.group(0)))
formatter.addLine(commentReplaced)
}
if (needToTruncate) {
formatter.addLine(s"[truncated to $maxLines lines (total lines is ${lines.length})]")
}
formatter.result()
}

Expand Down
10 changes: 5 additions & 5 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 @@ -1037,25 +1037,25 @@ object CodeGenerator extends Logging {
))
evaluator.setExtendedClass(classOf[GeneratedClass])

lazy val formatted = CodeFormatter.format(code)

logDebug({
// Only add extra debugging info to byte code when we are going to print the source code.
evaluator.setDebuggingInformation(true, true, false)
s"\n$formatted"
s"\n${CodeFormatter.format(code)}"
Copy link
Contributor

@ash211 ash211 Jul 17, 2017

Choose a reason for hiding this comment

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

I'd suggest dropping the string concatenation with \n here -- it requires an additional copy of the code to be held in-memory and for errors where the code is too long, this causes unnecessary additional pressure on the heap

Copy link
Member Author

@pjfanning pjfanning Jul 17, 2017

Choose a reason for hiding this comment

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

@ash211 the logging will be less user friendly without the \n and this is debug logging - I am working on an enhancement to the CodeFormatter class for the info level logging discussed earlier with @marmbrus that would allow a max number of lines to be specified

})

try {
evaluator.cook("generated.java", code.body)
recordCompilationStats(evaluator)
} catch {
case e: JaninoRuntimeException =>
val msg = s"failed to compile: $e\n$formatted"
val msg = s"failed to compile: $e"
logError(msg, e)
logInfo(s"\n${CodeFormatter.format(code, 1000)}")
Copy link
Member

@gatorsmile gatorsmile Jul 21, 2017

Choose a reason for hiding this comment

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

1000 -> maxLines = 1000

throw new JaninoRuntimeException(msg, e)
case e: CompileException =>
val msg = s"failed to compile: $e\n$formatted"
val msg = s"failed to compile: $e"
logError(msg, e)
logInfo(s"\n${CodeFormatter.format(code, 1000)}")
Copy link
Member

@gatorsmile gatorsmile Jul 21, 2017

Choose a reason for hiding this comment

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

The same here

throw new CompileException(msg, e.getLocation)
}
evaluator.getClazz().newInstance().asInstanceOf[GeneratedClass]
Expand Down
26 changes: 21 additions & 5 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 @@ -20,18 +20,18 @@ package org.apache.spark.sql.catalyst.expressions.codegen
import org.apache.spark.SparkFunSuite
import org.apache.spark.sql.catalyst.util._


class CodeFormatterSuite extends SparkFunSuite {

def testCase(name: String)(
input: String, comment: Map[String, String] = Map.empty)(expected: String): Unit = {
def testCase(name: String)(input: String,
comment: Map[String, String] = Map.empty, maxLines: Int = -1)(expected: String): Unit = {
test(name) {
val sourceCode = new CodeAndComment(input.trim, comment)
if (CodeFormatter.format(sourceCode).trim !== expected.trim) {
if (CodeFormatter.format(sourceCode, maxLines).trim !== expected.trim) {
fail(
s"""
|== FAIL: Formatted code doesn't match ===
|${sideBySide(CodeFormatter.format(sourceCode).trim, expected.trim).mkString("\n")}
|${sideBySide(CodeFormatter.format(sourceCode, maxLines).trim,
expected.trim).mkString("\n")}
""".stripMargin)
}
}
Expand Down Expand Up @@ -129,6 +129,22 @@ class CodeFormatterSuite extends SparkFunSuite {
""".stripMargin
}

testCase("function calls with maxLines=2") (
"""
|foo(
|a,
|b,
|c)
""".stripMargin,
maxLines = 2
) {
"""
|/* 001 */ foo(
|/* 002 */ a,
|/* 003 */ [truncated to 2 lines (total lines is 4)]
""".stripMargin
}

testCase("single line comments") {
"""
|// This is a comment about class A { { { ( (
Expand Down