[SPARK-11352][SQL] Escape */ in the generated comments.

yhuai · yhuai · commit 14eadf921132 · 2015-12-01T16:24:14.000-08:00
https://issues.apache.org/jira/browse/SPARK-11352 Author: Yin Huai <yhuai@databricks.com> Closes apache#10072 from yhuai/SPARK-11352. (cherry picked from commit 5872a9d) Signed-off-by: Yin Huai <yhuai@databricks.com>
diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
@@ -95,15 +95,15 @@ abstract class Expression extends TreeNode[Expression] {
     ctx.subExprEliminationExprs.get(this).map { subExprState =>
       // This expression is repeated meaning the code to evaluated has already been added
       // as a function and called in advance. Just use it.
-      val code = s"/* $this */"
+      val code = s"/* ${this.toCommentSafeString} */"
       GeneratedExpressionCode(code, subExprState.isNull, subExprState.value)
     }.getOrElse {
       val isNull = ctx.freshName("isNull")
       val primitive = ctx.freshName("primitive")
       val ve = GeneratedExpressionCode("", isNull, primitive)
       ve.code = genCode(ctx, ve)
       // Add `this` in the comment.
-      ve.copy(s"/* $this */\n" + ve.code.trim)
+      ve.copy(s"/* ${this.toCommentSafeString} */\n" + ve.code.trim)
     }
   }
 
@@ -214,6 +214,12 @@ abstract class Expression extends TreeNode[Expression] {
   }
 
   override def toString: String = prettyName + flatArguments.mkString("(", ",", ")")
+
+  /**
+   * Returns the string representation of this expression that is safe to be put in
+   * code comments of generated code.
+   */
+  protected def toCommentSafeString: String = this.toString.replace("*/", "\\*\\/")
 }
 
 
diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodegenFallback.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodegenFallback.scala
@@ -33,7 +33,7 @@ trait CodegenFallback extends Expression {
     ctx.references += this
     val objectTerm = ctx.freshName("obj")
     s"""
-      /* expression: ${this} */
+      /* expression: ${this.toCommentSafeString} */
       java.lang.Object $objectTerm = expressions[${ctx.references.size - 1}].eval(${ctx.INPUT_ROW});
       boolean ${ev.isNull} = $objectTerm == null;
       ${ctx.javaType(this.dataType)} ${ev.value} = ${ctx.defaultValue(this.dataType)};
diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CodeGenerationSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CodeGenerationSuite.scala
@@ -98,4 +98,13 @@ class CodeGenerationSuite extends SparkFunSuite with ExpressionEvalHelper {
     unsafeRow.getStruct(3, 1).getStruct(0, 2).setInt(1, 4)
     assert(internalRow === internalRow2)
   }
+
+  test("*/ in the data") {
+    // When */ appears in a comment block (i.e. in /**/), code gen will break.
+    // So, in Expression and CodegenFallback, we escape */ to \*\/.
+    checkEvaluation(
+      EqualTo(BoundReference(0, StringType, false), Literal.create("*/", StringType)),
+      true,
+      InternalRow(UTF8String.fromString("*/")))
+  }
 }

Original file line number	Diff line number	Diff line change
`@@ -98,4 +98,13 @@ class CodeGenerationSuite extends SparkFunSuite with ExpressionEvalHelper {`
`98`	`98`	`unsafeRow.getStruct(3, 1).getStruct(0, 2).setInt(1, 4)`
`99`	`99`	`assert(internalRow === internalRow2)`
`100`	`100`	`}`
	`101`	`+`
	`102`	`+ test("*/ in the data") {`
	`103`	`+ // When / appears in a comment block (i.e. in /*/), code gen will break.`
	`104`	`+ // So, in Expression and CodegenFallback, we escape / to \\/.`
	`105`	`+ checkEvaluation(`
	`106`	`+ EqualTo(BoundReference(0, StringType, false), Literal.create("*/", StringType)),`
	`107`	`+ true,`
	`108`	`+ InternalRow(UTF8String.fromString("*/")))`
	`109`	`+ }`
`101`	`110`	`}`