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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
