Skip to content
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

DeltaCatalog.createTable should respect PROP_IS_MANAGED_LOCATION #3654

Merged
merged 4 commits into from
Sep 10, 2024
Merged

DeltaCatalog.createTable should respect PROP_IS_MANAGED_LOCATION #3654

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
89c5783
DeltaCatalog.createTable should respect PROP_IS_MANAGED_LOCATION
cloud-fan Sep 9, 2024
835b618
address comments
cloud-fan Sep 10, 2024
0c0855c
fix test
cloud-fan Sep 10, 2024
2b41714
Merge branch 'master' into fix
cloud-fan Sep 10, 2024
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
9 changes: 7 additions & 2 deletions spark/src/main/scala/org/apache/spark/sql/delta/catalog/DeltaCatalog.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,13 @@ class DeltaCatalog extends DelegatingCatalogExtension
.getOrElse(spark.sessionState.catalog.defaultTablePath(id))
val storage = DataSource.buildStorageFormatFromOptions(writeOptions)
.copy(locationUri = Option(loc))
val tableType =
if (location.isDefined) CatalogTableType.EXTERNAL else CatalogTableType.MANAGED
val isManagedLocation = Option(allTableProperties.get(TableCatalog.PROP_IS_MANAGED_LOCATION))
Copy link
Collaborator

Choose a reason for hiding this comment

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

TableCatalog also seems to have PROP_EXTERNAL ? Is this also serving the same purpose? What happens when both PROP_EXTERNAL and PROP_IS_MANAGED_LOCATION are set to true?

  /**
   * A reserved property to specify a table was created with EXTERNAL.
   */
  String PROP_EXTERNAL = "external";

Since we are making DeltaCatalog aware of the TableCatalog.PROP_IS_MANAGED_LOCATION, should we also make it aware of PROP_EXTERNAL for the sake of completeness in this PR itself?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

I also think we should add asserts and checks here so that UC-Spark catalog never sets these mutually exclusive properly inconsistently.

same comments i made in the linked PR - https://github.com/unitycatalog/unitycatalog/pull/449/files#r1750747701

Copy link
Contributor Author

@cloud-fan cloud-fan Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The PROP_EXTERNAL should only be true if the CREATE TABLE command has the keyword EXTERNAL specified. For example, CREATE EXTERNAL TABLE t .... It should not be true even if the keyword LOCATION is specified like CREATE TABLE t ... LOCATION ..., although most catalogs will create this table as external table since it has a user-specified location.

On the other hand, PROP_IS_MANAGED_LOCATION is used to indicate that the location is not user-specified by managed by the system.

That said, these two properties are not mutually exclusive. In fact, DeltaCatalog does not look at the PROP_EXTERNAL property at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also see unitycatalog/unitycatalog#449 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

so its possible that CREATE EXTERNAL TABLE ... <no location specified> command can produce PROP_EXTERNAL = true and PROP_IS_MANAGED_LOCATION=true if the Spark custom catalog calling into DeltaCatalog decides to generate a location even for an external table... right?

And in that case, this code will produce a managed table, even though the SQL command explicitly asked for create external table... right?

That's what I dont like about this overall setup. Yes, you are right they are not mutually exclusive, but setting both of them to true, while possible, is very very ambiguous.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is upto the catalog impl to decide how they want to proceed in such scenario? Can we add an assertion in Delta to fail in such a scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Delta is more like a proxy and it will eventually forward the request to HMS/UC or whatever custom catalog. I think it's better to leave it to the underlying catalog to enforce it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyway, this is not an issue for now as neither HMS nor UC generate location for external tables.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is fine for now. Thats why i merged the PR. But it always good to think of future proofing things so that there are no surprises in the future when somebody also loads DeltaCatalog with other catalogs.

.exists(_.equalsIgnoreCase("true"))
val tableType = if (location.isEmpty || isManagedLocation) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What will be the impact of this PR with non-UC catalogs like HMS if we backport this to branch 3.2?

Copy link
Contributor

Choose a reason for hiding this comment

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

or should we make it safer to backport by using the isUnityCatalog flag here as well?

I think its better we use the flag.

CatalogTableType.MANAGED
} else {
CatalogTableType.EXTERNAL
}
val commentOpt = Option(allTableProperties.get("comment"))


Expand Down
32 changes: 29 additions & 3 deletions spark/src/test/scala/org/apache/spark/sql/delta/CustomCatalogSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,22 @@ class CustomCatalogSuite extends QueryTest with SharedSparkSession
}
}
}

test("custom catalog that generates location for managed tables") {
// Reset catalog manager so that the new `spark_catalog` implementation can apply.
spark.sessionState.catalogManager.reset()
withSQLConf("spark.sql.catalog.spark_catalog" -> classOf[DummySessionCatalog].getName) {
withTable("t") {
withTempPath { path =>
sql(s"CREATE TABLE t (id LONG) USING delta TBLPROPERTIES (fakeLoc='$path')")
val t = spark.sessionState.catalogManager.v2SessionCatalog.asInstanceOf[TableCatalog]
.loadTable(Identifier.of(Array("default"), "t"))
// It should be a managed table.
assert(!t.properties().containsKey(TableCatalog.PROP_EXTERNAL))
}
}
}
}
}

class DummyCatalog extends TableCatalog {
Expand Down Expand Up @@ -397,9 +413,10 @@ class DummySessionCatalogInner extends DelegatingCatalogExtension {
}

// A dummy catalog that adds a layer between DeltaCatalog and the Spark SessionCatalog,
// to attach additional table storage properties after the table is loaded.
// to attach additional table storage properties after the table is loaded, and generates location
// for managed tables.
class DummySessionCatalog extends TableCatalog {
private var deltaCatalog: DelegatingCatalogExtension = null
private var deltaCatalog: DeltaCatalog = null

override def initialize(name: String, options: CaseInsensitiveStringMap): Unit = {
val inner = new DummySessionCatalogInner()
Expand All @@ -422,7 +439,16 @@ class DummySessionCatalog extends TableCatalog {
schema: StructType,
partitions: Array[Transform],
properties: java.util.Map[String, String]): Table = {
deltaCatalog.createTable(ident, schema, partitions, properties)
if (!properties.containsKey(TableCatalog.PROP_EXTERNAL) &&
!properties.containsKey(TableCatalog.PROP_LOCATION)) {
val newProps = new java.util.HashMap[String, String]
newProps.putAll(properties)
newProps.put(TableCatalog.PROP_LOCATION, properties.get("fakeLoc"))
newProps.put(TableCatalog.PROP_IS_MANAGED_LOCATION, "true")
deltaCatalog.createTable(ident, schema, partitions, newProps)
} else {
deltaCatalog.createTable(ident, schema, partitions, properties)
}
}

override def alterTable(ident: Identifier, changes: TableChange*): Table = {
Expand Down
Loading