-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add support for NOT NULL in DDL statements #418
Conversation
@findepi I already reviewed this in the other repo. Let me know if you'd like to take a look, otherwise I'll merge this. |
c064bb8
to
6751471
Compare
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.
Have you changed anything other than related to rebase?
presto-parser/src/main/java/io/prestosql/sql/testing/TreeAssertions.java
Show resolved
Hide resolved
presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/BaseJdbcClient.java
Outdated
Show resolved
Hide resolved
public void renameTable(JdbcIdentity identity, JdbcTableHandle handle, SchemaTableName newTableName) | ||
{ | ||
// MySQL doesn't support specifying the catalog name in a rename. By setting the | ||
// catalogName parameter to null, it will be omitted in the ALTER TABLE statement. |
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.
we should checkArgument(handle.getSchemaName() eq newTableName.getSchema())
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.
Perhaps I'm not following, but they don't have to be the same, as it's legal to rename a table across databases/schemas.
presto-mysql/src/test/java/io/prestosql/plugin/mysql/TestMySqlTypeMapping.java
Show resolved
Hide resolved
I don't like how the "Add DDL support for JDBC connectors" commit contains several changes that are not directly related. |
@electrum i only skimmed. LGTM. |
@kokosing The only change was I moved |
This requires mysql-connector-java to be updated.
- create table, add column, and drop column for everything - rename column and rename table for MySQL and PostgreSQL Extracted-From: https://github.com/prestodb/presto
Enable NOT NULL in the parser and add a flag in the metadata to check whether or not a connector supports this new functionality. If a connector doesn't support NOT NULL semantics in columns, yet this is supplied, then a semantic exception is thrown indicating to the user that this functionality is not supported in their catalog. Extracted-From: https://github.com/prestodb/presto
No description provided.