You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by "bowenliang123 (via GitHub)" <gi...@apache.org> on 2023/12/26 06:44:36 UTC
[PR] [KYUUBI #5377][FOLLOWUP] Improvements and delete result on exit for saving result file in Spark Engine [kyuubi]
bowenliang123 opened a new pull request, #5923:
URL: https://github.com/apache/kyuubi/pull/5923
# :mag: Description
## Issue References ๐
<!-- Append the issue number after #. If there is no issue for you to link create one or -->
<!-- If there are no issues to link, please provide details here. -->
As described.
## Describe Your Solution ๐ง
- delete result data file on exit for saving result file in Spark Engine
- cleanup redundant code
## Types of changes :bookmark:
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Bugfix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
## Test Plan ๐งช
#### Behavior Without This Pull Request :coffin:
#### Behavior With This Pull Request :tada:
#### Related Unit Tests
---
# Checklists
## ๐ Author Self Checklist
<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
- [ ] My code follows the [style guidelines](https://kyuubi.readthedocs.io/en/master/contributing/code/style.html) of this project
- [ ] I have performed a self-review
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [ ] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my feature works
- [ ] New and existing unit tests pass locally with my changes
- [ ] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html)
## ๐ Committer Pre-Merge Checklist
- [ ] Pull request title is okay.
- [ ] No license issues.
- [ ] Milestone correctly set?
- [ ] Test coverage is ok
- [ ] Assignees are selected.
- [ ] Minimum number of approvals
- [ ] No changes are requested
**Be nice. Be informative.**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5377][FOLLOWUP] Improvements and delete result on exit for saving result data file in Spark Engine [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #5923:
URL: https://github.com/apache/kyuubi/pull/5923#discussion_r1437420468
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/ExecuteStatement.scala:
##########
@@ -180,21 +180,24 @@ class ExecuteStatement(
val sessionId = session.handle.identifier.toString
val savePath = session.sessionManager.getConf.get(OPERATION_RESULT_SAVE_TO_FILE_DIR)
saveFileName = Some(s"$savePath/$engineId/$sessionId/$statementId")
- // Rename all col name to avoid duplicate columns
- val colName = range(0, result.schema.size).map(x => "col" + x)
-
- val codec = if (SPARK_ENGINE_RUNTIME_VERSION >= "3.2") "zstd" else "zlib"
- // df.write will introduce an extra shuffle for the outermost limit, and hurt performance
- if (resultMaxRows > 0) {
- result.toDF(colName: _*).limit(resultMaxRows).write
- .option("compression", codec).format("orc").save(saveFileName.get)
- } else {
- result.toDF(colName: _*).write
- .option("compression", codec).format("orc").save(saveFileName.get)
+ val saveFile = saveFileName.get
+ FileUtil.fullyDeleteOnExit(new File(saveFile))
+ val resultDfToSave: DataFrame = {
+ // Rename all col name to avoid duplicate columns
+ val df = resultDF.toDF(result.schema.indices.map(x => s"col_$x"): _*)
+ if (resultMaxRows > 0) {
+ // df.write will introduce an extra shuffle for the outermost limit,
+ // and hurt performance
+ df.limit(resultMaxRows)
+ } else {
+ df
+ }
}
- info(s"Save result to $saveFileName")
+ val codec = if (SPARK_ENGINE_RUNTIME_VERSION >= "3.2") "zstd" else "zlib"
+ resultDfToSave.write.option("compression", codec).format("orc").save(saveFile)
+ info(s"Save result of statement $statementId to $saveFileName.get")
Review Comment:
OK, changed to use the `saveFile` variable directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5377][FOLLOWUP] Improvements and delete result on exit for saving result data file in Spark Engine [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 closed pull request #5923: [KYUUBI #5377][FOLLOWUP] Improvements and delete result on exit for saving result data file in Spark Engine
URL: https://github.com/apache/kyuubi/pull/5923
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5377][FOLLOWUP] Improvements and delete result on exit for saving result data file in Spark Engine [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #5923:
URL: https://github.com/apache/kyuubi/pull/5923#discussion_r1437431542
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/ExecuteStatement.scala:
##########
@@ -180,21 +180,24 @@ class ExecuteStatement(
val sessionId = session.handle.identifier.toString
val savePath = session.sessionManager.getConf.get(OPERATION_RESULT_SAVE_TO_FILE_DIR)
saveFileName = Some(s"$savePath/$engineId/$sessionId/$statementId")
- // Rename all col name to avoid duplicate columns
- val colName = range(0, result.schema.size).map(x => "col" + x)
-
- val codec = if (SPARK_ENGINE_RUNTIME_VERSION >= "3.2") "zstd" else "zlib"
- // df.write will introduce an extra shuffle for the outermost limit, and hurt performance
- if (resultMaxRows > 0) {
- result.toDF(colName: _*).limit(resultMaxRows).write
- .option("compression", codec).format("orc").save(saveFileName.get)
- } else {
- result.toDF(colName: _*).write
- .option("compression", codec).format("orc").save(saveFileName.get)
+ val saveFile = saveFileName.get
+ FileUtil.fullyDeleteOnExit(new File(saveFile))
Review Comment:
OK, changed to use hadoop fs to cleanup saved result file, as exited code in `close()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5377][FOLLOWUP] Improvements and delete result on exit for saving result data file in Spark Engine [kyuubi]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #5923:
URL: https://github.com/apache/kyuubi/pull/5923#issuecomment-1869366730
## [Codecov](https://app.codecov.io/gh/apache/kyuubi/pull/5923?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `10 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`86bec4d`)](https://app.codecov.io/gh/apache/kyuubi/commit/86bec4d914cc0de089b5183f87e3a45ade9d60e3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.61% compared to head [(`58b8882`)](https://app.codecov.io/gh/apache/kyuubi/pull/5923?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.51%.
> Report is 1 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/kyuubi/pull/5923?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...uubi/engine/spark/operation/ExecuteStatement.scala](https://app.codecov.io/gh/apache/kyuubi/pull/5923?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9vcGVyYXRpb24vRXhlY3V0ZVN0YXRlbWVudC5zY2FsYQ==) | 0.00% | [10 Missing :warning: ](https://app.codecov.io/gh/apache/kyuubi/pull/5923?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #5923 +/- ##
============================================
- Coverage 61.61% 61.51% -0.10%
Complexity 23 23
============================================
Files 616 616
Lines 36387 36389 +2
Branches 4978 4979 +1
============================================
- Hits 22419 22385 -34
- Misses 11557 11586 +29
- Partials 2411 2418 +7
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/kyuubi/pull/5923?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5377][FOLLOWUP] Improvements and delete result on exit for saving result data file in Spark Engine [kyuubi]
Posted by "cxzl25 (via GitHub)" <gi...@apache.org>.
cxzl25 commented on code in PR #5923:
URL: https://github.com/apache/kyuubi/pull/5923#discussion_r1441614229
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/ExecuteStatement.scala:
##########
@@ -178,23 +174,29 @@ class ExecuteStatement(
resultSaveThreshold,
result)) {
val sessionId = session.handle.identifier.toString
- val savePath = session.sessionManager.getConf.get(OPERATION_RESULT_SAVE_TO_FILE_DIR)
- saveFileName = Some(s"$savePath/$engineId/$sessionId/$statementId")
- // Rename all col name to avoid duplicate columns
- val colName = range(0, result.schema.size).map(x => "col" + x)
-
- val codec = if (SPARK_ENGINE_RUNTIME_VERSION >= "3.2") "zstd" else "zlib"
- // df.write will introduce an extra shuffle for the outermost limit, and hurt performance
- if (resultMaxRows > 0) {
- result.toDF(colName: _*).limit(resultMaxRows).write
- .option("compression", codec).format("orc").save(saveFileName.get)
- } else {
- result.toDF(colName: _*).write
- .option("compression", codec).format("orc").save(saveFileName.get)
+ saveFileName = {
+ val resultSaveToDir =
+ session.sessionManager.getConf.get(OPERATION_RESULT_SAVE_TO_FILE_DIR)
+ Some(s"$resultSaveToDir/$engineId/$sessionId/$statementId")
}
- info(s"Save result to $saveFileName")
+ Utils.addShutdownHook(() => cleanupSavedResultFile())
Review Comment:
After the engine exits, the entire directory will be cleaned, which is more efficient than cleaning files one by one.
If you want to clean up the query results quickly, I suggest deleting it directly in the close method.
https://github.com/apache/kyuubi/blob/4cceb1dbd72ec9d6ac0c9dc9a5b2b21f0600ba9b/externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/SparkSQLEngine.scala#L94-L101
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5377][FOLLOWUP] Improvements and delete result on exit for saving result data file in Spark Engine [kyuubi]
Posted by "cxzl25 (via GitHub)" <gi...@apache.org>.
cxzl25 commented on code in PR #5923:
URL: https://github.com/apache/kyuubi/pull/5923#discussion_r1436461659
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/ExecuteStatement.scala:
##########
@@ -180,21 +180,24 @@ class ExecuteStatement(
val sessionId = session.handle.identifier.toString
val savePath = session.sessionManager.getConf.get(OPERATION_RESULT_SAVE_TO_FILE_DIR)
saveFileName = Some(s"$savePath/$engineId/$sessionId/$statementId")
- // Rename all col name to avoid duplicate columns
- val colName = range(0, result.schema.size).map(x => "col" + x)
-
- val codec = if (SPARK_ENGINE_RUNTIME_VERSION >= "3.2") "zstd" else "zlib"
- // df.write will introduce an extra shuffle for the outermost limit, and hurt performance
- if (resultMaxRows > 0) {
- result.toDF(colName: _*).limit(resultMaxRows).write
- .option("compression", codec).format("orc").save(saveFileName.get)
- } else {
- result.toDF(colName: _*).write
- .option("compression", codec).format("orc").save(saveFileName.get)
+ val saveFile = saveFileName.get
+ FileUtil.fullyDeleteOnExit(new File(saveFile))
+ val resultDfToSave: DataFrame = {
+ // Rename all col name to avoid duplicate columns
+ val df = resultDF.toDF(result.schema.indices.map(x => s"col_$x"): _*)
+ if (resultMaxRows > 0) {
+ // df.write will introduce an extra shuffle for the outermost limit,
+ // and hurt performance
+ df.limit(resultMaxRows)
+ } else {
+ df
+ }
}
- info(s"Save result to $saveFileName")
+ val codec = if (SPARK_ENGINE_RUNTIME_VERSION >= "3.2") "zstd" else "zlib"
+ resultDfToSave.write.option("compression", codec).format("orc").save(saveFile)
+ info(s"Save result of statement $statementId to $saveFileName.get")
Review Comment:
```suggestion
info(s"Save result of statement $statementId to ${saveFileName.get}")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5377][FOLLOWUP] Improvements and delete result on exit for saving result data file in Spark Engine [kyuubi]
Posted by "cxzl25 (via GitHub)" <gi...@apache.org>.
cxzl25 commented on code in PR #5923:
URL: https://github.com/apache/kyuubi/pull/5923#discussion_r1455041144
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/ExecuteStatement.scala:
##########
@@ -178,23 +174,29 @@ class ExecuteStatement(
resultSaveThreshold,
result)) {
val sessionId = session.handle.identifier.toString
- val savePath = session.sessionManager.getConf.get(OPERATION_RESULT_SAVE_TO_FILE_DIR)
- saveFileName = Some(s"$savePath/$engineId/$sessionId/$statementId")
- // Rename all col name to avoid duplicate columns
- val colName = range(0, result.schema.size).map(x => "col" + x)
-
- val codec = if (SPARK_ENGINE_RUNTIME_VERSION >= "3.2") "zstd" else "zlib"
- // df.write will introduce an extra shuffle for the outermost limit, and hurt performance
- if (resultMaxRows > 0) {
- result.toDF(colName: _*).limit(resultMaxRows).write
- .option("compression", codec).format("orc").save(saveFileName.get)
- } else {
- result.toDF(colName: _*).write
- .option("compression", codec).format("orc").save(saveFileName.get)
+ saveFileName = {
+ val resultSaveToDir =
+ session.sessionManager.getConf.get(OPERATION_RESULT_SAVE_TO_FILE_DIR)
+ Some(s"$resultSaveToDir/$engineId/$sessionId/$statementId")
}
- info(s"Save result to $saveFileName")
+ Utils.addShutdownHook(() => cleanupSavedResultFile())
Review Comment:
Already did.
https://github.com/apache/kyuubi/blob/e9e2d189ba05c899fd85041e2a3bb7db4f5f009f/externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/ExecuteStatement.scala#L70-L78
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5377][FOLLOWUP] Improvements and delete result on exit for saving result data file in Spark Engine [kyuubi]
Posted by "cxzl25 (via GitHub)" <gi...@apache.org>.
cxzl25 commented on code in PR #5923:
URL: https://github.com/apache/kyuubi/pull/5923#discussion_r1436463471
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/ExecuteStatement.scala:
##########
@@ -180,21 +180,24 @@ class ExecuteStatement(
val sessionId = session.handle.identifier.toString
val savePath = session.sessionManager.getConf.get(OPERATION_RESULT_SAVE_TO_FILE_DIR)
saveFileName = Some(s"$savePath/$engineId/$sessionId/$statementId")
- // Rename all col name to avoid duplicate columns
- val colName = range(0, result.schema.size).map(x => "col" + x)
-
- val codec = if (SPARK_ENGINE_RUNTIME_VERSION >= "3.2") "zstd" else "zlib"
- // df.write will introduce an extra shuffle for the outermost limit, and hurt performance
- if (resultMaxRows > 0) {
- result.toDF(colName: _*).limit(resultMaxRows).write
- .option("compression", codec).format("orc").save(saveFileName.get)
- } else {
- result.toDF(colName: _*).write
- .option("compression", codec).format("orc").save(saveFileName.get)
+ val saveFile = saveFileName.get
+ FileUtil.fullyDeleteOnExit(new File(saveFile))
Review Comment:
I don't think this method can clean up remote files, such as HDFS.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #5377][FOLLOWUP] Improvements and delete result on exit for saving result data file in Spark Engine [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on PR #5923:
URL: https://github.com/apache/kyuubi/pull/5923#issuecomment-1869315822
cc @lsm1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org