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
[FLINK-19883][table] Support 'IF EXISTS' in DDL for ALTER TABLE #17155
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit a89b8c4 (Mon Sep 06 05:43:28 UTC 2021) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
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 picking this up. I gave this a first round of review! :-)
...-table-api-java/src/main/java/org/apache/flink/table/operations/ddl/AlterTableOperation.java
Outdated
Show resolved
Hide resolved
...api-java/src/main/java/org/apache/flink/table/operations/ddl/AlterTableOptionsOperation.java
Outdated
Show resolved
Hide resolved
...planner/src/main/java/org/apache/flink/table/planner/operations/SqlToOperationConverter.java
Outdated
Show resolved
Hide resolved
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/SqlAlterTable.java
Outdated
Show resolved
Hide resolved
...able/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/SqlAlterTableRename.java
Outdated
Show resolved
Hide resolved
...a/src/main/java/org/apache/flink/table/operations/ddl/AlterTableDropConstraintOperation.java
Outdated
Show resolved
Hide resolved
...api-java/src/main/java/org/apache/flink/table/operations/ddl/AlterTableOptionsOperation.java
Show resolved
Hide resolved
...-api-java/src/main/java/org/apache/flink/table/operations/ddl/AlterTableRenameOperation.java
Show resolved
Hide resolved
...-api-java/src/main/java/org/apache/flink/table/operations/ddl/AlterTableRenameOperation.java
Outdated
Show resolved
Hide resolved
...table-planner/src/test/scala/org/apache/flink/table/planner/catalog/CatalogTableITCase.scala
Outdated
Show resolved
Hide resolved
@wuchong @Airblader , I have addressed the comments mentioned above. Please help to review again. |
...-table/flink-table-api-java/src/main/java/org/apache/flink/table/catalog/CatalogManager.java
Outdated
Show resolved
Hide resolved
public class AlterTableOptionsOperation extends AlterTableOperation { | ||
private final CatalogTable catalogTable; | ||
private final Map<String, String> tableOptions; |
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 still not really happy with this; now we added redundant information for the sake of the summary string, but this information isn't actually used by CatalogManager
to alter the table.
In principle I think this class shouldn't hold a CatalogTable
at all, but rather only an identifier (which it already does from its superclass) and the new options to be added. TableEnvironmentImpl#executeInternal
then just looks up the table when needed (as it already does for some other operations).
Happy to hear someone else's thoughts on this as 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.
@Airblader , the CatalogTable
isn't hold only by AlterTableOptionsOperation
. Other operations like AlterTableSchemaOperation
also hold the CatalogTable
.
IMO, the converter between the SQL and operation should not pass the CatalogTable
. The CatalogTable
is looked up when calling TableEnvironmentImpl#executeInternal
. But the changes could be pushed another PR to unify for updating.
cc @wuchong
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.
Yes, other operations do that, too, and probably shouldn't do it either. The design of operations needs to be improved overall, because both the operations and the converter contain way too much logic. I think it would be good to improve this for this operation with this PR, because we kind of need it. Otherwise we'd have either incorrectness or redundancy, and both would be a worse situation than before.
I don't think it's a big change to fix this here. It's a pretty small change to just look up the table in TableEnvironmentImpl
instead of passing it to the operation during the conversion.
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.
@Airblader , OK, I would like to change for this.
...api-java/src/main/java/org/apache/flink/table/operations/ddl/AlterTableOptionsOperation.java
Outdated
Show resolved
Hide resolved
Thanks for your contribution @SteNicholas, Can you continue to finish this work? |
@lsyldliu, thanks for the reminder. I will update this pull request this week. Sorry for the delay to update. |
Hi @SteNicholas, would you like to continue this task? |
What is the purpose of the change
ALTER TABLE
does not seem to support theIF EXISTS
part in the DDL, but the corresponding methodsCatalog#renameTable
andCatalog#alterTable
do support it. Flink DDL should supportIF EXISTS
in DDL forALTER TABLE
.Brief change log
IF EXISTS
syntax support for theALTER TABLE
DDL.Verifying this change
SqlToOperationConverterTest
,CatalogTableITCase
andtable.q
add the tests forALTER TABLE IF EXISTS
syntax.Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation