Remove ON DELETE CASCADE #623
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gtrettenero
force-pushed
the
dbCascade
branch
3 times, most recently
from
10e3a71 to
8950237
Compare
fix checkstyle fix checkstyle see if the test error is due to schema change check tests check tests checkstyle test test test test test test test test hopefully last test checkstyle already exists checkstyle test move test fix moved test fix test final
yingjianwu98
reviewed
May 7, 2025
|
|
||
| Assertions.assertThrows(DataIntegrityViolationException.class, () -> | ||
| polarisConnector.deleteDatabase(dbName)); | ||
| Assert.assertTrue(polarisConnector.databaseExists(dbName)); |
There was a problem hiding this comment.
nit: Could you also add a check to make sure the table exist as well?
yingjianwu98
reviewed
May 7, 2025
| throw new InvalidMetaException(name, exception); | ||
| } catch (Exception exception) { | ||
| throw new ConnectorException( | ||
| String.format("Failed deleting polaris database %s", name), exception); | ||
| } | ||
| System.out.println("DID NOT CATCH THE DB DELETE ERROR"); |
There was a problem hiding this comment.
nit: should we get rid of this?
ldsantos0911
reviewed
May 7, 2025
| @@ -25,5 +25,5 @@ create table TBLS ( | |||
| created_date TIMESTAMP not null, | |||
| last_updated_by STRING(255), | |||
| last_updated_date TIMESTAMP not null, | |||
| foreign key (db_name) references DBS(name) ON DELETE CASCADE ON UPDATE CASCADE | |||
| foreign key (db_name) references DBS(name) ON DELETE RESTRICT ON UPDATE CASCADE | |||
There was a problem hiding this comment.
Do we want cascading updates? dbname -> dbname2 would also be bad. Less bad, but still bad.
| public DatabasePreconditionFailedException(final QualifiedName name, | ||
| @Nullable final String message, | ||
| @Nullable final Throwable error) { | ||
| super(String.format("Precondition failed to update table %s. %s", name, message), error); |
There was a problem hiding this comment.
nit: I think this should be update database?
| /** | ||
| * Test database deletion if table exists and ON DELETE CASCADE is disabled. | ||
| */ | ||
| @Test |
There was a problem hiding this comment.
It would be good to have a test at the PolarisConnectorDatabaseService level as well.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.