You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "wecharyu (via GitHub)" <gi...@apache.org> on 2023/02/17 18:24:48 UTC
[GitHub] [spark] wecharyu opened a new pull request, #40069: [SPARK-42480][SQL] Improve the performance of drop partitions
wecharyu opened a new pull request, #40069:
URL: https://github.com/apache/spark/pull/40069
### What changes were proposed in this pull request?
1. Change to get matching partition names rather than partition objects when drop partitions
### Why are the changes needed?
1. Partition names are enough to drop partitions
2. It can reduce the time overhead and driver memory overhead.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Passing the existing tests.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #40069: [SPARK-42480][SQL] Improve the performance of drop partitions
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #40069:
URL: https://github.com/apache/spark/pull/40069#discussion_r1130236038
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -1227,6 +1227,14 @@ object SQLConf {
.booleanConf
.createWithDefault(false)
+ val HIVE_METASTORE_DROP_PARTITION_BY_NAME =
+ buildConf("spark.sql.hive.dropPartitionByName.enabled")
+ .doc("When true, Spark will get partition name rather than partition object " +
+ "to drop partition, which can improve the performance of drop partition.")
+ .version("3.4.0")
Review Comment:
Thank you for the decision. I also support your decision. Here is my +1.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sunchao commented on a diff in pull request #40069: [SPARK-42480][SQL] Improve the performance of drop partitions
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #40069:
URL: https://github.com/apache/spark/pull/40069#discussion_r1130235276
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -1227,6 +1227,14 @@ object SQLConf {
.booleanConf
.createWithDefault(false)
+ val HIVE_METASTORE_DROP_PARTITION_BY_NAME =
+ buildConf("spark.sql.hive.dropPartitionByName.enabled")
+ .doc("When true, Spark will get partition name rather than partition object " +
+ "to drop partition, which can improve the performance of drop partition.")
+ .version("3.4.0")
Review Comment:
thanks for pointing out @dongjoon-hyun ! yes, let me backport this to 3.4.0 too and update the JIRA accordingly.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #40069: [SPARK-42480][SQL] Improve the performance of drop partitions
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #40069:
URL: https://github.com/apache/spark/pull/40069#discussion_r1110243153
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/util/PartitioningUtils.scala:
##########
@@ -30,6 +30,7 @@ import org.apache.spark.sql.types.{CharType, DataType, StringType, StructField,
import org.apache.spark.unsafe.types.UTF8String
private[sql] object PartitioningUtils {
+ private val PATTERN_FOR_KEY_EQ_VAL = "(.+)=(.+)".r
Review Comment:
Too idealistic, not all partition tables follow this rule. For example, we can use
`alter table ... partition(...) set location ...` to relocate the partition to any directory
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #40069: [SPARK-42480][SQL] Improve the performance of drop partitions
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #40069:
URL: https://github.com/apache/spark/pull/40069#discussion_r1110257681
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/util/PartitioningUtils.scala:
##########
@@ -30,6 +30,7 @@ import org.apache.spark.sql.types.{CharType, DataType, StringType, StructField,
import org.apache.spark.unsafe.types.UTF8String
private[sql] object PartitioningUtils {
+ private val PATTERN_FOR_KEY_EQ_VAL = "(.+)=(.+)".r
Review Comment:
So if the data corresponding to the partition `a=1` is stored in dir `/1/`, will there be a bad case with this pr?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sunchao commented on a diff in pull request #40069: [SPARK-42480][SQL] Improve the performance of drop partitions
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #40069:
URL: https://github.com/apache/spark/pull/40069#discussion_r1130235798
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -1227,6 +1227,14 @@ object SQLConf {
.booleanConf
.createWithDefault(false)
+ val HIVE_METASTORE_DROP_PARTITION_BY_NAME =
+ buildConf("spark.sql.hive.dropPartitionByName.enabled")
+ .doc("When true, Spark will get partition name rather than partition object " +
+ "to drop partition, which can improve the performance of drop partition.")
+ .version("3.4.0")
Review Comment:
I think it's pretty safe to backport to branch-3.4 since the feature is turned off by default.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wecharyu commented on a diff in pull request #40069: [SPARK-42480][SQL] Improve the performance of drop partitions
Posted by "wecharyu (via GitHub)" <gi...@apache.org>.
wecharyu commented on code in PR #40069:
URL: https://github.com/apache/spark/pull/40069#discussion_r1110559214
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/util/PartitioningUtils.scala:
##########
@@ -30,6 +30,7 @@ import org.apache.spark.sql.types.{CharType, DataType, StringType, StructField,
import org.apache.spark.unsafe.types.UTF8String
private[sql] object PartitioningUtils {
+ private val PATTERN_FOR_KEY_EQ_VAL = "(.+)=(.+)".r
Review Comment:
@LuciferYang Thanks for your review, partition name is always followed this rule in Hive [makePartName](https://github.com/apache/hive/blob/1747bc7b8c58d84f455c95f26f1183a65246f331/standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/Warehouse.java#L571).
Partition name is only related to partition keys and values, other partition fields like location will not affect it.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wecharyu commented on pull request #40069: [SPARK-42480][SQL] Improve the performance of drop partitions
Posted by "wecharyu (via GitHub)" <gi...@apache.org>.
wecharyu commented on PR #40069:
URL: https://github.com/apache/spark/pull/40069#issuecomment-1439487785
Addressed comments. @LuciferYang
And gentle ping @wangyum @sunchao: could you also take a look?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #40069: [SPARK-42480][SQL] Improve the performance of drop partitions
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #40069:
URL: https://github.com/apache/spark/pull/40069#discussion_r1110563308
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/util/PartitioningUtils.scala:
##########
@@ -30,6 +30,7 @@ import org.apache.spark.sql.types.{CharType, DataType, StringType, StructField,
import org.apache.spark.unsafe.types.UTF8String
private[sql] object PartitioningUtils {
+ private val PATTERN_FOR_KEY_EQ_VAL = "(.+)=(.+)".r
Review Comment:
cc @wangyum @sunchao FYI
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wecharyu commented on pull request #40069: [SPARK-42480][SQL] Improve the performance of drop partitions
Posted by "wecharyu (via GitHub)" <gi...@apache.org>.
wecharyu commented on PR #40069:
URL: https://github.com/apache/spark/pull/40069#issuecomment-1442242138
Add a conf `spark.sql.hive.dropPartitionByName.enabled` and two tests. cc: @sunchao
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #40069: [SPARK-42480][SQL] Improve the performance of drop partitions
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #40069:
URL: https://github.com/apache/spark/pull/40069#discussion_r1110243153
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/util/PartitioningUtils.scala:
##########
@@ -30,6 +30,7 @@ import org.apache.spark.sql.types.{CharType, DataType, StringType, StructField,
import org.apache.spark.unsafe.types.UTF8String
private[sql] object PartitioningUtils {
+ private val PATTERN_FOR_KEY_EQ_VAL = "(.+)=(.+)".r
Review Comment:
Too idealistic, not all partition tables follow this rule. For example, we can use
`alter table ... partition(...) set location ...` to relocate the partition to any directory.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #40069: [SPARK-42480][SQL] Improve the performance of drop partitions
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #40069:
URL: https://github.com/apache/spark/pull/40069#discussion_r1130233967
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -1227,6 +1227,14 @@ object SQLConf {
.booleanConf
.createWithDefault(false)
+ val HIVE_METASTORE_DROP_PARTITION_BY_NAME =
+ buildConf("spark.sql.hive.dropPartitionByName.enabled")
+ .doc("When true, Spark will get partition name rather than partition object " +
+ "to drop partition, which can improve the performance of drop partition.")
+ .version("3.4.0")
Review Comment:
You can do that if you need this. Otherwise, we need to change this to `3.5.0`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -1227,6 +1227,14 @@ object SQLConf {
.booleanConf
.createWithDefault(false)
+ val HIVE_METASTORE_DROP_PARTITION_BY_NAME =
+ buildConf("spark.sql.hive.dropPartitionByName.enabled")
+ .doc("When true, Spark will get partition name rather than partition object " +
+ "to drop partition, which can improve the performance of drop partition.")
+ .version("3.4.0")
Review Comment:
You can do backporting still if you need this. Otherwise, we need to change this to `3.5.0`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sunchao closed pull request #40069: [SPARK-42480][SQL] Improve the performance of drop partitions
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao closed pull request #40069: [SPARK-42480][SQL] Improve the performance of drop partitions
URL: https://github.com/apache/spark/pull/40069
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #40069: [SPARK-42480][SQL] Improve the performance of drop partitions
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #40069:
URL: https://github.com/apache/spark/pull/40069#discussion_r1130233668
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -1227,6 +1227,14 @@ object SQLConf {
.booleanConf
.createWithDefault(false)
+ val HIVE_METASTORE_DROP_PARTITION_BY_NAME =
+ buildConf("spark.sql.hive.dropPartitionByName.enabled")
+ .doc("When true, Spark will get partition name rather than partition object " +
+ "to drop partition, which can improve the performance of drop partition.")
+ .version("3.4.0")
Review Comment:
Hi, @sunchao . You need to backport this to branch-3.4.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #40069: [SPARK-42480][SQL] Improve the performance of drop partitions
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #40069:
URL: https://github.com/apache/spark/pull/40069#issuecomment-1461082510
Thank youso much, @wecharyu, @sunchao and all!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sunchao commented on pull request #40069: [SPARK-42480][SQL] Improve the performance of drop partitions
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on PR #40069:
URL: https://github.com/apache/spark/pull/40069#issuecomment-1461080621
Merged, thanks!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #40069: [SPARK-42480][SQL] Improve the performance of drop partitions
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #40069:
URL: https://github.com/apache/spark/pull/40069#discussion_r1110563308
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/util/PartitioningUtils.scala:
##########
@@ -30,6 +30,7 @@ import org.apache.spark.sql.types.{CharType, DataType, StringType, StructField,
import org.apache.spark.unsafe.types.UTF8String
private[sql] object PartitioningUtils {
+ private val PATTERN_FOR_KEY_EQ_VAL = "(.+)=(.+)".r
Review Comment:
I remember seeing similar cases in the production environment, but I can't remember the details. Need to have tests to check the corner scenes we can think of
cc @wangyum @sunchao FYI
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org