Copilot commented on code in PR #1625:
URL: https://github.com/apache/fluss/pull/1625#discussion_r2371320572
##########
fluss-server/src/test/java/org/apache/fluss/server/testutils/RpcMessageTestUtils.java:
##########
@@ -139,6 +141,20 @@ public static CreateTableRequest newCreateTableRequest(
return createTableRequest;
}
+ public static AlterTableConfigsRequest newAlterTableConfigsRequest(
+ TablePath tablePath,
+ List<PbAlterConfigsRequestInfo> pbAlterConfigsRequestInfos,
+ boolean ignoreIfExists) {
+ AlterTableConfigsRequest alterTableConfigsRequest = new
AlterTableConfigsRequest();
+ alterTableConfigsRequest
+ .addAllConfigChanges(pbAlterConfigsRequestInfos)
+ .setIgnoreIfNotExists(ignoreIfExists)
+ .setTablePath()
Review Comment:
Missing method call after setTablePath(). This should likely be followed by
a setter method or the table path should be passed as an argument to properly
configure the request.
##########
fluss-server/src/main/java/org/apache/fluss/server/utils/ServerRpcMessageUtils.java:
##########
@@ -238,6 +240,22 @@ public static ServerNode toServerNode(PbServerNode
pbServerNode, ServerType serv
pbServerNode.hasRack() ? pbServerNode.getRack() : null);
}
+ public static TableChange toFlussTableChange(
+ PbAlterConfigsRequestInfo pbAlterConfigsRequestInfo) {
+ switch (pbAlterConfigsRequestInfo.getOpType()) {
+ case 1: // SET_OPTION
+ return TableChange.set(
+ pbAlterConfigsRequestInfo.getConfigKey(),
+ pbAlterConfigsRequestInfo.getConfigValue());
+ case 2: // RESET_OPTION
+ return
TableChange.reset(pbAlterConfigsRequestInfo.getConfigKey());
Review Comment:
Using magic numbers for operation types makes the code harder to maintain.
Consider using the AlterTableConfigsOpType enum constants instead of hardcoded
integers.
##########
fluss-client/src/main/java/org/apache/fluss/client/admin/FlussAdmin.java:
##########
@@ -235,6 +240,25 @@ public CompletableFuture<Void> createTable(
return gateway.createTable(request).thenApply(r -> null);
}
+ @Override
+ public CompletableFuture<Void> alterTable(
+ TablePath tablePath, List<TableChange> tableChanges, boolean
ignoreIfNotExists) {
+ tablePath.validate();
+ AlterTableConfigsRequest request = new AlterTableConfigsRequest();
+
+ List<PbAlterConfigsRequestInfo> pbFlussTableChanges =
+ tableChanges.stream()
+
.map(FlussTableChangeProtoConverter::toPbAlterConfigsRequestInfo)
+ .collect(Collectors.toList());
+
+ request.addAllConfigChanges(pbFlussTableChanges)
+ .setIgnoreIfNotExists(ignoreIfNotExists)
+ .setTablePath()
+ .setDatabaseName(tablePath.getDatabaseName())
+ .setTableName(tablePath.getTableName());
Review Comment:
Missing method call after setTablePath(). This should likely be followed by
a setter method or the table path should be passed as an argument to properly
configure the request.
##########
fluss-rpc/src/main/proto/FlussApi.proto:
##########
@@ -108,6 +108,24 @@ message CreateTableRequest {
message CreateTableResponse {
}
+// alter table request and response
+message AlterTableConfigsRequest {
+ required PbTablePath table_path = 1;
+ required bool ignore_if_not_exists = 2;
+ repeated PbAlterConfigsRequestInfo config_changes = 3;
+}
+
+message PbAlterConfigsRequestInfo {
+ required string config_key = 1;
+ optional string config_value = 2;
+ required int32 op_type = 3; // SET=0, DELETE=1, APPEND=2, SUBTRACT=3
Review Comment:
The comment shows SET=0, DELETE=1 but the code implementation uses SET=1,
DELETE=2. This inconsistency between the comment and actual implementation
could cause confusion.
```suggestion
required int32 op_type = 3; // SET=1, DELETE=2, APPEND=3, SUBTRACT=4
```
##########
fluss-server/src/main/java/org/apache/fluss/server/coordinator/CoordinatorService.java:
##########
@@ -294,6 +301,54 @@ public CompletableFuture<CreateTableResponse>
createTable(CreateTableRequest req
return CompletableFuture.completedFuture(new CreateTableResponse());
}
+ @Override
+ public CompletableFuture<AlterTablePropertiesResponse> alterTable(
+ AlterTableConfigsRequest request) {
+ TablePath tablePath = toTablePath(request.getTablePath());
+ tablePath.validate();
+ if (authorizer != null) {
+ authorizer.authorize(currentSession(), OperationType.ALTER,
Resource.table(tablePath));
+ }
+
+ AlterTablePropertiesResponse alterTableResponse = new
AlterTablePropertiesResponse();
+
+ handleFlussTableChanges(
+ tablePath, request.getConfigChangesList(),
request.isIgnoreIfNotExists());
+
+ return CompletableFuture.completedFuture(alterTableResponse);
+ }
+
+ private void handleFlussTableChanges(
+ TablePath tablePath,
+ List<PbAlterConfigsRequestInfo> configsRequestInfos,
+ boolean ignoreIfNotExists) {
+
+ List<TableChange> tableChanges =
+ configsRequestInfos.stream()
+ .filter(Objects::nonNull)
+ .map(ServerRpcMessageUtils::toFlussTableChange)
+ .collect(Collectors.toList());
+
+ List<TableChange.SetOption> setOptions = new ArrayList<>();
+ List<TableChange.ResetOption> resetOptions = new ArrayList<>();
+
+ for (TableChange tableChange : tableChanges) {
+ if (tableChange instanceof TableChange.SetOption) {
+ setOptions.add((TableChange.SetOption) tableChange);
+ } else if (tableChange instanceof TableChange.ResetOption) {
+ resetOptions.add((TableChange.ResetOption) tableChange);
+ }
+ // add more FlussTableChange type
+ }
+
+ if (!setOptions.isEmpty() || !resetOptions.isEmpty()) {
+ // handle properties
+ metadataManager.alterTableProperties(
+ tablePath, setOptions, resetOptions, ignoreIfNotExists);
+ }
+ return;
Review Comment:
[nitpick] Unnecessary return statement at the end of a void method. This can
be removed for cleaner code.
```suggestion
```
--
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]
