New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-31673][SQL] QueryExection.debug.toFile() to take an addtional explain mode param #28493
Conversation
Test build #122484 has finished for PR 28493 at commit
|
The change looks useful to me. cc: @MaxGekk |
sql/core/src/main/scala/org/apache/spark/sql/execution/QueryExecution.scala
Outdated
Show resolved
Hide resolved
+1 too |
Test build #122522 has finished for PR 28493 at commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay.
retest this please |
Test build #123048 has finished for PR 28493 at commit
|
retest this please |
Test build #123054 has finished for PR 28493 at commit
|
@MaxGekk did you find some time to take a look? If not, I will just merge after double checking. |
} | ||
} | ||
|
||
private def simpleString(formatted: Boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: put formatted: Boolean,
to the next line
*/ | ||
def toFile(path: String, maxFields: Int = Int.MaxValue): Unit = { | ||
def toFile(path: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put path: String
to the next line. See https://github.com/databricks/scala-style-guide#spacing-and-indentation
@@ -53,6 +53,21 @@ class QueryExecutionSuite extends SharedSparkSession { | |||
s"*(1) Range (0, $expected, step=1, splits=2)", | |||
"")) | |||
} | |||
|
|||
def checkDumpedPlansInFormattedMode(path: String, expected: Int): Unit = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function is used only in one test. Could you embed its body to the test.
@@ -249,18 +277,26 @@ class QueryExecution( | |||
* Dumps debug information about query execution into the specified file. | |||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you describe path
too.
def toFile(path: String, maxFields: Int = Int.MaxValue): Unit = { | ||
def toFile(path: String, | ||
maxFields: Int = Int.MaxValue, | ||
explainMode: Option[String] = None): Unit = { | ||
val filePath = new Path(path) | ||
val fs = filePath.getFileSystem(sparkSession.sessionState.newHadoopConf()) | ||
val writer = new BufferedWriter(new OutputStreamWriter(fs.create(filePath))) | ||
val append = (s: String) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, remove append
, and pass writer.write
directly:
explainString(mode, maxFields, writer.write)
e5e360f
to
1fea0e2
Compare
Test build #123095 has finished for PR 28493 at commit
|
Merged to master. |
Thank you very much @HyukjinKwon @maropu @MaxGekk |
What changes were proposed in this pull request?
Currently QueryExecution.debug.toFile dumps the query plan information in a fixed format. This PR adds an additional explain mode parameter that writes the debug information as per the user supplied format.
Why are the changes needed?
Hopefully enhances the usability of debug.toFile(..)
Does this PR introduce any user-facing change?
No
How was this patch tested?
Added a test in QueryExecutionSuite