[GitHub] [spark] c21 commented on a change in pull request #28649: [SPARK-31829] Check for partition existence before computation

[GitBox <gi...@apache.org>]

c21 commented on a change in pull request #28649:
URL: https://github.com/apache/spark/pull/28649#discussion_r462057050



##########
File path: sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/InsertIntoHiveTable.scala
##########
@@ -166,6 +175,17 @@ case class InsertIntoHiveTable(
       if (isDynamic.init.zip(isDynamic.tail).contains((true, false))) {
         throw new AnalysisException(ErrorMsg.PARTITION_DYN_STA_ORDER.getMsg)
       }
+    } else if (numStaticPartitions > 0) {
+      // scalastyle:off
+      // ifNotExists is only valid with static partition, refer to
+      // https://cwiki.apache.org/confluence/display/Hive/LanguageManual+DML#LanguageManualDML-InsertingdataintoHiveTablesfromqueries
+      // scalastyle:on
+      if (!oldPart.isEmpty && ifPartitionNotExists) {
+        val partitionStr = partitionSpec.map(_.productIterator.mkString("=")).mkString("/")
+        logWarning(s"Partition '$partitionStr' already exist, skip running job to overwrite " +

Review comment:
       +1 for adding a warning message here, I feel previous behavior with silently exiting query might be quite confusing for users and developers.
   
   nit: as we could have multiple `INSERT OVERWRITE` in one query, do we also want to include table name (`table.identifier.table`) in warning message?

##########
File path: sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/InsertIntoHiveTable.scala
##########
@@ -148,6 +148,15 @@ case class InsertIntoHiveTable(
            |Table partitions: ${table.partitionColumnNames.mkString(",")}""".stripMargin)
     }
 
+    lazy val oldPart = if ( numDynamicPartitions == 0) {

Review comment:
       +1

##########
File path: sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/InsertIntoHiveTable.scala
##########
@@ -166,6 +175,17 @@ case class InsertIntoHiveTable(
       if (isDynamic.init.zip(isDynamic.tail).contains((true, false))) {
         throw new AnalysisException(ErrorMsg.PARTITION_DYN_STA_ORDER.getMsg)
       }
+    } else if (numStaticPartitions > 0) {
+      // scalastyle:off
+      // ifNotExists is only valid with static partition, refer to
+      // https://cwiki.apache.org/confluence/display/Hive/LanguageManual+DML#LanguageManualDML-InsertingdataintoHiveTablesfromqueries
+      // scalastyle:on
+      if (!oldPart.isEmpty && ifPartitionNotExists) {

Review comment:
       nit: `if (oldPart.isDefined && ifPartitionNotExists)`




----------------------------------------------------------------
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.

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