[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-15205][SQL] Codegen can compile the sam...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
