DeltaCatalog.createTable should respect PROP_IS_MANAGED_LOCATION #3654
Conversation
spark/src/main/scala/org/apache/spark/sql/delta/catalog/DeltaCatalog.scala
Show resolved
Hide resolved
spark/src/test/scala/org/apache/spark/sql/delta/CustomCatalogSuite.scala
Show resolved
Hide resolved
| @@ -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)) | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
+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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Anyway, this is not an issue for now as neither HMS nor UC generate location for external tables.
There was a problem hiding this comment.
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.
| if (location.isDefined) CatalogTableType.EXTERNAL else CatalogTableType.MANAGED | ||
| val isManagedLocation = Option(allTableProperties.get(TableCatalog.PROP_IS_MANAGED_LOCATION)) | ||
| .exists(_.equalsIgnoreCase("true")) | ||
| val tableType = if (location.isEmpty || isManagedLocation) { |
There was a problem hiding this comment.
What will be the impact of this PR with non-UC catalogs like HMS if we backport this to branch 3.2?
There was a problem hiding this comment.
or should we make it safer to backport by using the isUnityCatalog flag here as well?
I think its better we use the flag.
|
Approving and merging the PR to speed up backporting to 3.2. |
…ta-io#3654) <!-- Thanks for sending a pull request! Here are some tips for you: 1. If this is your first time, please read our contributor guidelines: https://github.com/delta-io/delta/blob/master/CONTRIBUTING.md 2. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP] Your PR title ...'. 3. Be sure to keep the PR description updated to reflect all changes. 4. Please write your PR title to summarize what this PR proposes. 5. If possible, provide a concise example to reproduce the issue for a faster review. 6. If applicable, include the corresponding issue number in the PR title and link it in the body. --> #### Which Delta project/connector is this regarding? <!-- Please add the component selected below to the beginning of the pull request title For example: [Spark] Title of my pull request --> - [x] Spark - [ ] Standalone - [ ] Flink - [ ] Kernel - [ ] Other (fill in here) ## Description <!-- - Describe what this PR changes. - Describe why we need the change. If this PR resolves an issue be sure to include "Resolves #XXX" to correctly link and close the issue upon merge. --> Even if a table has the location field, it should still be a managed table if `PROP_IS_MANAGED_LOCATION` is present in the table properties. Note: this case won't happen with Spark integration solely. It's only an issue for third-party catalogs that delegate requests to `DeltaCatalog`, such as Unity Catalog. ## How was this patch tested? <!-- If tests were added, say they were added here. Please make sure to test the changes thoroughly including negative and positive cases if possible. If the changes were tested in any way other than unit tests, please clarify how you tested step by step (ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future). If the changes were not tested, please explain why. --> new test ## Does this PR introduce _any_ user-facing changes? <!-- If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible. If possible, please also clarify if this is a user-facing change compared to the released Delta Lake versions or within the unreleased branches such as master. If no, write 'No'. --> no
Which Delta project/connector is this regarding?
Description
Even if a table has the location field, it should still be a managed table if
PROP_IS_MANAGED_LOCATIONis present in the table properties.Note: this case won't happen with Spark integration solely. It's only an issue for third-party catalogs that delegate requests to
DeltaCatalog, such as Unity Catalog.How was this patch tested?
new test
Does this PR introduce any user-facing changes?
no