cloud-fan commented on a change in pull request #28026:
URL: https://github.com/apache/spark/pull/28026#discussion_r513448543
##########
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:
Different people may have a different understanding of when it's
necessary to split a PR. I don't have a big problem with it. Again I was only
asking for individual JIRA tickets. One idea is to make an umbrella JIRA with 4
sub-tasks:
- Have a unified CREATE TABLE syntax that supports v1/v2 catalog
- sub-task 1: merge the native and Hive-style CREATE TABLE SQL syntax
(need to explain the merged syntax, e.g. USING and STORED AS/BY can't coexist,
PARTITION BY column name with data type and PARTITION BY transforms can't
coexist)
- sub-task 2: convert serde info to v2 table properties (explain the rules)
- sub-task 3: add EXTERNAL table property to indicate CREATE EXTERNAL TABLE
- sub-task 4: add `options.` prefix for table properties specified in
`OPTIONS ...` (explain that we duplicate `OPTIONS ...` in table properties
without `option.` prefix for backward compatibility. this probably need some
discussion.)
Personally I don't really see how sub-task 4 is related to CREATE TABLE
unification. Maybe we should put it as a separated JIRA ticket.
Anyway I'll start detailed code review this week.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
