Skip to content

[SPARK-14251][SQL] Add SQL command for printing out generated code for debugging#12099

Closed
dongjoon-hyun wants to merge 6 commits intoapache:masterfrom
dongjoon-hyun:SPARK-14251
Closed

[SPARK-14251][SQL] Add SQL command for printing out generated code for debugging#12099
dongjoon-hyun wants to merge 6 commits intoapache:masterfrom
dongjoon-hyun:SPARK-14251

Conversation

@dongjoon-hyun
Copy link
Member

What changes were proposed in this pull request?

This PR implements EXPLAIN CODEGEN SQL command which returns generated codes like debugCodegen. In spark-shell, we don't need to import debug module. In spark-sql, we can use this SQL command now.

Before

scala> import org.apache.spark.sql.execution.debug._
scala> sql("select 'a' as a group by 1").debugCodegen()
Found 2 WholeStageCodegen subtrees.
== Subtree 1 / 2 ==
...

Generated code:
...

== Subtree 2 / 2 ==
...

Generated code:
...

After

scala> sql("explain extended codegen select 'a' as a group by 1").collect().foreach(println)
[Found 2 WholeStageCodegen subtrees.]
[== Subtree 1 / 2 ==]
...
[]
[Generated code:]
...
[]
[== Subtree 2 / 2 ==]
...
[]
[Generated code:]
...

How was this patch tested?

Pass the Jenkins tests (including new testcases)

Copy link
Contributor

Choose a reason for hiding this comment

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

CODEGEN is not a keyword. Please add to the nonReserved rule.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hvanhovell does this fix the problem already? i wasn't sure what was before your comment vs after.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it is fixed. He added it to the nonReserved rule (see below).

@hvanhovell
Copy link
Contributor

@dongjoon-hyun I left a few small comments. Looks pretty solid overall.

@dongjoon-hyun
Copy link
Member Author

Thank you, @hvanhovell .

@dongjoon-hyun
Copy link
Member Author

Hi, @hvanhovell .
I updated the PR according to your comments. For the real generated code, I think it's safe to maintain the first three lines since that will not change frequently.

@SparkQA
Copy link

SparkQA commented Apr 1, 2016

Test build #54669 has finished for PR 12099 at commit 4e750e5.

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

@SparkQA
Copy link

SparkQA commented Apr 1, 2016

Test build #54673 has finished for PR 12099 at commit 83c3c01.

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

@dongjoon-hyun
Copy link
Member Author

Hi, @rxin .
Could you review this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

do we still need this function?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is still used in DebuggingSuite since I didn't change that testcase.

val res = sqlContext.range(10).groupBy("id").count().debugCodegenString()

Copy link
Contributor

Choose a reason for hiding this comment

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

yea let's change that

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure! No problem at all :)

@dongjoon-hyun
Copy link
Member Author

Hi, @rxin . debugCodegenString is removed and DebuggingSuite is modified accordingly.

@dongjoon-hyun
Copy link
Member Author

Now, EXPLAIN CODEGEN is the logically same with debugCodegen. I updated the description of this PR, too.

@dongjoon-hyun
Copy link
Member Author

One minor thing, why @DeepSparkBot does not visit this PR?
Today, @DeepSparkBot came to my other PR. :)

@SparkQA
Copy link

SparkQA commented Apr 1, 2016

Test build #54713 has finished for PR 12099 at commit 11efbbb.

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

@andrewor14
Copy link
Contributor

You need to say: deep-review this please

Choose a reason for hiding this comment

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

Detected unnecessary var.

Suggested improvement:

val outputString =
  if (codegen) {
    codegenString(queryExecution.executedPlan)
  } else if (extended) {
    queryExecution.toString
  } else {
    queryExecution.simpleString
  }

@DeepSparkBot
Copy link

@andrewor14 Review complete. No major issues found.

@dongjoon-hyun
Copy link
Member Author

Oh, I see. Thank you, @andrewor14 !

@SparkQA
Copy link

SparkQA commented Apr 1, 2016

Test build #54717 has finished for PR 12099 at commit 497a93e.

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

@dongjoon-hyun
Copy link
Member Author

I updated the code according to @DeepSparkBot 's comments, and also rebased to the master since master branch is now Scala 2.11.8.
It took sometime to test locally.

@SparkQA
Copy link

SparkQA commented Apr 2, 2016

Test build #54736 has finished for PR 12099 at commit 7b10a51.

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

@SparkQA
Copy link

SparkQA commented Apr 2, 2016

Test build #2728 has finished for PR 12099 at commit 7b10a51.

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

@rxin
Copy link
Contributor

rxin commented Apr 2, 2016

Thanks - going to merge this in master.

@asfgit asfgit closed this in fa1af0a Apr 2, 2016
@dongjoon-hyun
Copy link
Member Author

Thank you, @rxin , @hvanhovell , @andrewor14 , and @DeepSparkBot . :)

@SparkQA
Copy link

SparkQA commented Apr 2, 2016

Test build #54759 has finished for PR 12099 at commit 9334fa9.

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

@dongjoon-hyun dongjoon-hyun deleted the SPARK-14251 branch May 12, 2016 00:57
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.

6 participants