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-35198][table] Support the execution of refresh materialized table #24760
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.
@xuyangzhong Thanks for your contribution, I left some comments.
.../java/org/apache/flink/table/gateway/service/materializedtable/MaterializedTableManager.java
Outdated
Show resolved
Hide resolved
...l-parser/src/main/java/org/apache/flink/sql/parser/ddl/SqlAlterMaterializedTableRefresh.java
Outdated
Show resolved
Hide resolved
...che/flink/table/planner/operations/converters/SqlAlterMaterializedTableRefreshConverter.java
Outdated
Show resolved
Hide resolved
.../apache/flink/table/operations/materializedtable/AlterMaterializedTableRefreshOperation.java
Show resolved
Hide resolved
.../java/org/apache/flink/table/gateway/service/materializedtable/MaterializedTableManager.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
...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
...y/src/test/java/org/apache/flink/table/gateway/service/MaterializedTableStatementITCase.java
Show resolved
Hide resolved
Hi, @lsyldliu all comments have been addressed. Can you take a look again? |
.../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
Outdated
Show resolved
Hide resolved
if (!nonStringPartitionKeys.isEmpty()) { | ||
throw new ValidationException( | ||
String.format( | ||
"Currently, specifying non-char or non-string type partition fields" |
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.
What about using the following exception msg:
"Currently, manual refreshing materialized table only supports specifying char and string type partition keys. All specific partition keys with unsupported types are:\n\n %s."
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.
what about "Currently, manually refreshing materialized table only supports specifying char and string type partition keys. All specific partition keys with unsupported types are:\n\n%s."
?
.../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
Outdated
Show resolved
Hide resolved
throw new ValidationException( | ||
String.format( | ||
"The partition spec contains unknown partition keys: [%s]. All known partition keys are: [%s].", | ||
unknownPartitionKeys.stream() |
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 can refer to the following logic, util stringifyOption
method.
options.entrySet().stream()
.map(
optionEntry ->
stringifyOption(
optionEntry.getKey(),
optionEntry.getValue()))
.sorted()
.collect(Collectors.joining("\n")))
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 don't need to use the util because the var here is Set<String>
, not Map
What is the purpose of the change
Support the execution of refresh materialized table.
Brief change log
Verifying this change
Some tests are added.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation