-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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-36994][table] Support ALTER MATERIALIZED TABLE As <Query> statement #25880
base: master
Are you sure you want to change the base?
Conversation
f2205e0
to
9e31130
Compare
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.
@hackergin Thanks for your contribution, I left some comments.
...link-table-common/src/main/java/org/apache/flink/table/catalog/CatalogMaterializedTable.java
Outdated
Show resolved
Hide resolved
.../apache/flink/table/operations/materializedtable/AlterMaterializedTableAsQueryOperation.java
Outdated
Show resolved
Hide resolved
.../apache/flink/table/operations/materializedtable/AlterMaterializedTableAsQueryOperation.java
Outdated
Show resolved
Hide resolved
.../apache/flink/table/operations/materializedtable/AlterMaterializedTableAsQueryOperation.java
Outdated
Show resolved
Hide resolved
.../apache/flink/table/operations/materializedtable/AlterMaterializedTableAsQueryOperation.java
Outdated
Show resolved
Hide resolved
.../java/org/apache/flink/table/gateway/service/materializedtable/MaterializedTableManager.java
Outdated
Show resolved
Hide resolved
.../java/org/apache/flink/table/gateway/service/materializedtable/MaterializedTableManager.java
Show resolved
Hide resolved
.../apache/flink/table/planner/operations/SqlMaterializedTableNodeToOperationConverterTest.java
Outdated
Show resolved
Hide resolved
...y/src/test/java/org/apache/flink/table/gateway/service/MaterializedTableStatementITCase.java
Outdated
Show resolved
Hide resolved
...y/src/test/java/org/apache/flink/table/gateway/service/MaterializedTableStatementITCase.java
Outdated
Show resolved
Hide resolved
.../apache/flink/table/operations/materializedtable/AlterMaterializedTableAsQueryOperation.java
Outdated
Show resolved
Hide resolved
} | ||
} | ||
for (int i = oldSchema.getColumns().size(); i < newSchema.getColumns().size(); i++) { | ||
Column newColumn = newSchema.getColumns().get(i); |
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.
If the new added column is duplicated with old column, what behavior is? I think you should a test case to verify 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.
Based on my testing, if duplicate field names appear in a query, the latter field names will automatically have suffixes added, which seems consistent with the logic of the Calcite framework. For example, if there are two fields named a
, the second a will automatically be renamed to a0
.
9e31130
to
9a19d23
Compare
90c372c
to
a204971
Compare
a204971
to
480c394
Compare
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 updating, I left some comments.
@@ -1374,4 +1391,24 @@ public String toString() { | |||
+ '}'; | |||
} | |||
} | |||
|
|||
/* A table change to modify the definition query. */ |
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.
/* A table change to modify the definition query. */ | |
/** A table change to modify the definition query. */ |
@@ -394,7 +404,7 @@ static ModifyRefreshHandler modifyRefreshHandler( | |||
* </pre> | |||
*/ | |||
@PublicEvolving | |||
class AddColumn implements TableChange { | |||
class AddColumn implements CatalogTableChange, MaterializedTableChange { |
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.
Please give more java docs to explain it also support materialized table
@Internal | ||
public class AlterMaterializedTableAsQueryOperation extends AlterMaterializedTableOperation { | ||
|
||
private final List<TableChange> columnChanges; |
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.
Use MaterializedTableChange
|
||
private final List<TableChange> columnChanges; | ||
|
||
private final TableChange definitionQueryChange; |
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.
We can put it the above list, If you need it, you can filter it from the list.
"ALTER MATERIALIZED TABLE", | ||
params, | ||
Collections.emptyList(), | ||
Operation::asSummaryString); |
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 think we only need to print the definition query here.
TableChange.AddColumn addColumn = (TableChange.AddColumn) tableChange; | ||
rollbackChanges.add(TableChange.dropColumn(addColumn.getColumn().getName())); | ||
} else if (tableChange instanceof TableChange.ModifyRefreshHandler) { | ||
rollbackChanges.add( |
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.
This is not correct, we should use the refresh handler of rollback refresh job
.isEqualTo( | ||
Collections.singletonList( | ||
Column.physical("order_amount_sum", DataTypes.INT()))); | ||
assertThat(oldTable.getDefinitionQuery()).isNotEqualTo(newTable.getDefinitionQuery()); |
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 think we should verify the definition query is equals.
.isEqualTo( | ||
Collections.singletonList( | ||
Column.physical("order_amount_sum", DataTypes.INT()))); | ||
assertThat(oldTable.getDefinitionQuery()).isNotEqualTo(newTable.getDefinitionQuery()); |
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.
ditto
.isEqualTo(oldTable.getResolvedSchema().getPrimaryKey()); | ||
assertThat(newTable.getResolvedSchema().getWatermarkSpecs()) | ||
.isEqualTo(oldTable.getResolvedSchema().getWatermarkSpecs()); | ||
assertThat(oldTable.getDefinitionQuery()).isNotEqualTo(newTable.getDefinitionQuery()); |
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.
ditto
|
||
assertThat(getAddedColumns(newTable.getResolvedSchema(), oldTable.getResolvedSchema())) | ||
.isEqualTo(Collections.singletonList(Column.physical("pv", DataTypes.INT()))); | ||
assertThat(oldTable.getDefinitionQuery()).isNotEqualTo(newTable.getDefinitionQuery()); |
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.
ditto
What is the purpose of the change
Add support query modification for materialized table
Brief change log
**
Verifying this change
Some unit test and ITCase will be added in MaterializedTableStatementITCase to verify these changes.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (no)Documentation