Skip to content

[SPARK-37878][SQL][FOLLOWUP] V1Table should always carry the "location" property #36498

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cloud-fan wants to merge 1 commit into from
Closed

[SPARK-37878][SQL][FOLLOWUP] V1Table should always carry the "location" property #36498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ public interface TableCatalog extends CatalogPlugin {
*/
String PROP_LOCATION = "location";

/**
* A reserved property to indicate that the table location is managed, not user-specified.
* If this property is "true", SHOW CREATE TABLE will not generate the LOCATION clause.
*/
String PROP_IS_MANAGED_LOCATION = "is_managed_location";
Copy link
Contributor

@Peng-Lei Peng-Lei May 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit :
actually, we can add a reserved property named PROP_TABLE_TYPE, because type include EXTERNAL MANAGED VIEW that also can control the different behaviors when TYPE == MANAGED. And PROP_TABLE_TYPE sounds more generic, may be we can add more type in future.

Copy link
Contributor Author

@cloud-fan cloud-fan May 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we will use TableCatalog to support views. I'm adding this new property as I think this is the most precise way. People can create EXTERNAL table without location, or create MANAGED TABLE with location. What we care in SHOW CREATE TABLE is if the location is generated by the catalog or not, instead of the table type.


/**
* A reserved property to specify a table was created with EXTERNAL.
*/
Expand Down
12 changes: 10 additions & 2 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ import org.apache.spark.sql.catalyst.plans.logical._
import org.apache.spark.sql.catalyst.trees.CurrentOrigin
import org.apache.spark.sql.catalyst.util.{CharVarcharUtils, DateTimeUtils, IntervalUtils, ResolveDefaultColumns}
import org.apache.spark.sql.catalyst.util.DateTimeUtils.{convertSpecialDate, convertSpecialTimestamp, convertSpecialTimestampNTZ, getZoneId, stringToDate, stringToTimestamp, stringToTimestampWithoutTimeZone}
import org.apache.spark.sql.connector.catalog.{SupportsNamespaces, TableCatalog}
import org.apache.spark.sql.connector.catalog.{CatalogV2Util, SupportsNamespaces, TableCatalog}
import org.apache.spark.sql.connector.catalog.TableChange.ColumnPosition
import org.apache.spark.sql.connector.expressions.{ApplyTransform, BucketTransform, DaysTransform, Expression => V2Expression, FieldReference, HoursTransform, IdentityTransform, LiteralValue, MonthsTransform, Transform, YearsTransform}
import org.apache.spark.sql.errors.QueryParsingErrors
Expand Down Expand Up @@ -3288,7 +3288,15 @@ class AstBuilder extends SqlBaseParserBaseVisitor[AnyRef] with SQLConfHelper wit
throw QueryParsingErrors.cannotCleanReservedTablePropertyError(
PROP_EXTERNAL, ctx, "please use CREATE EXTERNAL TABLE")
case (PROP_EXTERNAL, _) => false
case _ => true
// It's safe to set whatever table comment, so we don't make it a reserved table property.
case (PROP_COMMENT, _) => true
case (k, _) =>
Copy link
Contributor

@Peng-Lei Peng-Lei May 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is duplicate with case before. Just leave code you added is simple and clear.

Copy link
Contributor Author

@cloud-fan cloud-fan May 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need the code above to throw errors with a better error message, which gives suggestions like please use CREATE EXTERNAL TABLE

Copy link
Contributor

@huaxingao huaxingao May 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's where I got confused too. If we want to keep the code above to issue a better error message, why not adding something in parallel such as

case (PROP_IS_MANAGED_LOCATION, _) if !legacyOn =>
    throw QueryParsingErrors.cannotCleanReservedTablePropertyError(
       PROP_IS_MANAGED_LOCATION, ctx, "xxxxxx")
case (PROP_IS_MANAGED_LOCATION, _) =>
    false

Copy link
Contributor Author

@cloud-fan cloud-fan May 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may add more reserved properties in the future that the suggestion can only be please remove it from the TBLPROPERTIES list, so I wrote the code this way to ease the work of adding new reserved properties.

Copy link
Contributor

@huaxingao huaxingao May 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Thanks for the explanation.

val isReserved = CatalogV2Util.TABLE_RESERVED_PROPERTIES.contains(k)
if (!legacyOn && isReserved) {
throw QueryParsingErrors.cannotCleanReservedTablePropertyError(
k, ctx, "please remove it from the TBLPROPERTIES list.")
}
!isReserved
}
}

Expand Down
3 changes: 2 additions & 1 deletion sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ private[sql] object CatalogV2Util {
TableCatalog.PROP_LOCATION,
TableCatalog.PROP_PROVIDER,
TableCatalog.PROP_OWNER,
TableCatalog.PROP_EXTERNAL)
TableCatalog.PROP_EXTERNAL,
TableCatalog.PROP_IS_MANAGED_LOCATION)

/**
* The list of reserved namespace properties, which can not be removed or changed directly by
Expand Down
6 changes: 3 additions & 3 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/V1Table.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,15 +74,15 @@ private[sql] case class V1Table(v1Table: CatalogTable) extends Table {
private[sql] object V1Table {
def addV2TableProperties(v1Table: CatalogTable): Map[String, String] = {
val external = v1Table.tableType == CatalogTableType.EXTERNAL
val managed = v1Table.tableType == CatalogTableType.MANAGED

v1Table.properties ++
v1Table.storage.properties.map { case (key, value) =>
TableCatalog.OPTION_PREFIX + key -> value } ++
v1Table.provider.map(TableCatalog.PROP_PROVIDER -> _) ++
v1Table.comment.map(TableCatalog.PROP_COMMENT -> _) ++
(if (external) {
v1Table.storage.locationUri.map(TableCatalog.PROP_LOCATION -> _.toString)
} else None) ++
v1Table.storage.locationUri.map(TableCatalog.PROP_LOCATION -> _.toString) ++
(if (managed) Some(TableCatalog.PROP_IS_MANAGED_LOCATION -> "true") else None) ++
(if (external) Some(TableCatalog.PROP_EXTERNAL -> "true") else None) ++
Some(TableCatalog.PROP_OWNER -> v1Table.owner)
}
Expand Down
10 changes: 7 additions & 3 deletions ...re/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ShowCreateTableExec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,13 @@ case class ShowCreateTableExec(
}

private def showTableLocation(table: Table, builder: StringBuilder): Unit = {
Option(table.properties.get(TableCatalog.PROP_LOCATION))
.map("LOCATION '" + escapeSingleQuotedString(_) + "'\n")
.foreach(builder.append)
val isManagedOption = Option(table.properties.get(TableCatalog.PROP_IS_MANAGED_LOCATION))
// Only generate LOCATION clause if it's not managed.
if (isManagedOption.forall(_.equalsIgnoreCase("false"))) {
Option(table.properties.get(TableCatalog.PROP_LOCATION))
.map("LOCATION '" + escapeSingleQuotedString(_) + "'\n")
.foreach(builder.append)
}
}

private def showTableProperties(
Expand Down