-
Notifications
You must be signed in to change notification settings - Fork 13.9k
[FLINK-38791][table] Add support for ALTER MATERIALIZED TABLE MODIFY
#27327
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
| | | ||
| AlterTableAddOrModify(ctx) | ||
| | | ||
| <LPAREN> |
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: I am curious why do we have a mix of LPAREN and ( . I assume we should use LPAREN for all left brackets so it uses the grammar constant.
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.
<LPAREN> means there will be a token parsed if it faces ( in sql
( is just a part of syntax which has nothing to do with <LPAREN>
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.
it just says that if it meets something matching
inside (...)* then it should expect it arbitrary amount of times
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.
Thank you @snuyanzin. I just had minor comments. LGTM overall.
...l-parser/src/test/java/org/apache/flink/sql/parser/MaterializedTableStatementParserTest.java
Outdated
Show resolved
Hide resolved
| LogicalType dataType = | ||
| createDataType(context.getCatalogManager().getDataTypeFactory(), col); | ||
| LogicalType oldDataType = map.get(col.getName()).getDataType().getLogicalType(); | ||
| if (!LogicalTypeCasts.supportsImplicitCast(oldDataType, dataType)) { |
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.
Is this new logic or was this previous behavior?
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 a comment referencing code for CREATE [MATERIALIZED ]TABLE path with similar logic
...ache/flink/table/planner/operations/converters/SqlAlterMaterializedTableSchemaConverter.java
Outdated
Show resolved
Hide resolved
...ache/flink/table/planner/operations/converters/SqlAlterMaterializedTableSchemaConverter.java
Outdated
Show resolved
Hide resolved
...l-parser/src/test/java/org/apache/flink/sql/parser/MaterializedTableStatementParserTest.java
Show resolved
Hide resolved
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.
LGTM. Thanks @snuyanzin
|
@flinkbot run azure |
What is the purpose of the change
The next part of FLIP-550 which adds support for
ALTER MATERIALIZED TABLE MODIFYin way likeand
ALTER MATERIALIZED TABLE myMaterializedTable MODIFY ( ts AS current_timestamp FIRST, col_meta INT METADATA FROM 'mk1' VIRTUAL AFTER col_b, PRIMARY KEY (id) NOT ENFORCED, WATERMARK FOR ts AS ts - INTERVAL '3' SECOND );Brief change log
parser and converters
Verifying this change
parser and converter's test added
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation