diff --git a/api/src/main/java/com/datastrato/gravitino/rel/SchemaChange.java b/api/src/main/java/com/datastrato/gravitino/rel/SchemaChange.java index 2058dde3388..41f48fd6c73 100644 --- a/api/src/main/java/com/datastrato/gravitino/rel/SchemaChange.java +++ b/api/src/main/java/com/datastrato/gravitino/rel/SchemaChange.java @@ -23,9 +23,22 @@ import lombok.EqualsAndHashCode; import lombok.Getter; -/** NamespaceChange class to set the property and value pairs for the namespace. */ +/** + * The SchemaChange interface defines the public API for managing schemas in a catalog. If the + * catalog implementation supports schemas, it must implement this interface. + */ public interface SchemaChange { + /** + * SchemaChange class to update the comment for the schema. + * + * @param newComment The new comment to update. + * @return The SchemaChange object. + */ + static SchemaChange updateComment(String newComment) { + return new UpdateComment(newComment); + } + /** * SchemaChange class to set the property and value pairs for the schema. * @@ -47,6 +60,22 @@ static SchemaChange removeProperty(String property) { return new RemoveProperty(property); } + /** A SchemaChange to update a schema's comment. */ + @EqualsAndHashCode + @Getter + final class UpdateComment implements SchemaChange { + private final String newComment; + + private UpdateComment(String newComment) { + this.newComment = newComment; + } + } + + /** + * A SchemaChange to set a schema property. + * + *

If the property already exists, it must be replaced with the new value. + */ @Getter @EqualsAndHashCode final class SetProperty implements SchemaChange { @@ -59,6 +88,11 @@ private SetProperty(String property, String value) { } } + /** + * A SchemaChange to remove a schema property. + * + *

If the property does not exist, the change should succeed. + */ @Getter @EqualsAndHashCode final class RemoveProperty implements SchemaChange { diff --git a/api/src/test/java/com/datastrato/gravitino/TestSchemaChange.java b/api/src/test/java/com/datastrato/gravitino/TestSchemaChange.java index 85cdb8d6440..4c8bcd27539 100644 --- a/api/src/test/java/com/datastrato/gravitino/TestSchemaChange.java +++ b/api/src/test/java/com/datastrato/gravitino/TestSchemaChange.java @@ -12,10 +12,19 @@ import com.datastrato.gravitino.rel.SchemaChange; import com.datastrato.gravitino.rel.SchemaChange.RemoveProperty; import com.datastrato.gravitino.rel.SchemaChange.SetProperty; +import com.datastrato.gravitino.rel.SchemaChange.UpdateComment; import org.junit.jupiter.api.Test; public class TestSchemaChange { + @Test + public void testUpdateComment() { + String comment = "New comment"; + UpdateComment change = (UpdateComment) SchemaChange.updateComment(comment); + + assertEquals(comment, change.getNewComment()); + } + @Test void testSetProperty() { String property = "Jam"; @@ -87,4 +96,29 @@ void testSetPropertyNotEqualsAndHashCode() { assertFalse(changeB.equals(changeA)); assertNotEquals(changeA.hashCode(), changeB.hashCode()); } + + @Test + void testUpdateCommentEqualsAndHashCode() { + String commentA = "a comment"; + UpdateComment changeA = (UpdateComment) SchemaChange.updateComment(commentA); + String commentB = "a comment"; + UpdateComment changeB = (UpdateComment) SchemaChange.updateComment(commentB); + + assertTrue(changeA.equals(changeB)); + assertTrue(changeB.equals(changeA)); + assertEquals(changeA.hashCode(), changeB.hashCode()); + } + + @Test + void testUpdateCommentNotEqualsAndHashCode() { + String commentA = "a comment"; + UpdateComment changeA = (UpdateComment) SchemaChange.updateComment(commentA); + String commentB = "a new comment"; + UpdateComment changeB = (UpdateComment) SchemaChange.updateComment(commentB); + + assertFalse(changeA.equals(null)); + assertFalse(changeA.equals(changeB)); + assertFalse(changeB.equals(changeA)); + assertNotEquals(changeA.hashCode(), changeB.hashCode()); + } } diff --git a/catalogs/catalog-hive/src/main/java/com/datastrato/gravitino/catalog/hive/HiveCatalogOperations.java b/catalogs/catalog-hive/src/main/java/com/datastrato/gravitino/catalog/hive/HiveCatalogOperations.java index 94f9c022745..1ff02a0a2fd 100644 --- a/catalogs/catalog-hive/src/main/java/com/datastrato/gravitino/catalog/hive/HiveCatalogOperations.java +++ b/catalogs/catalog-hive/src/main/java/com/datastrato/gravitino/catalog/hive/HiveCatalogOperations.java @@ -283,8 +283,13 @@ public HiveSchema alterSchema(NameIdentifier ident, SchemaChange... changes) ident.name(), properties.keySet()); + boolean commentUpdated = false; + String comment = null; for (SchemaChange change : changes) { - if (change instanceof SchemaChange.SetProperty) { + if (change instanceof SchemaChange.UpdateComment) { + commentUpdated = true; + comment = ((SchemaChange.UpdateComment) change).getNewComment(); + } else if (change instanceof SchemaChange.SetProperty) { properties.put( ((SchemaChange.SetProperty) change).getProperty(), ((SchemaChange.SetProperty) change).getValue()); @@ -299,6 +304,9 @@ public HiveSchema alterSchema(NameIdentifier ident, SchemaChange... changes) // alter the hive database parameters Database alteredDatabase = database.deepCopy(); alteredDatabase.setParameters(properties); + if (commentUpdated) { + alteredDatabase.setDescription(comment); + } clientPool.run( client -> { diff --git a/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/IcebergCatalogOperations.java b/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/IcebergCatalogOperations.java index 4b6cdaf9074..35d28adca2c 100644 --- a/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/IcebergCatalogOperations.java +++ b/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/IcebergCatalogOperations.java @@ -246,8 +246,11 @@ public IcebergSchema alterSchema(NameIdentifier ident, SchemaChange... changes) List removals = new ArrayList<>(); Map updates = new HashMap<>(); Map resultProperties = new HashMap<>(metadata); + String comment = null; for (SchemaChange change : changes) { - if (change instanceof SchemaChange.SetProperty) { + if (change instanceof SchemaChange.UpdateComment) { + comment = ((SchemaChange.UpdateComment) change).getNewComment(); + } else if (change instanceof SchemaChange.SetProperty) { String key = ((SchemaChange.SetProperty) change).getProperty(); String val = ((SchemaChange.SetProperty) change).getValue(); updates.put(key, val); @@ -261,10 +264,12 @@ public IcebergSchema alterSchema(NameIdentifier ident, SchemaChange... changes) } } - String comment = - Optional.of(response.properties()) - .map(map -> map.get(IcebergSchemaPropertiesMetadata.COMMENT)) - .orElse(null); + comment = + Optional.ofNullable(comment) + .orElse( + Optional.of(response.properties()) + .map(map -> map.get(IcebergSchemaPropertiesMetadata.COMMENT)) + .orElse(null)); IcebergSchema icebergSchema = new IcebergSchema.Builder() .withName(ident.name()) diff --git a/clients/client-java/src/main/java/com/datastrato/gravitino/client/DTOConverters.java b/clients/client-java/src/main/java/com/datastrato/gravitino/client/DTOConverters.java index af944a0a55c..431368b4b0d 100644 --- a/clients/client-java/src/main/java/com/datastrato/gravitino/client/DTOConverters.java +++ b/clients/client-java/src/main/java/com/datastrato/gravitino/client/DTOConverters.java @@ -99,7 +99,11 @@ static CatalogUpdateRequest toCatalogUpdateRequest(CatalogChange change) { } static SchemaUpdateRequest toSchemaUpdateRequest(SchemaChange change) { - if (change instanceof SchemaChange.SetProperty) { + if (change instanceof SchemaChange.UpdateComment) { + return new SchemaUpdateRequest.UpdateSchemaCommentRequest( + ((SchemaChange.UpdateComment) change).getNewComment()); + + } else if (change instanceof SchemaChange.SetProperty) { return new SchemaUpdateRequest.SetSchemaPropertyRequest( ((SchemaChange.SetProperty) change).getProperty(), ((SchemaChange.SetProperty) change).getValue()); diff --git a/clients/client-java/src/test/java/com/datastrato/gravitino/client/TestRelationalCatalog.java b/clients/client-java/src/test/java/com/datastrato/gravitino/client/TestRelationalCatalog.java index 45138b51e8c..96c6fdd9181 100644 --- a/clients/client-java/src/test/java/com/datastrato/gravitino/client/TestRelationalCatalog.java +++ b/clients/client-java/src/test/java/com/datastrato/gravitino/client/TestRelationalCatalog.java @@ -235,6 +235,16 @@ public void testLoadSchema() throws JsonProcessingException { Assertions.assertTrue(ex1.getMessage().contains("schema not found")); } + @Test + public void testUpdateSchemaComment() throws JsonProcessingException { + NameIdentifier ident = NameIdentifier.of(metalakeName, catalogName, "schema1"); + SchemaUpdateRequest.UpdateSchemaCommentRequest req = + new SchemaUpdateRequest.UpdateSchemaCommentRequest("comment"); + SchemaDTO expectedSchema = createMockSchema("schema1", "comment", ImmutableMap.of("k1", "v1")); + + testAlterSchema(ident, req, expectedSchema); + } + @Test public void testSetSchemaProperty() throws JsonProcessingException { NameIdentifier ident = NameIdentifier.of(metalakeName, catalogName, "schema1"); diff --git a/common/src/main/java/com/datastrato/gravitino/dto/requests/SchemaUpdateRequest.java b/common/src/main/java/com/datastrato/gravitino/dto/requests/SchemaUpdateRequest.java index 045f3b3acb8..372dfa34a0d 100644 --- a/common/src/main/java/com/datastrato/gravitino/dto/requests/SchemaUpdateRequest.java +++ b/common/src/main/java/com/datastrato/gravitino/dto/requests/SchemaUpdateRequest.java @@ -19,6 +19,9 @@ @JsonIgnoreProperties(ignoreUnknown = true) @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY) @JsonSubTypes({ + @JsonSubTypes.Type( + value = SchemaUpdateRequest.UpdateSchemaCommentRequest.class, + name = "updateComment"), @JsonSubTypes.Type( value = SchemaUpdateRequest.SetSchemaPropertyRequest.class, name = "setProperty"), @@ -30,6 +33,35 @@ public interface SchemaUpdateRequest extends RESTRequest { SchemaChange schemaChange(); + @EqualsAndHashCode + @ToString + class UpdateSchemaCommentRequest implements SchemaUpdateRequest { + + @Getter + @JsonProperty("newComment") + private final String newComment; + + public UpdateSchemaCommentRequest(String newComment) { + this.newComment = newComment; + } + + public UpdateSchemaCommentRequest() { + this(null); + } + + @Override + public void validate() throws IllegalArgumentException { + Preconditions.checkArgument( + StringUtils.isNotBlank(newComment), + "\"newComment\" field is required and cannot be empty"); + } + + @Override + public SchemaChange schemaChange() { + return SchemaChange.updateComment(newComment); + } + } + @EqualsAndHashCode @ToString class SetSchemaPropertyRequest implements SchemaUpdateRequest { diff --git a/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/hive/CatalogHiveIT.java b/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/hive/CatalogHiveIT.java index 62aad9d1798..bc2273c3157 100644 --- a/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/hive/CatalogHiveIT.java +++ b/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/hive/CatalogHiveIT.java @@ -902,16 +902,20 @@ public void testAlterSchema() throws TException, InterruptedException { .alterSchema( ident, SchemaChange.removeProperty("key1"), - SchemaChange.setProperty("key2", "val2-alter")); + SchemaChange.setProperty("key2", "val2-alter"), + SchemaChange.updateComment("comment1")); - Map properties2 = catalog.asSchemas().loadSchema(ident).properties(); + Schema schema = catalog.asSchemas().loadSchema(ident); + Map properties2 = schema.properties(); Assertions.assertFalse(properties2.containsKey("key1")); Assertions.assertEquals("val2-alter", properties2.get("key2")); + Assertions.assertEquals("comment1", schema.comment()); Database database = hiveClientPool.run(client -> client.getDatabase(schemaName)); Map properties3 = database.getParameters(); Assertions.assertFalse(properties3.containsKey("key1")); Assertions.assertEquals("val2-alter", properties3.get("key2")); + Assertions.assertEquals("comment1", database.getDescription()); } @Test diff --git a/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/lakehouse/iceberg/CatalogIcebergIT.java b/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/lakehouse/iceberg/CatalogIcebergIT.java index b679eecf75d..46d6cbfddd4 100644 --- a/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/lakehouse/iceberg/CatalogIcebergIT.java +++ b/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/lakehouse/iceberg/CatalogIcebergIT.java @@ -270,10 +270,12 @@ void testOperationIcebergSchema() { Assertions.assertTrue(schemaNames.contains(testSchemaName)); // alert、load schema check. - schemas.alterSchema(schemaIdent, SchemaChange.setProperty("t1", "v1")); + schemas.alterSchema( + schemaIdent, SchemaChange.setProperty("t1", "v1"), SchemaChange.updateComment("comment1")); Schema schema = schemas.loadSchema(schemaIdent); String val = schema.properties().get("t1"); Assertions.assertEquals("v1", val); + Assertions.assertEquals("comment1", schema.comment()); Map hiveCatalogProps = hiveCatalog.loadNamespaceMetadata( diff --git a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestSchemaOperations.java b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestSchemaOperations.java index 56ab6242ffa..41eacb93839 100644 --- a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestSchemaOperations.java +++ b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestSchemaOperations.java @@ -275,84 +275,108 @@ public void testLoadSchema() { @Test public void testAlterSchema() { + SchemaUpdateRequest commentReq = + new SchemaUpdateRequest.UpdateSchemaCommentRequest("new comment"); + Schema commentSchema = mockSchema("schema1", "new comment", ImmutableMap.of("key", "value")); + SchemaUpdateRequest setReq = new SchemaUpdateRequest.SetSchemaPropertyRequest("key2", "value2"); - Schema updatedSchema = + Schema setSchema = mockSchema("schema1", "comment", ImmutableMap.of("key", "value", "key2", "value2")); SchemaUpdateRequest removeReq = new SchemaUpdateRequest.RemoveSchemaPropertyRequest("key2"); Schema removedSchema = mockSchema("schema1", "comment", ImmutableMap.of("key", "value")); + // Test update comment + when(dispatcher.alterSchema(any(), eq(commentReq.schemaChange()))).thenReturn(commentSchema); + SchemaUpdatesRequest req1 = new SchemaUpdatesRequest(ImmutableList.of(commentReq)); + Response resp1 = + target("/metalakes/" + metalake + "/catalogs/" + catalog + "/schemas/schema1") + .request(MediaType.APPLICATION_JSON_TYPE) + .accept("application/vnd.gravitino.v1+json") + .put(javax.ws.rs.client.Entity.entity(req1, MediaType.APPLICATION_JSON_TYPE)); + + Assertions.assertEquals(Response.Status.OK.getStatusCode(), resp1.getStatus()); + Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE, resp1.getMediaType()); + + SchemaResponse schemaResp1 = resp1.readEntity(SchemaResponse.class); + Assertions.assertEquals(0, schemaResp1.getCode()); + + SchemaDTO schemaDTO1 = schemaResp1.getSchema(); + Assertions.assertEquals("schema1", schemaDTO1.name()); + Assertions.assertEquals("new comment", schemaDTO1.comment()); + Assertions.assertEquals(ImmutableMap.of("key", "value"), schemaDTO1.properties()); + // Test set property - when(dispatcher.alterSchema(any(), eq(setReq.schemaChange()))).thenReturn(updatedSchema); - SchemaUpdatesRequest req = new SchemaUpdatesRequest(ImmutableList.of(setReq)); - Response resp = + when(dispatcher.alterSchema(any(), eq(setReq.schemaChange()))).thenReturn(setSchema); + SchemaUpdatesRequest req2 = new SchemaUpdatesRequest(ImmutableList.of(setReq)); + Response resp2 = target("/metalakes/" + metalake + "/catalogs/" + catalog + "/schemas/schema1") .request(MediaType.APPLICATION_JSON_TYPE) .accept("application/vnd.gravitino.v1+json") - .put(javax.ws.rs.client.Entity.entity(req, MediaType.APPLICATION_JSON_TYPE)); + .put(javax.ws.rs.client.Entity.entity(req2, MediaType.APPLICATION_JSON_TYPE)); - Assertions.assertEquals(Response.Status.OK.getStatusCode(), resp.getStatus()); - Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE, resp.getMediaType()); + Assertions.assertEquals(Response.Status.OK.getStatusCode(), resp2.getStatus()); + Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE, resp2.getMediaType()); - SchemaResponse schemaResp = resp.readEntity(SchemaResponse.class); - Assertions.assertEquals(0, schemaResp.getCode()); + SchemaResponse schemaResp2 = resp2.readEntity(SchemaResponse.class); + Assertions.assertEquals(0, schemaResp2.getCode()); - SchemaDTO schemaDTO = schemaResp.getSchema(); - Assertions.assertEquals("schema1", schemaDTO.name()); - Assertions.assertEquals("comment", schemaDTO.comment()); + SchemaDTO schemaDTO2 = schemaResp2.getSchema(); + Assertions.assertEquals("schema1", schemaDTO2.name()); + Assertions.assertEquals("comment", schemaDTO2.comment()); Assertions.assertEquals( - ImmutableMap.of("key", "value", "key2", "value2"), schemaDTO.properties()); + ImmutableMap.of("key", "value", "key2", "value2"), schemaDTO2.properties()); // Test remove property when(dispatcher.alterSchema(any(), eq(removeReq.schemaChange()))).thenReturn(removedSchema); - SchemaUpdatesRequest req1 = new SchemaUpdatesRequest(ImmutableList.of(removeReq)); - Response resp1 = + SchemaUpdatesRequest req3 = new SchemaUpdatesRequest(ImmutableList.of(removeReq)); + Response resp3 = target("/metalakes/" + metalake + "/catalogs/" + catalog + "/schemas/schema1") .request(MediaType.APPLICATION_JSON_TYPE) .accept("application/vnd.gravitino.v1+json") - .put(javax.ws.rs.client.Entity.entity(req1, MediaType.APPLICATION_JSON_TYPE)); + .put(javax.ws.rs.client.Entity.entity(req3, MediaType.APPLICATION_JSON_TYPE)); - Assertions.assertEquals(Response.Status.OK.getStatusCode(), resp1.getStatus()); - Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE, resp1.getMediaType()); + Assertions.assertEquals(Response.Status.OK.getStatusCode(), resp3.getStatus()); + Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE, resp3.getMediaType()); - SchemaResponse schemaResp1 = resp1.readEntity(SchemaResponse.class); - Assertions.assertEquals(0, schemaResp1.getCode()); + SchemaResponse schemaResp3 = resp3.readEntity(SchemaResponse.class); + Assertions.assertEquals(0, schemaResp3.getCode()); - SchemaDTO schemaDTO1 = schemaResp1.getSchema(); - Assertions.assertEquals("schema1", schemaDTO1.name()); - Assertions.assertEquals("comment", schemaDTO1.comment()); - Assertions.assertEquals(ImmutableMap.of("key", "value"), schemaDTO1.properties()); + SchemaDTO schemaDTO3 = schemaResp3.getSchema(); + Assertions.assertEquals("schema1", schemaDTO3.name()); + Assertions.assertEquals("comment", schemaDTO3.comment()); + Assertions.assertEquals(ImmutableMap.of("key", "value"), schemaDTO3.properties()); // Test throw NoSuchSchemaException doThrow(new NoSuchSchemaException("mock error")).when(dispatcher).alterSchema(any(), any()); - Response resp2 = + Response resp4 = target("/metalakes/" + metalake + "/catalogs/" + catalog + "/schemas/schema1") .request(MediaType.APPLICATION_JSON_TYPE) .accept("application/vnd.gravitino.v1+json") .put(javax.ws.rs.client.Entity.entity(req1, MediaType.APPLICATION_JSON_TYPE)); - Assertions.assertEquals(Response.Status.NOT_FOUND.getStatusCode(), resp2.getStatus()); - Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE, resp2.getMediaType()); + Assertions.assertEquals(Response.Status.NOT_FOUND.getStatusCode(), resp4.getStatus()); + Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE, resp4.getMediaType()); - ErrorResponse errorResp = resp2.readEntity(ErrorResponse.class); - Assertions.assertEquals(ErrorConstants.NOT_FOUND_CODE, errorResp.getCode()); - Assertions.assertEquals(NoSuchSchemaException.class.getSimpleName(), errorResp.getType()); + ErrorResponse errorResp4 = resp4.readEntity(ErrorResponse.class); + Assertions.assertEquals(ErrorConstants.NOT_FOUND_CODE, errorResp4.getCode()); + Assertions.assertEquals(NoSuchSchemaException.class.getSimpleName(), errorResp4.getType()); // Test throw RuntimeException doThrow(new RuntimeException("mock error")).when(dispatcher).alterSchema(any(), any()); - Response resp3 = + Response resp5 = target("/metalakes/" + metalake + "/catalogs/" + catalog + "/schemas/schema1") .request(MediaType.APPLICATION_JSON_TYPE) .accept("application/vnd.gravitino.v1+json") .put(javax.ws.rs.client.Entity.entity(req1, MediaType.APPLICATION_JSON_TYPE)); Assertions.assertEquals( - Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(), resp3.getStatus()); - Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE, resp3.getMediaType()); + Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(), resp5.getStatus()); + Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE, resp5.getMediaType()); - ErrorResponse errorResp3 = resp3.readEntity(ErrorResponse.class); - Assertions.assertEquals(ErrorConstants.INTERNAL_ERROR_CODE, errorResp3.getCode()); - Assertions.assertEquals(RuntimeException.class.getSimpleName(), errorResp3.getType()); + ErrorResponse errorResp5 = resp5.readEntity(ErrorResponse.class); + Assertions.assertEquals(ErrorConstants.INTERNAL_ERROR_CODE, errorResp5.getCode()); + Assertions.assertEquals(RuntimeException.class.getSimpleName(), errorResp5.getType()); } @Test