You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by sarutak <gi...@git.apache.org> on 2016/05/08 00:34:17 UTC

[GitHub] spark pull request: [SPARK-15205][SQL][WIP] Codegen can compile mo...

GitHub user sarutak opened a pull request:

    https://github.com/apache/spark/pull/12979

    [SPARK-15205][SQL][WIP] Codegen can compile more than twice for the same source code

    ## What changes were proposed in this pull request?
    
    Sometimes, we have generated codes they are equal except for comments.
    
    One example is here.
    {code}
    val df = sc.parallelize(1 to 10).toDF
    df.selectExpr("value + 1").show // query1
    df.selectExpr("value + 2").show // query2
    {code}
    
    The following code is one of generated code when query1 above is executed.
    
    {code}
    /* 001 */ 
    /* 002 */ public java.lang.Object generate(Object[] references) {
    /* 003 */   return new SpecificSafeProjection(references);
    /* 004 */ }
    /* 005 */ 
    /* 006 */ class SpecificSafeProjection extends org.apache.spark.sql.catalyst.expressions.codegen.BaseProjection {
    /* 007 */   
    /* 008 */   private Object[] references;
    /* 009 */   private MutableRow mutableRow;
    /* 010 */   private Object[] values;
    /* 011 */   private org.apache.spark.sql.types.StructType schema;
    /* 012 */   
    /* 013 */   
    /* 014 */   public SpecificSafeProjection(Object[] references) {
    /* 015 */     this.references = references;
    /* 016 */     mutableRow = (MutableRow) references[references.length - 1];
    /* 017 */     
    /* 018 */     this.schema = (org.apache.spark.sql.types.StructType) references[0];
    /* 019 */   }
    /* 020 */   
    /* 021 */   public java.lang.Object apply(java.lang.Object _i) {
    /* 022 */     InternalRow i = (InternalRow) _i;
    /* 023 */     /* createexternalrow(if (isnull(input[0, int])) null else input[0, int], StructField((value + 1),IntegerType,false)) */
    /* 024 */     values = new Object[1];
    /* 025 */     /* if (isnull(input[0, int])) null else input[0, int] */
    /* 026 */     /* isnull(input[0, int]) */
    /* 027 */     /* input[0, int] */
    /* 028 */     int value3 = i.getInt(0);
    /* 029 */     boolean isNull1 = false;
    /* 030 */     int value1 = -1;
    /* 031 */     if (!false && false) {
    /* 032 */       /* null */
    /* 033 */       final int value4 = -1;
    /* 034 */       isNull1 = true;
    /* 035 */       value1 = value4;
    /* 036 */     } else {
    /* 037 */       /* input[0, int] */
    /* 038 */       int value5 = i.getInt(0);
    /* 039 */       isNull1 = false;
    /* 040 */       value1 = value5;
    /* 041 */     }
    /* 042 */     if (isNull1) {
    /* 043 */       values[0] = null;
    /* 044 */     } else {
    /* 045 */       values[0] = value1;
    /* 046 */     }
    /* 047 */     
    /* 048 */     final org.apache.spark.sql.Row value = new org.apache.spark.sql.catalyst.expressions.GenericRowWithSchema(values, this.schema);
    /* 049 */     if (false) {
    /* 050 */       mutableRow.setNullAt(0);
    /* 051 */     } else {
    /* 052 */       
    /* 053 */       mutableRow.update(0, value);
    /* 054 */     }
    /* 055 */     
    /* 056 */     return mutableRow;
    /* 057 */   }
    /* 058 */ }
    /* 059 */ 
    {code}
    
    On the other hand, the following code is for query2.
    
    {code}
    /* 001 */ 
    /* 002 */ public java.lang.Object generate(Object[] references) {
    /* 003 */   return new SpecificSafeProjection(references);
    /* 004 */ }
    /* 005 */ 
    /* 006 */ class SpecificSafeProjection extends org.apache.spark.sql.catalyst.expressions.codegen.BaseProjection {
    /* 007 */   
    /* 008 */   private Object[] references;
    /* 009 */   private MutableRow mutableRow;
    /* 010 */   private Object[] values;
    /* 011 */   private org.apache.spark.sql.types.StructType schema;
    /* 012 */   
    /* 013 */   
    /* 014 */   public SpecificSafeProjection(Object[] references) {
    /* 015 */     this.references = references;
    /* 016 */     mutableRow = (MutableRow) references[references.length - 1];
    /* 017 */     
    /* 018 */     this.schema = (org.apache.spark.sql.types.StructType) references[0];
    /* 019 */   }
    /* 020 */   
    /* 021 */   public java.lang.Object apply(java.lang.Object _i) {
    /* 022 */     InternalRow i = (InternalRow) _i;
    /* 023 */     /* createexternalrow(if (isnull(input[0, int])) null else input[0, int], StructField((value + 2),IntegerType,false)) */
    /* 024 */     values = new Object[1];
    /* 025 */     /* if (isnull(input[0, int])) null else input[0, int] */
    /* 026 */     /* isnull(input[0, int]) */
    /* 027 */     /* input[0, int] */
    /* 028 */     int value3 = i.getInt(0);
    /* 029 */     boolean isNull1 = false;
    /* 030 */     int value1 = -1;
    /* 031 */     if (!false && false) {
    /* 032 */       /* null */
    /* 033 */       final int value4 = -1;
    /* 034 */       isNull1 = true;
    /* 035 */       value1 = value4;
    /* 036 */     } else {
    /* 037 */       /* input[0, int] */
    /* 038 */       int value5 = i.getInt(0);
    /* 039 */       isNull1 = false;
    /* 040 */       value1 = value5;
    /* 041 */     }
    /* 042 */     if (isNull1) {
    /* 043 */       values[0] = null;
    /* 044 */     } else {
    /* 045 */       values[0] = value1;
    /* 046 */     }
    /* 047 */     
    /* 048 */     final org.apache.spark.sql.Row value = new org.apache.spark.sql.catalyst.expressions.GenericRowWithSchema(values, this.schema);
    /* 049 */     if (false) {
    /* 050 */       mutableRow.setNullAt(0);
    /* 051 */     } else {
    /* 052 */       
    /* 053 */       mutableRow.update(0, value);
    /* 054 */     }
    /* 055 */     
    /* 056 */     return mutableRow;
    /* 057 */   }
    /* 058 */ }
    /* 059 */ 
    {code}
    
    As you can notice, those two generated codes are essentially equal but not equal as String objects so they will be compiled each.
    
    
    In this PR, I introduced place holder for comments like /*{comment_placeholder1}*/.
    The code to be compiled has only  comment-style place holder.
    When logging the generated code, place holders are replaced with corresponding actual comments.
    
    ## How was this patch tested?
    
    Currently I mark this PR as WIP and I'll add test cases in this PR.
    


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/sarutak/spark SPARK-15205

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/spark/pull/12979.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #12979
    
----
commit 766f5c64f93163ee1b8b7ea8b1ef986f9a731733
Author: Kousuke Saruta <sa...@oss.nttdata.co.jp>
Date:   2016-05-08T00:27:41Z

    Fixed duplicated generated code issue

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63919915
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
    @@ -740,6 +813,9 @@ abstract class CodeGenerator[InType <: AnyRef, OutType <: AnyRef] extends Loggin
       /** Binds an input expression to a given input schema */
       protected def bind(in: InType, inputSchema: Seq[Attribute]): InType
     
    +  /** Generate the code to be compiled represented as string */
    +  def generateCodeAndComments(ctx: CodegenContext, in: InType): CodeAndComment
    --- End diff --
    
    We don't need to test it for every generated code, may not worth it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-219932274
  
    @sarutak It's expected to compile twice on two different queries, it does not worth to optimize this corner case (ideally it should generate different source code even without comments).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63916876
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
    @@ -706,6 +711,60 @@ class CodegenContext {
         if (doSubexpressionElimination) subexpressionElimination(expressions)
         expressions.map(e => e.genCode(this))
       }
    +
    +  /**
    +   * get a map of the pair of a place holder and a corresponding comment
    +   */
    +  def getPlaceHolderToComments(): collection.Map[String, String] = {
    +    placeHolderToComments
    +  }
    +
    +  /**
    +   * Returns the string representation of this expression that is safe to be put in
    +   * code comments of generated code. The length is capped at 128 characters.
    +   */
    +  private[this] def toCommentSafeString(str: String): String = {
    +    val len = math.min(str.length, 128)
    +    val suffix = if (str.length > len) "..." else ""
    +
    +    // Unicode literals, like \u0022, should be escaped before
    +    // they are put in code comment to avoid codegen breaking.
    +    // To escape them, single "\" should be prepended to a series of "\" just before "u"
    +    // only when the number of "\" is odd.
    +    // For example, \u0022 should become to \\u0022
    +    // but \\u0022 should not become to \\\u0022 because the first backslash escapes the second one,
    +    // and \u0022 will remain, means not escaped.
    +    // Otherwise, the runtime Java compiler will fail to compile or code injection can be allowed.
    +    // For details, see SPARK-15165.
    +    str.substring(0, len).replace("*/", "*\\/")
    +      .replaceAll("(^|[^\\\\])(\\\\(\\\\\\\\)*u)", "$1\\\\$2") + suffix
    --- End diff --
    
    Maybe we don't need this anymore.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220538733
  
    **[Test build #58945 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/58945/consoleFull)** for PR 12979 at commit [`6ebfa10`](https://github.com/apache/spark/commit/6ebfa10d63a2234dae4e567f06279f5e7feb3df9).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15205][SQL][WIP] Codegen can compile mo...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-217683640
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/58079/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63762115
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateMutableProjection.scala ---
    @@ -133,7 +133,9 @@ object GenerateMutableProjection extends CodeGenerator[Seq[Expression], MutableP
           }
         """
     
    -    logDebug(s"code for ${expressions.mkString(",")}:\n${CodeFormatter.format(code)}")
    --- End diff --
    
    Sorry It's not clear for me why this line does not need to change.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220320973
  
    **[Test build #58867 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/58867/consoleFull)** for PR 12979 at commit [`d29c7e4`](https://github.com/apache/spark/commit/d29c7e4a535c264e49be598a383cce2feaa86c40).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220526078
  
    **[Test build #58944 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/58944/consoleFull)** for PR 12979 at commit [`3c49567`](https://github.com/apache/spark/commit/3c495670716ecf63d21110f7c7ee93500051d26a).
     * This patch **fails Spark unit tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220697757
  
    **[Test build #59004 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59004/consoleFull)** for PR 12979 at commit [`9c7ff14`](https://github.com/apache/spark/commit/9c7ff143e6b635d34da4250e35dd770efff01b5d).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220568432
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/58946/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220342761
  
    **[Test build #58867 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/58867/consoleFull)** for PR 12979 at commit [`d29c7e4`](https://github.com/apache/spark/commit/d29c7e4a535c264e49be598a383cce2feaa86c40).
     * This patch **fails Spark unit tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63943856
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
    @@ -706,6 +711,35 @@ class CodegenContext {
         if (doSubexpressionElimination) subexpressionElimination(expressions)
         expressions.map(e => e.genCode(this))
       }
    +
    +  /**
    +   * get a map of the pair of a place holder and a corresponding comment
    +   */
    +  def getPlaceHolderToComments(): collection.Map[String, String] = placeHolderToComments
    +
    +  /**
    +   * Register a multi-line comment and return the corresponding place holder
    +   */
    +  def registerMultilineComment(text: String): String = {
    +    val placeHolder = s"/*${freshName("comment_placeholder")}*/"
    +    val comment = text.split("(\r\n)|\r|\n").mkString("/**\n * ", "\n * ", "\n */")
    +    placeHolderToComments += (placeHolder -> comment)
    +    placeHolder
    +  }
    +
    +  /**
    +   * Register a comment and return the corresponding place holder
    +   */
    +  def registerComment(text: String): String = {
    +    if (text.contains("\n") || text.contains("\r")) {
    +      registerMultilineComment(text)
    +    } else {
    +      val placeHolder = s"/*${freshName("comment_placeholder")}*/"
    --- End diff --
    
    /*comment_placeholder*/ -> /*c*/
    
    This is read by human, could be shorter.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220321548
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/58866/
    Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63920473
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
    @@ -706,6 +711,60 @@ class CodegenContext {
         if (doSubexpressionElimination) subexpressionElimination(expressions)
         expressions.map(e => e.genCode(this))
       }
    +
    +  /**
    +   * get a map of the pair of a place holder and a corresponding comment
    +   */
    +  def getPlaceHolderToComments(): collection.Map[String, String] = {
    +    placeHolderToComments
    +  }
    +
    +  /**
    +   * Returns the string representation of this expression that is safe to be put in
    +   * code comments of generated code. The length is capped at 128 characters.
    +   */
    +  private[this] def toCommentSafeString(str: String): String = {
    +    val len = math.min(str.length, 128)
    +    val suffix = if (str.length > len) "..." else ""
    +
    +    // Unicode literals, like \u0022, should be escaped before
    +    // they are put in code comment to avoid codegen breaking.
    +    // To escape them, single "\" should be prepended to a series of "\" just before "u"
    +    // only when the number of "\" is odd.
    +    // For example, \u0022 should become to \\u0022
    +    // but \\u0022 should not become to \\\u0022 because the first backslash escapes the second one,
    +    // and \u0022 will remain, means not escaped.
    +    // Otherwise, the runtime Java compiler will fail to compile or code injection can be allowed.
    +    // For details, see SPARK-15165.
    +    str.substring(0, len).replace("*/", "*\\/")
    +      .replaceAll("(^|[^\\\\])(\\\\(\\\\\\\\)*u)", "$1\\\\$2") + suffix
    --- End diff --
    
    sgtm


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63762638
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateMutableProjection.scala ---
    @@ -133,7 +133,9 @@ object GenerateMutableProjection extends CodeGenerator[Seq[Expression], MutableP
           }
         """
     
    -    logDebug(s"code for ${expressions.mkString(",")}:\n${CodeFormatter.format(code)}")
    --- End diff --
    
    format() could accept CodeAndComment now, and code is CodeAndComment


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220675952
  
    O.K, I'll do it. Need another PR for `branch-1.5` too?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220526125
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/58944/
    Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220697999
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/59004/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63915953
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
    @@ -717,6 +776,20 @@ abstract class GeneratedClass {
     }
     
     /**
    + * A wrapper for the source code to be compiled by [[CodeGenerator]].
    + */
    +class CodeAndComment(val body: String, val comment: collection.Map[String, String])
    +  extends Serializable {
    +  override def equals(that: Any): Boolean = that match {
    +    case t: CodeAndComment if t eq null => false
    --- End diff --
    
    t will not be null if matched


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220638840
  
    **[Test build #59001 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59001/consoleFull)** for PR 12979 at commit [`4029d44`](https://github.com/apache/spark/commit/4029d44fee7a566c323cfa91641037493d8ee687).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220378688
  
    **[Test build #58871 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/58871/consoleFull)** for PR 12979 at commit [`6a7cac4`](https://github.com/apache/spark/commit/6a7cac4388e8baa5339fce2f0e9ad22797406fcc).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `      |/* 002 */  class A `


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63917072
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
    @@ -740,6 +813,9 @@ abstract class CodeGenerator[InType <: AnyRef, OutType <: AnyRef] extends Loggin
       /** Binds an input expression to a given input schema */
       protected def bind(in: InType, inputSchema: Seq[Attribute]): InType
     
    +  /** Generate the code to be compiled represented as string */
    +  def generateCodeAndComments(ctx: CodegenContext, in: InType): CodeAndComment
    --- End diff --
    
    Why added this? It seems to me that this required more unnecessary changes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15205][SQL][WIP] Codegen can compile th...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-218584628
  
    Merged build finished. Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15205][SQL][WIP] Codegen can compile th...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-218584633
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/58394/
    Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220350425
  
    **[Test build #58871 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/58871/consoleFull)** for PR 12979 at commit [`6a7cac4`](https://github.com/apache/spark/commit/6a7cac4388e8baa5339fce2f0e9ad22797406fcc).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220675439
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/59008/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15205][SQL][WIP] Codegen can compile mo...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-217683613
  
    **[Test build #58079 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/58079/consoleFull)** for PR 12979 at commit [`766f5c6`](https://github.com/apache/spark/commit/766f5c64f93163ee1b8b7ea8b1ef986f9a731733).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `class SourceCode(val body: String, val comment: Map[String, String]) extends Serializable `


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63744465
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/WholeStageCodegenExec.scala ---
    @@ -333,7 +333,11 @@ case class WholeStageCodegenExec(child: SparkPlan) extends UnaryExecNode with Co
           """.trim
     
         // try to compile, helpful for debug
    -    val cleanedSource = CodeFormatter.stripExtraNewLines(source)
    +    val cleanedSource =
    --- End diff --
    
    Could you also update the comment for plan? see L310


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/spark/pull/12979


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63744230
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateMutableProjection.scala ---
    @@ -133,7 +133,9 @@ object GenerateMutableProjection extends CodeGenerator[Seq[Expression], MutableP
           }
         """
     
    -    logDebug(s"code for ${expressions.mkString(",")}:\n${CodeFormatter.format(code)}")
    --- End diff --
    
    this line does not need to change


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220538992
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220664994
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220429840
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/58880/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220342987
  
    Merged build finished. Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220677426
  
    Maybe 1.5 and 1.6 could share the same PR (if no much conflicts)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15205][SQL][WIP] Codegen can compile th...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-218560075
  
    **[Test build #58394 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/58394/consoleFull)** for PR 12979 at commit [`60b8e0b`](https://github.com/apache/spark/commit/60b8e0b9c0ca94fe2ec46c9ab2c33664e2693269).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220675619
  
    @sarutak Could you create another PR for 1.6? (If we have not fix the security bug in 1.6)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220675436
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15205][SQL][WIP] Codegen can compile th...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-219930465
  
    **[Test build #58746 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/58746/consoleFull)** for PR 12979 at commit [`4cb5ed3`](https://github.com/apache/spark/commit/4cb5ed3ade800b87561c10ac3bb1dd6f84ba8a0a).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63743568
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
    @@ -200,6 +201,11 @@ class CodegenContext {
       var freshNamePrefix = ""
     
       /**
    +   * The map from a place holder to a corresponding comment
    +   */
    +  private val placeHolderToCommentMap = new mutable.HashMap[String, String]
    --- End diff --
    
    placeHolderToComments


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220664995
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/59003/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220643128
  
    **[Test build #59004 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59004/consoleFull)** for PR 12979 at commit [`9c7ff14`](https://github.com/apache/spark/commit/9c7ff143e6b635d34da4250e35dd770efff01b5d).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220526335
  
    retest this please.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220096845
  
    These place holders are good for safety, could you update the PR title and description?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220319710
  
    **[Test build #58866 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/58866/consoleFull)** for PR 12979 at commit [`b7e7b3d`](https://github.com/apache/spark/commit/b7e7b3dcb1e7802b403d9727ad04253b891533c6).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63919385
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
    @@ -717,6 +776,20 @@ abstract class GeneratedClass {
     }
     
     /**
    + * A wrapper for the source code to be compiled by [[CodeGenerator]].
    + */
    +class CodeAndComment(val body: String, val comment: collection.Map[String, String])
    +  extends Serializable {
    +  override def equals(that: Any): Boolean = that match {
    +    case t: CodeAndComment if t eq null => false
    --- End diff --
    
    Ah, right. Thanks.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63743334
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala ---
    @@ -97,15 +97,20 @@ abstract class Expression extends TreeNode[Expression] {
         ctx.subExprEliminationExprs.get(this).map { subExprState =>
           // This expression is repeated which means that the code to evaluate it has already been added
           // as a function before. In that case, we just re-use it.
    -      val code = s"/* ${toCommentSafeString(this.toString)} */"
    -      ExprCode(code, subExprState.isNull, subExprState.value)
    +      val placeHolder = s"/*{${ctx.freshName("comment_placeholder")}}*/"
    --- End diff --
    
    Why add extra `{}` here? we could also make `comment_placeholder` shorter


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63944461
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/WholeStageCodegenExec.scala ---
    @@ -78,8 +77,9 @@ trait CodegenSupport extends SparkPlan {
       final def produce(ctx: CodegenContext, parent: CodegenSupport): String = executeQuery {
         this.parent = parent
         ctx.freshNamePrefix = variablePrefix
    +    val placeHolder = ctx.registerComment(s"PRODUCE: ${this.simpleString}")
         s"""
    -       |/*** PRODUCE: ${toCommentSafeString(this.simpleString)} */
    +       |$placeHolder
    --- End diff --
    
    inline `ctx.registerComment(s"PRODUCE: ${this.simpleString}")` here


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220675159
  
    LGTM, 
    Merging this into master and 2.0, thanks!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15205][SQL][WIP] Codegen can compile th...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-218584392
  
    **[Test build #58394 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/58394/consoleFull)** for PR 12979 at commit [`60b8e0b`](https://github.com/apache/spark/commit/60b8e0b9c0ca94fe2ec46c9ab2c33664e2693269).
     * This patch **fails PySpark unit tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220321546
  
    Merged build finished. Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220378989
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220321521
  
    **[Test build #58866 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/58866/consoleFull)** for PR 12979 at commit [`b7e7b3d`](https://github.com/apache/spark/commit/b7e7b3dcb1e7802b403d9727ad04253b891533c6).
     * This patch **fails MiMa tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220429568
  
    **[Test build #58880 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/58880/consoleFull)** for PR 12979 at commit [`211e90b`](https://github.com/apache/spark/commit/211e90bbad8703c62225315f986e9b28d388e50c).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220342991
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/58867/
    Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220693945
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15205][SQL][WIP] Codegen can compile mo...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-217683639
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63916719
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
    @@ -706,6 +711,60 @@ class CodegenContext {
         if (doSubexpressionElimination) subexpressionElimination(expressions)
         expressions.map(e => e.genCode(this))
       }
    +
    +  /**
    +   * get a map of the pair of a place holder and a corresponding comment
    +   */
    +  def getPlaceHolderToComments(): collection.Map[String, String] = {
    +    placeHolderToComments
    +  }
    +
    +  /**
    +   * Returns the string representation of this expression that is safe to be put in
    +   * code comments of generated code. The length is capped at 128 characters.
    +   */
    +  private[this] def toCommentSafeString(str: String): String = {
    --- End diff --
    
    This is only used by CodeFormatter, I'd like to move it there.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63743517
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
    @@ -706,6 +712,20 @@ class CodegenContext {
         if (doSubexpressionElimination) subexpressionElimination(expressions)
         expressions.map(e => e.genCode(this))
       }
    +
    +  /**
    +   * Add a pair of a place holder and a corresponding comment
    +   */
    +  def addCommentEntry(placeHolder: String, comment: String): Unit = {
    +    placeHolderToCommentMap += (placeHolder -> comment)
    +  }
    +
    +  /**
    +   * Copy an immutable map of the pair of a place holder and a corresponding comment
    +   */
    +  def copyPlaceHolderToCommentMap(): immutable.Map[String, String] = {
    --- End diff --
    
    CodegenContext is not re-used, do we still need this copy?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220664748
  
    **[Test build #59003 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59003/consoleFull)** for PR 12979 at commit [`7fd047c`](https://github.com/apache/spark/commit/7fd047cf6639a61e5a4132c37d85a139b21bd266).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...

Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63750085
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala ---
    @@ -97,15 +97,20 @@ abstract class Expression extends TreeNode[Expression] {
         ctx.subExprEliminationExprs.get(this).map { subExprState =>
           // This expression is repeated which means that the code to evaluate it has already been added
           // as a function before. In that case, we just re-use it.
    -      val code = s"/* ${toCommentSafeString(this.toString)} */"
    -      ExprCode(code, subExprState.isNull, subExprState.value)
    +      val placeHolder = s"/*{${ctx.freshName("comment_placeholder")}}*/"
    --- End diff --
    
    Actually, `}` is really needed.
    If we have place holders like `comment_placeholder1` and `comment_placeholder12`, and the actual comment `Hello` corresponds to `comment_placeholder1`, `comment_placeholder1` will be replaced with `Hello` but also `comment_placeholder12` will be replaced with `Hello2`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63944155
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
    @@ -717,6 +751,20 @@ abstract class GeneratedClass {
     }
     
     /**
    + * A wrapper for the source code to be compiled by [[CodeGenerator]].
    + */
    +class CodeAndComment(val body: String, val comment: collection.Map[String, String])
    +  extends Serializable {
    +  override def equals(that: Any): Boolean = that match {
    +    case null => false
    --- End diff --
    
    this case could be removed


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220538994
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/58945/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220675104
  
    **[Test build #59008 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59008/consoleFull)** for PR 12979 at commit [`a039d18`](https://github.com/apache/spark/commit/a039d18bc77d29d2113099c02383c5abb2b1cf09).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220521459
  
    **[Test build #58944 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/58944/consoleFull)** for PR 12979 at commit [`3c49567`](https://github.com/apache/spark/commit/3c495670716ecf63d21110f7c7ee93500051d26a).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220378999
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/58871/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220568430
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220429839
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220568189
  
    **[Test build #58946 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/58946/consoleFull)** for PR 12979 at commit [`6ebfa10`](https://github.com/apache/spark/commit/6ebfa10d63a2234dae4e567f06279f5e7feb3df9).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220697997
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-219947527
  
    **[Test build #58746 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/58746/consoleFull)** for PR 12979 at commit [`4cb5ed3`](https://github.com/apache/spark/commit/4cb5ed3ade800b87561c10ac3bb1dd6f84ba8a0a).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `class CodeAndComment(val body: String, val comment: Map[String, String]) extends Serializable `


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63943394
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/columnar/GenerateColumnAccessor.scala ---
    @@ -224,7 +224,9 @@ object GenerateColumnAccessor extends CodeGenerator[Seq[DataType], ColumnarItera
             }
           }"""
     
    -    logDebug(s"Generated ColumnarIterator:\n${CodeFormatter.format(code)}")
    +    val code = new CodeAndComment(codeBody, ctx.getPlaceHolderToComments())
    --- End diff --
    
    ```
    val code = new CodeAndComment(codeBody, ctx.getPlaceHolderToComments())
    logDebug(s"Generated ColumnarIterator:\n${CodeFormatter.format(code)}")
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220522052
  
    **[Test build #58945 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/58945/consoleFull)** for PR 12979 at commit [`6ebfa10`](https://github.com/apache/spark/commit/6ebfa10d63a2234dae4e567f06279f5e7feb3df9).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220641694
  
    **[Test build #59003 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59003/consoleFull)** for PR 12979 at commit [`7fd047c`](https://github.com/apache/spark/commit/7fd047cf6639a61e5a4132c37d85a139b21bd266).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220390039
  
    @sarutak I like this idea, could you simplify it and minimize the changes?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63943606
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
    @@ -706,6 +711,35 @@ class CodegenContext {
         if (doSubexpressionElimination) subexpressionElimination(expressions)
         expressions.map(e => e.genCode(this))
       }
    +
    +  /**
    +   * get a map of the pair of a place holder and a corresponding comment
    +   */
    +  def getPlaceHolderToComments(): collection.Map[String, String] = placeHolderToComments
    +
    +  /**
    +   * Register a multi-line comment and return the corresponding place holder
    +   */
    +  def registerMultilineComment(text: String): String = {
    --- End diff --
    
    private


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63918888
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
    @@ -740,6 +813,9 @@ abstract class CodeGenerator[InType <: AnyRef, OutType <: AnyRef] extends Loggin
       /** Binds an input expression to a given input schema */
       protected def bind(in: InType, inputSchema: Seq[Attribute]): InType
     
    +  /** Generate the code to be compiled represented as string */
    +  def generateCodeAndComments(ctx: CodegenContext, in: InType): CodeAndComment
    --- End diff --
    
    I added this method for testability. We can test whether place holder is put in generated code as we intended.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63919939
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
    @@ -706,6 +711,60 @@ class CodegenContext {
         if (doSubexpressionElimination) subexpressionElimination(expressions)
         expressions.map(e => e.genCode(this))
       }
    +
    +  /**
    +   * get a map of the pair of a place holder and a corresponding comment
    +   */
    +  def getPlaceHolderToComments(): collection.Map[String, String] = {
    +    placeHolderToComments
    +  }
    +
    +  /**
    +   * Returns the string representation of this expression that is safe to be put in
    +   * code comments of generated code. The length is capped at 128 characters.
    +   */
    +  private[this] def toCommentSafeString(str: String): String = {
    +    val len = math.min(str.length, 128)
    +    val suffix = if (str.length > len) "..." else ""
    +
    +    // Unicode literals, like \u0022, should be escaped before
    +    // they are put in code comment to avoid codegen breaking.
    +    // To escape them, single "\" should be prepended to a series of "\" just before "u"
    +    // only when the number of "\" is odd.
    +    // For example, \u0022 should become to \\u0022
    +    // but \\u0022 should not become to \\\u0022 because the first backslash escapes the second one,
    +    // and \u0022 will remain, means not escaped.
    +    // Otherwise, the runtime Java compiler will fail to compile or code injection can be allowed.
    +    // For details, see SPARK-15165.
    +    str.substring(0, len).replace("*/", "*\\/")
    +      .replaceAll("(^|[^\\\\])(\\\\(\\\\\\\\)*u)", "$1\\\\$2") + suffix
    --- End diff --
    
    Yeah, I think we can remove this method rather than move to `CodeFormatter`. What do you think?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63763448
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateMutableProjection.scala ---
    @@ -133,7 +133,9 @@ object GenerateMutableProjection extends CodeGenerator[Seq[Expression], MutableP
           }
         """
     
    -    logDebug(s"code for ${expressions.mkString(",")}:\n${CodeFormatter.format(code)}")
    --- End diff --
    
    I see. You mean we don't need change the line to `logDebug(s"code for ${expressions.mkString(",")}:\n$formatted")`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220404758
  
    **[Test build #58880 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/58880/consoleFull)** for PR 12979 at commit [`211e90b`](https://github.com/apache/spark/commit/211e90bbad8703c62225315f986e9b28d388e50c).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-219947763
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220526877
  
    **[Test build #58946 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/58946/consoleFull)** for PR 12979 at commit [`6ebfa10`](https://github.com/apache/spark/commit/6ebfa10d63a2234dae4e567f06279f5e7feb3df9).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220650978
  
    **[Test build #59008 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59008/consoleFull)** for PR 12979 at commit [`a039d18`](https://github.com/apache/spark/commit/a039d18bc77d29d2113099c02383c5abb2b1cf09).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220693564
  
    **[Test build #59001 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59001/consoleFull)** for PR 12979 at commit [`4029d44`](https://github.com/apache/spark/commit/4029d44fee7a566c323cfa91641037493d8ee687).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220693949
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/59001/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63744305
  
    --- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CodeGenerationSuite.scala ---
    @@ -177,6 +177,55 @@ class CodeGenerationSuite extends SparkFunSuite with ExpressionEvalHelper {
         assert(internalRow === internalRow2)
       }
     
    +  test("reuse cached code") {
    --- End diff --
    
    remove this test case


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15205][SQL][WIP] Codegen can compile mo...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-217679242
  
    **[Test build #58079 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/58079/consoleFull)** for PR 12979 at commit [`766f5c6`](https://github.com/apache/spark/commit/766f5c64f93163ee1b8b7ea8b1ef986f9a731733).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...

Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12979#discussion_r63743830
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
    @@ -717,6 +737,23 @@ abstract class GeneratedClass {
     }
     
     /**
    + * A wrapper for the source code to be compiled by [[CodeGenerator]].
    + */
    +class CodeAndComment(val body: String, val comment: Map[String, String]) extends Serializable {
    +  override def equals(that: Any): Boolean = {
    +    if (!that.isInstanceOf[CodeAndComment]) {
    +      return false
    +    }
    +    val thatSourceCode = that.asInstanceOf[CodeAndComment]
    +    if (thatSourceCode eq null) return false
    --- End diff --
    
    We could use match-case to simplify this


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15165][SPARK-15205][SQL] Introduce plac...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-220526124
  
    Merged build finished. Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/12979#issuecomment-219947764
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/58746/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org