Copilot commented on code in PR #2799:
URL: https://github.com/apache/fluss/pull/2799#discussion_r2902863819
##########
fluss-lake/fluss-lake-paimon/src/main/java/org/apache/fluss/lake/paimon/utils/PaimonConversions.java:
##########
@@ -275,6 +274,14 @@ private static void validatePaimonOptions(Map<String,
String> properties) {
});
}
+ private static void validateAlterPaimonOptions(String key) {
+ if (PAIMON_UNSETTABLE_OPTIONS.contains(key)
+ || CoreOptions.IMMUTABLE_OPTIONS.contains(key)) {
+ throw new InvalidConfigException(
+ String.format("The Paimon option %s cannot be changed.",
key));
Review Comment:
`validateAlterPaimonOptions` receives the converted Paimon key (e.g.,
`paimon.bucket` becomes `bucket`), so the thrown error message will refer to
the stripped key. This is inconsistent with create-time validation (which
reports the original user-specified key) and can be confusing to users
troubleshooting ALTER TABLE failures. Consider including the original Fluss
property key in the exception message (or re-attaching the `paimon.` prefix
when applicable) so the error points to exactly what the user set/reset.
```suggestion
String displayKey = key;
if (!displayKey.startsWith(PAIMON_CONF_PREFIX)) {
displayKey = PAIMON_CONF_PREFIX + displayKey;
}
if (PAIMON_UNSETTABLE_OPTIONS.contains(key)
|| CoreOptions.IMMUTABLE_OPTIONS.contains(key)) {
throw new InvalidConfigException(
String.format("The Paimon option %s cannot be changed.",
displayKey));
```
##########
fluss-flink/fluss-flink-common/src/test/java/org/apache/fluss/flink/catalog/FlinkCatalogITCase.java:
##########
@@ -286,16 +286,8 @@ void testAlterTableConfig() throws Exception {
.hasMessage("The option 'bootstrap.servers' is not supported
to alter yet.");
String unSupportedDml6 =
- "alter table test_alter_table_append_only set
('paimon.file.format' = 'orc')";
- assertThatThrownBy(() -> tEnv.executeSql(unSupportedDml6))
- .rootCause()
- .isInstanceOf(InvalidConfigException.class)
- .hasMessage(
- "Property 'paimon.file.format' is not supported to
alter which is for datalake table.");
-
- String unSupportedDml7 =
"alter table test_alter_table_append_only set
('auto-increment.fields' = 'b')";
- assertThatThrownBy(() -> tEnv.executeSql(unSupportedDml7))
+ assertThatThrownBy(() -> tEnv.executeSql(unSupportedDml6))
.rootCause()
.isInstanceOf(CatalogException.class)
.hasMessage("The option 'auto-increment.fields' is not
supported to alter yet.");
Review Comment:
This test previously asserted that altering a lake-format-specific option
via Flink SQL (e.g. a `paimon.*` key) fails. With the server-side restriction
removed and validation moved into the lake implementation, that behavior should
be re-asserted with the new expected outcome/message (and ideally also add a
positive case for a changeable `paimon.*` option). As-is, the Flink SQL path no
longer has coverage for ALTER TABLE SET/RESET of lake table properties, which
is the main feature of this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
