Skip to content

Conversation

@kiszk
Copy link
Member

@kiszk kiszk commented Feb 14, 2018

What changes were proposed in this pull request?

This PR always adds codegenStageId in comment of the generated class. This is a replication of #20419 for post-Spark 2.3.
Closes #20419

/* 001 */ public Object generate(Object[] references) {
/* 002 */   return new GeneratedIteratorForCodegenStage1(references);
/* 003 */ }
/* 004 */
/* 005 */ // codegenStageId=1
/* 006 */ final class GeneratedIteratorForCodegenStage1 extends org.apache.spark.sql.execution.BufferedRowIterator {
/* 007 */   private Object[] references;
...

How was this patch tested?

Existing tests

@SparkQA
Copy link

SparkQA commented Feb 14, 2018

Test build #87452 has finished for PR 20612 at commit 1bf0640.

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

@kiszk
Copy link
Member Author

kiszk commented Feb 15, 2018

ping @gatorsmile, @rednaxelafx

@rednaxelafx
Copy link
Contributor

LGTM. Let's wait for one more LGTM from @gatorsmile / @cloud-fan .

val name = freshName("c")
if (force ||
SparkEnv.get != null && SparkEnv.get.conf.getBoolean("spark.sql.codegen.comments", false)) {
val name = if (placeholderId != "") placeholderId else freshName("c")
Copy link
Contributor

Choose a reason for hiding this comment

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

although the caller should guarantee placeholderId is unique, shall we add a check here for safe? e.g. assert(!placeHolderToComments.containsKey("placeholderId"))

Copy link
Member Author

@kiszk kiszk Feb 20, 2018

Choose a reason for hiding this comment

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

+1, sure

@cloud-fan
Copy link
Contributor

LGTM

@SparkQA
Copy link

SparkQA commented Feb 20, 2018

Test build #87555 has finished for PR 20612 at commit eed9dcd.

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

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@asfgit asfgit closed this in 95e25ed Feb 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants