Add support for SET DEFAULT and DROP DEFAULT statements#26162
Conversation
5dce2c0 to
5f15ffb
Compare
71301fb to
cb68fd6
Compare
| if (columnMetadata.isHidden()) { | ||
| throw semanticException(NOT_SUPPORTED, statement, "Cannot modify hidden column"); | ||
| } | ||
| if (columnMetadata.getDefaultValue().isEmpty()) { | ||
| throw semanticException(NOT_SUPPORTED, statement, "Column '%s' does not have a default value", column); | ||
| } | ||
| metadata.dropDefaultValue(session, tableHandle, columnHandle); |
There was a problem hiding this comment.
Like we do for SetDefaultValueTask - can we also check if the Connector supports them out ?
| if (redirectionAwareTableHandle.tableHandle().isEmpty()) { | ||
| String exceptionMessage = "Table '%s' does not exist".formatted(tableName); | ||
| if (metadata.getMaterializedView(session, tableName).isPresent()) { | ||
| exceptionMessage += ", but a materialized view with that name exists."; | ||
| } | ||
| else if (metadata.isView(session, tableName)) { | ||
| exceptionMessage += ", but a view with that name exists."; | ||
| } | ||
| if (!statement.isTableExists()) { | ||
| throw semanticException(TABLE_NOT_FOUND, statement, "%s", exceptionMessage); | ||
| } | ||
| return immediateVoidFuture(); | ||
| } |
There was a problem hiding this comment.
Maybe as a follow-up - Do we need to abstract this out for all the DDL operations to be perfomed on a table ?
| | ALTER TABLE (IF EXISTS)? tableName=qualifiedName | ||
| ALTER COLUMN columnName=identifier SET DEFAULT literal #setDefaultValue | ||
| | ALTER TABLE (IF EXISTS)? tableName=qualifiedName | ||
| ALTER COLUMN columnName=identifier DROP DEFAULT #dropDefaultValue |
There was a problem hiding this comment.
Do we have any plans on setting the same for field as well ?
There was a problem hiding this comment.
There is no plan about it as far as I know.
cb68fd6 to
be71180
Compare
|
This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack. |
be71180 to
223dff2
Compare
|
This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack. |
17bfa50 to
d68f292
Compare
kasiafi
left a comment
There was a problem hiding this comment.
Reviewed.
By the way, I noticed that when we alter column type, we don't do any generic checks related to the default value. What does the spec say about it?
d68f292 to
4fc218e
Compare
4fc218e to
e0de208
Compare
|
The spec doesn't explicitly mention checking default values in |
e0de208 to
a9591b6
Compare
kasiafi
left a comment
There was a problem hiding this comment.
The spec doesn't explicitly mention checking default values in 11.19 , if I understand it correctly.
Right. Instead, the spec defines constraints on the new type based on the existing type. If those constraints are respected, then probably it guarantees that the default value remains valid.
However, we don't enforce these constraints in the engine, so the consistency here depends on the connector behavior.
@martint is this something we should revisit?
| throw semanticException(NOT_SUPPORTED, statement, "Cannot modify hidden column"); | ||
| } | ||
| if (columnMetadata.getDefaultValue().isEmpty()) { | ||
| throw semanticException(GENERIC_USER_ERROR, statement, "Column '%s' does not have a default value", columnName); |
There was a problem hiding this comment.
You can add a new error code for this case.
|
This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack. |
a9591b6 to
66d0138
Compare
|
This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack. |
66d0138 to
75e1604
Compare
|
Syntax looks good. |
75e1604 to
e29ef5c
Compare
|
(Squashed a fixup commit) |
Description
Add support for the following syntax:
DROP DEFAULTon columns that don't have defaults throws an exception because SQL spec states "The descriptor of C shall include a default value".Release notes