[GitHub] [spark] HyukjinKwon commented on a change in pull request #28026: [SPARK-31257][SQL] Unify create table syntax

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

HyukjinKwon commented on a change in pull request #28026:
URL: https://github.com/apache/spark/pull/28026#discussion_r513197469



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala
##########
@@ -295,18 +295,61 @@ private[sql] object CatalogV2Util {
     catalog.name().equalsIgnoreCase(CatalogManager.SESSION_CATALOG_NAME)
   }
 
-  def convertTableProperties(
+  def convertTableProperties(c: CreateTableStatement): Map[String, String] = {
+    convertTableProperties(
+      c.properties, c.options, c.serde, c.location, c.comment, c.provider, c.external)
+  }
+
+  def convertTableProperties(c: CreateTableAsSelectStatement): Map[String, String] = {
+    convertTableProperties(
+      c.properties, c.options, c.serde, c.location, c.comment, c.provider, c.external)
+  }
+
+  def convertTableProperties(r: ReplaceTableStatement): Map[String, String] = {
+    convertTableProperties(r.properties, r.options, r.serde, r.location, r.comment, r.provider)
+  }
+
+  def convertTableProperties(r: ReplaceTableAsSelectStatement): Map[String, String] = {
+    convertTableProperties(r.properties, r.options, r.serde, r.location, r.comment, r.provider)
+  }
+
+  private def convertTableProperties(
       properties: Map[String, String],
       options: Map[String, String],
+      serdeInfo: Option[SerdeInfo],
       location: Option[String],
       comment: Option[String],
-      provider: Option[String]): Map[String, String] = {
-    properties ++ options ++
+      provider: Option[String],
+      external: Boolean = false): Map[String, String] = {
+    properties ++
+      options ++ // to make the transition to the "option." prefix easier, add both
+      options.map { case (key, value) => TableCatalog.OPTION_PREFIX + key -> value } ++
+      convertToProperties(serdeInfo) ++
+      (if (external) Map(TableCatalog.PROP_EXTERNAL -> "true") else Map.empty) ++

Review comment:
       Making separate JIRAs and PRs:
   
   - make it easier to port/revert/manage, e.g.) reverting a specific change.
   - easier to track the discussions specific to smaller scope, e.g.) the current discussion thread looks already very long to read
   - +1653 and −1125 change is large. It makes easily people to miss key things during a review.
   - this `CREATE TABLE` syntax is pretty much an entry point of users. Here is where we need more calls for review. I would encourage to make it easier to review so many people can review closely to prevent any surprise by simply overlooking.
   
   If I am not mistaken, it is usually considered as a good practice to split if that can logically be split. I also encourage to split PRs in general. If we should not split, I think the argument should be about why it should not be split (instead of why it should be split).
   




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