-
Notifications
You must be signed in to change notification settings - Fork 2k
Allow CREATE OR REPLACE with CatalogManaged if not changing commit coordinator #5889
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.
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
base: master
Are you sure you want to change the base?
Conversation
yumingxuanguo-db
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
tdas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there does not seem to any unit tests. can we add a unit tests to ensure that this actually works?
for example, we now have full end-to-end UC tests using the UC server running in the JVM - https://github.com/delta-io/delta/tree/master/spark/unitycatalog/src/test/java/io/sparkuctest
can you make sure that there is test coverage here that ensures that this works end to end?
|
|
||
| // Allow specifying CatalogManaged in REPLACE TABLE if the existing table is already | ||
| // CatalogManaged. In this case, the commit coordinator properties are treated as a no-op. | ||
| // Block if trying to enable CatalogManaged on a non-CatalogManaged table via REPLACE. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @xzhseh , seems like you are adding an enforced checking to prevent REPLACE an existing non-catalogManaged table to be a managed one.
I will suggest to add an integration tests to cover the changes you made. https://github.com/delta-io/delta/blob/master/spark/unitycatalog/src/test/java/io/sparkuctest/UCDeltaTableCreationTest.java
Which Delta project/connector is this regarding?
Description
As titled.
How was this patch tested?
UTs.
Does this PR introduce any user-facing changes?
Yes.