-
Notifications
You must be signed in to change notification settings - Fork 13.9k
[FLINK-38856][table] Add support for ALTER MATERIALIZED TABLE DROP schema component
#27381
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
Conversation
…zedtable` package
…ction`, `DropFunction` to dedicated converters
| @@ -315,182 +264,56 @@ | |||
| } else if (validated instanceof SqlCompileAndExecutePlan) { | |||
| return Optional.of( | |||
| converter.convertCompileAndExecutePlan((SqlCompileAndExecutePlan) validated)); | |||
| } else if (validated instanceof SqlAnalyzeTable) { | |||
| return Optional.of(converter.convertAnalyzeTable((SqlAnalyzeTable) validated)); | |||
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.
all of these moved into separate converter classes
it allowed to make this class less than 1k lines
| if (!identifier.isSimple()) { | ||
| throw new UnsupportedOperationException( | ||
| String.format( | ||
| "%sAlter nested row type %s is not supported yet.", |
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.
nit: from what I can see the exMsgPrefix comes from the TableKind enum. So would read something like
MATERIALIZED TABLEAlter nested row ....
I wonder if it would read better as Alter %s nested row ...
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.
nit: from what I can see the exMsgPrefix comes from the TableKind enum. So would read something like
MATERIALIZED_TABLEAlter nested row ....
This is not true, it comes from https://github.com/snuyanzin/flink/blob/44838279dd0575d02c308b86ce752a87db86a9ee/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/materializedtable/AbstractAlterMaterializedTableConverter.java#L41-L42
TableKind enum is the second arg
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.
Ok thanks - my bad. I saw
Line 82 in 94c6549
| this.exMsgPrefix = String.format(ERROR_TEMPLATE, tableKindStr.toUpperCase(Locale.ROOT)); |
| if (oldTable.getResolvedSchema().getWatermarkSpecs().isEmpty()) { | ||
| throw new ValidationException( | ||
| String.format( | ||
| "%sThe current %s does not define any watermark strategy.", |
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.
similar comment . It will read strangely without a space before the first The
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.
as mentioned above, there always will be \n
| .isInstanceOf(ValidationException.class) | ||
| .hasMessageContaining( | ||
| "The base table does not define a primary key constraint named 'ct2'. Available constraint name: ['ct1']."); | ||
| "The current table does not define a primary key constraint named 'ct2'. Available constraint name: ['ct1']."); |
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.
I suggest including the the message prefix in these tests - so we can see it reads well.
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.
added
ideally it should be parameterized and refactored, however this PR is already large, so probably in a separate one
| } | ||
| | | ||
| <DROP> <DISTRIBUTION> { | ||
| <DROP> |
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.
I like this rationalisation of the grammar. Can we update the docs to match it?
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.
I'm going to submit a separate PR for docs which should cover FLIP-550 (CREATE, ALTER, DROP)
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.
| assertThatThrownBy(() -> parse("alter table tb1 drop primary key")) | ||
| .isInstanceOf(ValidationException.class) | ||
| .hasMessageContaining("The base table does not define any primary key."); | ||
| .hasMessageContaining("The current table does not define any primary key."); |
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.
Following same approach as suggested at #27302 (comment)
twalthr
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.
Thanks for all these massive refactorings. Very valuable!
.../flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/table/SqlAlterTableDrop.java
Outdated
Show resolved
Hide resolved
.../flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/table/SqlAlterTableDrop.java
Outdated
Show resolved
Hide resolved
...r/src/main/java/org/apache/flink/table/planner/operations/converters/MergeTableLikeUtil.java
Show resolved
Hide resolved
| UnresolvedIdentifier unresolvedIdentifier = | ||
| UnresolvedIdentifier.of(sqlDropFunction.getFullName()); | ||
| if (sqlDropFunction.isSystemFunction()) { | ||
| return new DropTempSystemFunctionOperation( |
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.
nit: Interesting that we don't check for isTemporary here. I hope the parser catches this.
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.
looks like there is nothing in parser for that...
Test shows it fails in FunctionCatalog where there is a validation
Lines 134 to 146 in b597afa
| public boolean dropTemporarySystemFunction(String name, boolean ignoreIfNotExist) { | |
| final String normalizedName = FunctionIdentifier.normalizeName(name); | |
| final CatalogFunction function = tempSystemFunctions.remove(normalizedName); | |
| if (function == null && !ignoreIfNotExist) { | |
| throw new ValidationException( | |
| String.format( | |
| "Could not drop temporary system function. A function named '%s' doesn't exist.", | |
| name)); | |
| } | |
| unregisterFunctionJarResources(function); | |
| return function != null; |
.../src/test/java/org/apache/flink/table/planner/operations/SqlDdlToOperationConverterTest.java
Outdated
Show resolved
Hide resolved
.../apache/flink/table/planner/operations/SqlMaterializedTableNodeToOperationConverterTest.java
Outdated
Show resolved
Hide resolved
| throw new UnsupportedOperationException( | ||
| String.format( | ||
| "%sAltering the nested row type `%s` is not supported yet.", | ||
| exMsgPrefix, String.join("`.`", identifier.names))); |
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.
Ideally quoting requires dialect which could be derived with help of planner.
In order to not make it complicated continue without this since this is only a message for Exception
|
@flinkbot run azure |
What is the purpose of the change
The PR implements
ALTER MATERIALIZED TABLE ... DROPpart of FLIP-550 likeBesides that it also refactors a bit
SqlNodeToOperationConversionby extraction of function and analyze table functionality into dedicated converters.Verifying this change
There are new tests for failed and success cases, also existing tests
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation