[SPARK-15165][SPARK-15205][SQL] Introduce place holder for comments in generated code #12979

sarutak · 2016-05-08T00:34:15Z

What changes were proposed in this pull request?

This PR introduce place holder for comment in generated code and the purpose is same for #12939 but much safer.

Generated code to be compiled doesn't include actual comments but includes place holder instead.

Place holders in generated code will be replaced with actual comments only at the time of logging.

Also, this PR can resolve SPARK-15205.

How was this patch tested?

Existing tests.

SparkQA · 2016-05-08T02:20:34Z

Test build #58079 has finished for PR 12979 at commit 766f5c6.

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

SparkQA · 2016-05-11T20:44:20Z

Test build #58394 has finished for PR 12979 at commit 60b8e0b.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

davies · 2016-05-18T05:57:12Z

@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).

SparkQA · 2016-05-18T07:27:55Z

Test build #58746 has finished for PR 12979 at commit 4cb5ed3.

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

davies · 2016-05-18T17:09:46Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala

      // 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")}}*/"


Why add extra {} here? we could also make comment_placeholder shorter

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.

davies · 2016-05-18T17:17:56Z

These place holders are good for safety, could you update the PR title and description?

SparkQA · 2016-05-19T13:19:45Z

Test build #58866 has finished for PR 12979 at commit b7e7b3d.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-05-20T06:16:31Z

Test build #58944 has finished for PR 12979 at commit 3c49567.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

sarutak · 2016-05-20T06:18:36Z

retest this please.

SparkQA · 2016-05-20T07:39:21Z

Test build #58945 has finished for PR 12979 at commit 6ebfa10.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-05-20T10:11:40Z

Test build #58946 has finished for PR 12979 at commit 6ebfa10.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-05-20T17:14:11Z

Test build #59003 has finished for PR 12979 at commit 7fd047c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-05-20T17:55:38Z

Test build #59008 has finished for PR 12979 at commit a039d18.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

davies · 2016-05-20T17:55:51Z

LGTM,
Merging this into master and 2.0, thanks!

… in generated code ## What changes were proposed in this pull request? This PR introduce place holder for comment in generated code and the purpose is same for #12939 but much safer. Generated code to be compiled doesn't include actual comments but includes place holder instead. Place holders in generated code will be replaced with actual comments only at the time of logging. Also, this PR can resolve SPARK-15205. ## How was this patch tested? Existing tests. Author: Kousuke Saruta <[email protected]> Closes #12979 from sarutak/SPARK-15205. (cherry picked from commit 22947cd) Signed-off-by: Davies Liu <[email protected]>

davies · 2016-05-20T17:57:35Z

@sarutak Could you create another PR for 1.6? (If we have not fix the security bug in 1.6)

sarutak · 2016-05-20T17:58:50Z

O.K, I'll do it. Need another PR for branch-1.5 too?

davies · 2016-05-20T18:04:54Z

Maybe 1.5 and 1.6 could share the same PR (if no much conflicts)

SparkQA · 2016-05-20T19:11:10Z

Test build #59001 has finished for PR 12979 at commit 4029d44.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-05-20T19:29:44Z

Test build #59004 has finished for PR 12979 at commit 9c7ff14.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

## What changes were proposed in this pull request? This PR fixes 3 slow tests: 1. `ParquetQuerySuite.read/write wide table`: This is not a good unit test as it runs more than 5 minutes. This PR removes it and add a new regression test in `CodeGenerationSuite`, which is more "unit". 2. `ParquetQuerySuite.returning batch for wide table`: reduce the threshold and use smaller data size. 3. `DatasetSuite.SPARK-14554: Dataset.map may generate wrong java code for wide table`: Improve `CodeFormatter.format`(introduced at #12979) can dramatically speed this it up. ## How was this patch tested? N/A Author: Wenchen Fan <[email protected]> Closes #13273 from cloud-fan/test. (cherry picked from commit 50b660d) Signed-off-by: Cheng Lian <[email protected]>

## What changes were proposed in this pull request? This PR fixes 3 slow tests: 1. `ParquetQuerySuite.read/write wide table`: This is not a good unit test as it runs more than 5 minutes. This PR removes it and add a new regression test in `CodeGenerationSuite`, which is more "unit". 2. `ParquetQuerySuite.returning batch for wide table`: reduce the threshold and use smaller data size. 3. `DatasetSuite.SPARK-14554: Dataset.map may generate wrong java code for wide table`: Improve `CodeFormatter.format`(introduced at #12979) can dramatically speed this it up. ## How was this patch tested? N/A Author: Wenchen Fan <[email protected]> Closes #13273 from cloud-fan/test.

Fixed duplicated generated code issue

766f5c6

sarutak changed the title ~~[SPARK-15205][SQL][WIP] Codegen can compile more than twice for the same source code~~ [SPARK-15205][SQL][WIP] Codegen can compile the same code more than twice May 8, 2016

sarutak mentioned this pull request May 8, 2016

[SPARK-13432][SQL] add the source file name and line into a generated Java code #11301

Closed

sarutak added 4 commits May 9, 2016 11:35

Merge branch 'master' of git://git.apache.org/spark into SPARK-15205

3ebd87d

Renamed getPlaceHolderAndCommentMap to getPlaceHolderToCommentMap

209ebda

Merge branch 'master' of git://git.apache.org/spark into SPARK-15205

3d70c1d

Added a test

60b8e0b

sarutak added 3 commits May 18, 2016 13:41

Merge branch 'SPARK-15205' of github.com:sarutak/spark into SPARK-15205

f6315cc

Merge branch 'master' of git://git.apache.org/spark into SPARK-15205

72407e3

Changed minor things

4cb5ed3

sarutak changed the title ~~[SPARK-15205][SQL][WIP] Codegen can compile the same code more than twice~~ [SPARK-15205][SQL] Codegen can compile the same code more than twice May 18, 2016

sarutak mentioned this pull request May 18, 2016

[SPARK-15165][SQL] Codegen can break because toCommentSafeString is not actually safe #12939

Closed

davies reviewed May 18, 2016
View reviewed changes

sarutak changed the title ~~[SPARK-15205][SQL] Codegen can compile the same code more than twice~~ [SPARK-15165][SPARK-15205][SQL] Introduce place holder for comments in generated code May 18, 2016

sarutak added 3 commits May 19, 2016 03:42

Merge branch 'master' of git://git.apache.org/spark into SPARK-15205

3283048

Merge branch 'master' of git://git.apache.org/spark into SPARK-15205

2c50388

Refactored

2aba334

sarutak force-pushed the SPARK-15205 branch from b7e7b3d to d29c7e4 Compare May 19, 2016 13:12

sarutak force-pushed the SPARK-15205 branch 2 times, most recently from 4029d44 to 7fd047c Compare May 20, 2016 15:41

Modified based on the feedback

9c7ff14

sarutak force-pushed the SPARK-15205 branch from 7fd047c to 9c7ff14 Compare May 20, 2016 15:45

Reverted some changes

a039d18

asfgit closed this in 22947cd May 20, 2016

davies mentioned this pull request May 20, 2016

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

Closed

cloud-fan mentioned this pull request May 24, 2016

[SPARK-15498][TESTS] fix slow tests #13273

Closed

sarutak deleted the SPARK-15205 branch June 4, 2021 20:47

[SPARK-15165][SPARK-15205][SQL] Introduce place holder for comments in generated code #12979

[SPARK-15165][SPARK-15205][SQL] Introduce place holder for comments in generated code #12979

Uh oh!

Conversation

sarutak commented May 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented May 8, 2016

Uh oh!

SparkQA commented May 11, 2016

Uh oh!

davies commented May 18, 2016

Uh oh!

SparkQA commented May 18, 2016

Uh oh!

davies May 18, 2016

Choose a reason for hiding this comment

Uh oh!

sarutak May 18, 2016

Choose a reason for hiding this comment

Uh oh!

davies commented May 18, 2016

Uh oh!

SparkQA commented May 19, 2016

Uh oh!

SparkQA commented May 20, 2016

Uh oh!

sarutak commented May 20, 2016

Uh oh!

SparkQA commented May 20, 2016

Uh oh!

SparkQA commented May 20, 2016

Uh oh!

SparkQA commented May 20, 2016

Uh oh!

SparkQA commented May 20, 2016

Uh oh!

davies commented May 20, 2016

Uh oh!

davies commented May 20, 2016

Uh oh!

sarutak commented May 20, 2016

Uh oh!

davies commented May 20, 2016

Uh oh!

SparkQA commented May 20, 2016

Uh oh!

SparkQA commented May 20, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sarutak commented May 8, 2016 •

edited

Loading