luoyuxia commented on code in PR #3459:
URL: https://github.com/apache/fluss/pull/3459#discussion_r3465530372
##########
fluss-server/src/main/java/org/apache/fluss/server/coordinator/MetadataManager.java:
##########
@@ -440,6 +441,7 @@ public void alterTableSchema(
if (!schemaChanges.isEmpty()) {
Schema newSchema =
Review Comment:
I think we should also updat table option to reflect the new added merge
coumn.
Like
```
CREATE TABLE test_last_value (
id BIGINT,
status STRING,
last_login TIMESTAMP(3),
PRIMARY KEY (id) NOT ENFORCED
) WITH (
'table.merge-engine' = 'aggregation',
**'fields.status.agg' = 'last_value',**
**'fields.last_login.agg' = 'last_value'**
);
```
##########
fluss-common/src/main/java/org/apache/fluss/metadata/TableChange.java:
##########
@@ -269,6 +294,8 @@ public String toString() {
+ ", comment='"
+ comment
+ '\''
+ + ", aggFunction="
Review Comment:
nit: if aggFunction is null, we don't need to print "aggFunction=null"
##########
fluss-client/src/test/java/org/apache/fluss/client/admin/FlussAdminITCase.java:
##########
@@ -568,6 +568,62 @@ void testAlterTableColumn() throws Exception {
.hasMessageContaining("Column nested_row already exists");
}
+ @Test
+ void testAlterAggregationTableColumnWithAggFunction() throws Exception {
Review Comment:
I think we had better to verify the merge works for new coulmn, may need to
write rows to verify it.
--
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]
