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

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

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



##########
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:
       I'm not sure Separate PR's really makes sense since this seems to be coherent whole to me, but if we need more Jira's just for change tracking that seems fine although I'm not sure how much use that is? Is the idea that it will be easier to use JIRA search to find when the changes occurred because I usually find tracking back from the commit to the Jira much easier than going from Jira -> the code. We also rarely have Jira's as updated as the PR's and commits. 




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