-
Notifications
You must be signed in to change notification settings - Fork 13k
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-30497][table-api] Introduce TableChange to represent DROP change #21592
Conversation
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 the contribution, and it looks good in general. I left some comments. Btw, please run mvn spotless:apply
to fix the format issue.
flink-table/flink-table-common/src/main/java/org/apache/flink/table/catalog/TableChange.java
Outdated
Show resolved
Hide resolved
spec -> | ||
Stream.concat( | ||
visitor.visit(spec.getWatermarkExpression()).stream(), | ||
Stream.of(spec.getRowtimeAttribute()))) |
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 curious under what condition the watermark expression does not contain the rowtime field?
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.
For example, you can specify WATERMARK FOR rowtime AS col - INTERVAL 5 SECOND
. I add a case in the ColumnReferencesFinderTest
.
...ner/src/test/java/org/apache/flink/table/planner/operations/SqlToOperationConverterTest.java
Outdated
Show resolved
Hide resolved
...ner/src/test/java/org/apache/flink/table/planner/operations/SqlToOperationConverterTest.java
Outdated
Show resolved
Hide resolved
...ner/src/test/java/org/apache/flink/table/planner/operations/SqlToOperationConverterTest.java
Outdated
Show resolved
Hide resolved
...able-planner/src/main/java/org/apache/flink/table/planner/utils/OperationConverterUtils.java
Outdated
Show resolved
Hide resolved
...le-planner/src/main/java/org/apache/flink/table/planner/operations/AlterSchemaConverter.java
Outdated
Show resolved
Hide resolved
columns.remove(columnName); | ||
columns.add(newName); | ||
|
||
primaryKeys.remove(columnName); |
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 don't think we can directly add newColumnName
to primaryKeys
.
primaryKeys.remove(columnName); | |
if (primaryKeys.remove(columnName)) { | |
primaryKeys.add(newName); | |
} |
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.
Btw, I don't think we need to update primaryKeys
here because buildUpdatedPrimaryKey
will compute the primary key name.
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. I remove this.
columnToDependencies | ||
.getOrDefault(columnName, Collections.emptySet()) | ||
.forEach( | ||
referredColumn -> { | ||
columnToReferences.get(referredColumn).remove(columnName); | ||
columnToReferences.get(referredColumn).add(newName); | ||
}); | ||
columnToDependencies.put(newName, columnToDependencies.remove(columnName)); | ||
|
||
columns.remove(columnName); | ||
columns.add(newName); |
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.
Do we need to update columnToDependencies
and columnToReferences
?
ALTER TABLE RENAME
is not a bulk operation; there won't exist any further calls to get value from them.
...le-planner/src/main/java/org/apache/flink/table/planner/operations/AlterSchemaConverter.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Jane Chan <55568005+LadyForest@users.noreply.github.com>
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 the update! The PR is almost good to merge except for one minor comment.
...planner/src/main/java/org/apache/flink/table/planner/operations/SqlToOperationConverter.java
Outdated
Show resolved
Hide resolved
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 the update! The PR is almost good to merge except for one minor 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 the update, LGTM 👍
This closes apache#21592 (cherry picked from commit cce059c)
What is the purpose of the change
Introduce TableChange to represent DROP syntax.
Verifying this change
This change is already covered by existing tests, such as SqlToOperationConverterTest.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation