You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by "Yikf (via GitHub)" <gi...@apache.org> on 2024/02/25 06:45:27 UTC
[PR] [KYUUBI #6078] KSHC should handle the commit of the partitioned table as dynamic partition at write path [kyuubi]
Yikf opened a new pull request, #6082:
URL: https://github.com/apache/kyuubi/pull/6082
# :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. -->
This pull request fixes https://github.com/apache/kyuubi/issues/6078, KSHC should handle the commit of the partitioned table as dynamic partition at write path, that's beacuse the process of writing with Apache Spark DataSourceV2 using dynamic partitioning to handle static partitions.
## Describe Your Solution ๐ง
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
## Types of changes :bookmark:
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [x] 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
---
# 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! -->
- [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html)
**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 #6078] KSHC should handle the commit of the partitioned table as dynamic partition at write path [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #6082:
URL: https://github.com/apache/kyuubi/pull/6082#discussion_r1502105127
##########
extensions/spark/kyuubi-spark-connector-hive/src/main/scala/org/apache/kyuubi/spark/connector/hive/write/HiveWriteBuilder.scala:
##########
@@ -47,45 +41,22 @@ case class HiveWriteBuilder(
info,
hiveTableCatalog,
forceOverwrite,
- mergePartitionSpec(),
- ifPartitionNotExists)
+ dynamicPartitionSpec())
}
override def overwrite(filters: Array[Filter]): WriteBuilder = {
- filters match {
- case Array(AlwaysTrue) => // no partition, do nothing
- case _ => staticPartition = deduplicateFilters(filters)
- }
- overwriteDynamicPartitions()
+ forceOverwrite = true
Review Comment:
why `filters` can be ignored here?
--
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 #6078] KSHC should handle the commit of the partitioned table as dynamic partition at write path [kyuubi]
Posted by "Yikf (via GitHub)" <gi...@apache.org>.
Yikf commented on code in PR #6082:
URL: https://github.com/apache/kyuubi/pull/6082#discussion_r1502192826
##########
extensions/spark/kyuubi-spark-connector-hive/src/main/scala/org/apache/kyuubi/spark/connector/hive/write/HiveWriteBuilder.scala:
##########
@@ -47,45 +41,22 @@ case class HiveWriteBuilder(
info,
hiveTableCatalog,
forceOverwrite,
- mergePartitionSpec(),
- ifPartitionNotExists)
+ dynamicPartitionSpec())
}
override def overwrite(filters: Array[Filter]): WriteBuilder = {
- filters match {
- case Array(AlwaysTrue) => // no partition, do nothing
- case _ => staticPartition = deduplicateFilters(filters)
- }
- overwriteDynamicPartitions()
+ forceOverwrite = true
Review Comment:
The current DSv2 SupportsOverwrite#overwrite is designed to filter out (delete) data that matches deleteExpr when writer commit, But in the current catalyst implementation, these filterExprs are static partition EqualNullSafe expressions, KSHC does not currently require these processing filterExprs.
--
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 #6078] KSHC should handle the commit of the partitioned table as dynamic partition at write path [kyuubi]
Posted by "Yikf (via GitHub)" <gi...@apache.org>.
Yikf commented on PR #6082:
URL: https://github.com/apache/kyuubi/pull/6082#issuecomment-1962833842
Please take a look if you have a time, @FANNG1 @pan3793
--
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 #6078] KSHC should handle the commit of the partitioned table as dynamic partition at write path [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #6082:
URL: https://github.com/apache/kyuubi/pull/6082#discussion_r1515465515
##########
extensions/spark/kyuubi-spark-connector-hive/src/main/scala/org/apache/kyuubi/spark/connector/hive/write/HiveWriteBuilder.scala:
##########
@@ -47,45 +41,22 @@ case class HiveWriteBuilder(
info,
hiveTableCatalog,
forceOverwrite,
- mergePartitionSpec(),
- ifPartitionNotExists)
+ dynamicPartitionSpec())
}
override def overwrite(filters: Array[Filter]): WriteBuilder = {
- filters match {
- case Array(AlwaysTrue) => // no partition, do nothing
- case _ => staticPartition = deduplicateFilters(filters)
- }
- overwriteDynamicPartitions()
+ forceOverwrite = true
Review Comment:
can we add an `requires` guard here?
##########
extensions/spark/kyuubi-spark-connector-hive/src/main/scala/org/apache/kyuubi/spark/connector/hive/write/HiveWriteBuilder.scala:
##########
@@ -47,45 +41,22 @@ case class HiveWriteBuilder(
info,
hiveTableCatalog,
forceOverwrite,
- mergePartitionSpec(),
- ifPartitionNotExists)
+ dynamicPartitionSpec())
}
override def overwrite(filters: Array[Filter]): WriteBuilder = {
- filters match {
- case Array(AlwaysTrue) => // no partition, do nothing
- case _ => staticPartition = deduplicateFilters(filters)
- }
- overwriteDynamicPartitions()
+ forceOverwrite = true
Review Comment:
can we add a `requires` guard here?
--
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 #6078] KSHC should handle the commit of the partitioned table as dynamic partition at write path [kyuubi]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #6082:
URL: https://github.com/apache/kyuubi/pull/6082#issuecomment-1962847897
## [Codecov](https://app.codecov.io/gh/apache/kyuubi/pull/6082?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `29.82456%` with `40 lines` in your changes are missing coverage. Please review.
> Project coverage is 61.14%. Comparing base [(`1adf392`)](https://app.codecov.io/gh/apache/kyuubi/commit/1adf39294c607f1a2cd8527145de6ddfd21b7c92?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`2ae1836`)](https://app.codecov.io/gh/apache/kyuubi/pull/6082?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 2 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/kyuubi/pull/6082?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...bi/spark/connector/hive/write/HiveBatchWrite.scala](https://app.codecov.io/gh/apache/kyuubi/pull/6082?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1Ymktc3BhcmstY29ubmVjdG9yLWhpdmUvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1Ymkvc3BhcmsvY29ubmVjdG9yL2hpdmUvd3JpdGUvSGl2ZUJhdGNoV3JpdGUuc2NhbGE=) | 21.56% | [39 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/kyuubi/pull/6082?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 #6082 +/- ##
============================================
+ Coverage 61.12% 61.14% +0.02%
Complexity 23 23
============================================
Files 623 623
Lines 37206 37166 -40
Branches 5041 5030 -11
============================================
- Hits 22743 22727 -16
+ Misses 12012 11995 -17
+ Partials 2451 2444 -7
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/kyuubi/pull/6082?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 #6078] KSHC should handle the commit of the partitioned table as dynamic partition at write path [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #6082:
URL: https://github.com/apache/kyuubi/pull/6082#issuecomment-1983123702
Overall LGTM, let me merge it first, and address comment in follow-up PRs.
--
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 #6078] KSHC should handle the commit of the partitioned table as dynamic partition at write path [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 closed pull request #6082: [KYUUBI #6078] KSHC should handle the commit of the partitioned table as dynamic partition at write path
URL: https://github.com/apache/kyuubi/pull/6082
--
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