From 36eaf328bc7f5e848ab498fe37de95b4493beeff Mon Sep 17 00:00:00 2001 From: Qi Yu Date: Thu, 5 Sep 2024 04:49:51 +0800 Subject: [PATCH] [#4845] fix(core): Fix bugs when updating metalake or catalog with nullable comment. (#4846) ### What changes were proposed in this pull request? Change the update SQL in the mapper to handle nullable comment. ### Why are the changes needed? It's a bug to fix. Fix: #4845 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? Added UTs and ITs --- .../relational/mapper/CatalogMetaMapper.java | 3 +- .../relational/mapper/MetalakeMetaMapper.java | 3 +- .../storage/relational/TestJDBCBackend.java | 91 +++++++++++++++++++ .../integration/test/client/CatalogIT.java | 22 +++++ .../integration/test/client/MetalakeIT.java | 14 +++ 5 files changed, 131 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/CatalogMetaMapper.java b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/CatalogMetaMapper.java index b45cbcec960..faedbcd9642 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/CatalogMetaMapper.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/CatalogMetaMapper.java @@ -150,7 +150,8 @@ CatalogPO selectCatalogMetaByMetalakeIdAndName( + " AND metalake_id = #{oldCatalogMeta.metalakeId}" + " AND type = #{oldCatalogMeta.type}" + " AND provider = #{oldCatalogMeta.provider}" - + " AND catalog_comment = #{oldCatalogMeta.catalogComment}" + + " AND (catalog_comment = #{oldCatalogMeta.catalogComment} " + + " OR (catalog_comment IS NULL and #{oldCatalogMeta.catalogComment} IS NULL))" + " AND properties = #{oldCatalogMeta.properties}" + " AND audit_info = #{oldCatalogMeta.auditInfo}" + " AND current_version = #{oldCatalogMeta.currentVersion}" diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/MetalakeMetaMapper.java b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/MetalakeMetaMapper.java index 4c041a99357..89f8d13ceb1 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/MetalakeMetaMapper.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/MetalakeMetaMapper.java @@ -134,7 +134,8 @@ public interface MetalakeMetaMapper { + " last_version = #{newMetalakeMeta.lastVersion}" + " WHERE metalake_id = #{oldMetalakeMeta.metalakeId}" + " AND metalake_name = #{oldMetalakeMeta.metalakeName}" - + " AND metalake_comment = #{oldMetalakeMeta.metalakeComment}" + + " AND (metalake_comment = #{oldMetalakeMeta.metalakeComment} " + + " OR (metalake_comment IS NULL and #{oldMetalakeMeta.metalakeComment} IS NULL))" + " AND properties = #{oldMetalakeMeta.properties}" + " AND audit_info = #{oldMetalakeMeta.auditInfo}" + " AND schema_version = #{oldMetalakeMeta.schemaVersion}" diff --git a/core/src/test/java/org/apache/gravitino/storage/relational/TestJDBCBackend.java b/core/src/test/java/org/apache/gravitino/storage/relational/TestJDBCBackend.java index 2b7e06e782d..26430d2fb5e 100644 --- a/core/src/test/java/org/apache/gravitino/storage/relational/TestJDBCBackend.java +++ b/core/src/test/java/org/apache/gravitino/storage/relational/TestJDBCBackend.java @@ -84,6 +84,7 @@ import org.apache.gravitino.utils.NamespaceUtil; import org.apache.ibatis.session.SqlSession; import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -434,6 +435,96 @@ public void testUpdateAlreadyExistsException() throws IOException { e -> createTopicEntity(topicCopy.id(), topicCopy.namespace(), "topic", auditInfo))); } + @Test + void testUpdateMetalakeWithNullableComment() throws IOException { + AuditInfo auditInfo = + AuditInfo.builder().withCreator("creator").withCreateTime(Instant.now()).build(); + + BaseMetalake metalake = + BaseMetalake.builder() + .withId(RandomIdGenerator.INSTANCE.nextId()) + .withName("metalake" + RandomIdGenerator.INSTANCE.nextId()) + .withAuditInfo(auditInfo) + .withComment(null) + .withProperties(null) + .withVersion(SchemaVersion.V_0_1) + .build(); + + backend.insert(metalake, false); + + backend.update( + metalake.nameIdentifier(), + Entity.EntityType.METALAKE, + e -> + BaseMetalake.builder() + .withId(metalake.id()) + .withName(metalake.name()) + .withAuditInfo(auditInfo) + .withComment("comment") + .withProperties(metalake.properties()) + .withVersion(metalake.getVersion()) + .build()); + + BaseMetalake updatedMetalake = + backend.get(metalake.nameIdentifier(), Entity.EntityType.METALAKE); + Assertions.assertNotNull(updatedMetalake.comment()); + + backend.delete(metalake.nameIdentifier(), Entity.EntityType.METALAKE, false); + } + + @Test + void testUpdateCatalogWithNullableComment() throws IOException { + AuditInfo auditInfo = + AuditInfo.builder().withCreator("creator").withCreateTime(Instant.now()).build(); + + String metalakeName = "metalake" + RandomIdGenerator.INSTANCE.nextId(); + BaseMetalake metalake = + BaseMetalake.builder() + .withId(RandomIdGenerator.INSTANCE.nextId()) + .withName(metalakeName) + .withAuditInfo(auditInfo) + .withComment("") + .withProperties(null) + .withVersion(SchemaVersion.V_0_1) + .build(); + backend.insert(metalake, false); + + CatalogEntity catalog = + CatalogEntity.builder() + .withId(RandomIdGenerator.INSTANCE.nextId()) + .withNamespace(NamespaceUtil.ofCatalog(metalakeName)) + .withName("catalog") + .withAuditInfo(auditInfo) + .withComment(null) + .withProperties(null) + .withType(Catalog.Type.RELATIONAL) + .withProvider("test") + .build(); + + backend.insert(catalog, false); + + backend.update( + catalog.nameIdentifier(), + Entity.EntityType.CATALOG, + e -> + CatalogEntity.builder() + .withId(catalog.id()) + .withNamespace(catalog.namespace()) + .withName(catalog.name()) + .withAuditInfo(auditInfo) + .withComment("comment") + .withProperties(catalog.getProperties()) + .withType(Catalog.Type.RELATIONAL) + .withProvider("test") + .build()); + + CatalogEntity updatedCatalog = backend.get(catalog.nameIdentifier(), Entity.EntityType.CATALOG); + Assertions.assertNotNull(updatedCatalog.getComment()); + + backend.delete(catalog.nameIdentifier(), Entity.EntityType.CATALOG, false); + backend.delete(metalake.nameIdentifier(), Entity.EntityType.METALAKE, false); + } + @Test public void testMetaLifeCycleFromCreationToDeletion() throws IOException { AuditInfo auditInfo = diff --git a/integration-test/src/test/java/org/apache/gravitino/integration/test/client/CatalogIT.java b/integration-test/src/test/java/org/apache/gravitino/integration/test/client/CatalogIT.java index 60f7a7dd080..57dcd66cae6 100644 --- a/integration-test/src/test/java/org/apache/gravitino/integration/test/client/CatalogIT.java +++ b/integration-test/src/test/java/org/apache/gravitino/integration/test/client/CatalogIT.java @@ -26,6 +26,7 @@ import java.util.Map; import org.apache.commons.lang.ArrayUtils; import org.apache.gravitino.Catalog; +import org.apache.gravitino.CatalogChange; import org.apache.gravitino.client.GravitinoMetalake; import org.apache.gravitino.integration.test.container.ContainerSuite; import org.apache.gravitino.integration.test.container.HiveContainer; @@ -278,4 +279,25 @@ public void testCreateCatalogWithPackage() { Assertions.assertTrue( exception.getMessage().contains("Invalid package path: /tmp/none_exist_path_to_package")); } + + @Test + void testUpdateCatalogWithNullableComment() { + String catalogName = GravitinoITUtils.genRandomName("catalog"); + Assertions.assertFalse(metalake.catalogExists(catalogName)); + + Map properties = Maps.newHashMap(); + properties.put("metastore.uris", hmsUri); + Catalog catalog = + metalake.createCatalog(catalogName, Catalog.Type.RELATIONAL, "hive", null, properties); + Assertions.assertTrue(metalake.catalogExists(catalogName)); + + Assertions.assertEquals(catalogName, catalog.name()); + Assertions.assertEquals(null, catalog.comment()); + + Catalog updatedCatalog = + metalake.alterCatalog(catalogName, CatalogChange.updateComment("new catalog comment")); + Assertions.assertEquals("new catalog comment", updatedCatalog.comment()); + + metalake.dropCatalog(catalogName); + } } diff --git a/integration-test/src/test/java/org/apache/gravitino/integration/test/client/MetalakeIT.java b/integration-test/src/test/java/org/apache/gravitino/integration/test/client/MetalakeIT.java index bbd93ff2e69..f9dcfa132a1 100644 --- a/integration-test/src/test/java/org/apache/gravitino/integration/test/client/MetalakeIT.java +++ b/integration-test/src/test/java/org/apache/gravitino/integration/test/client/MetalakeIT.java @@ -194,6 +194,20 @@ public void testDropMetalakes() { assertFalse(client.dropMetalake(metalakeA.name()), "metalake should be non-existent"); } + @Test + public void testUpdateMetalakeWithNullableComment() { + client.createMetalake(metalakeNameA, null, Collections.emptyMap()); + GravitinoMetalake metalake = client.loadMetalake(metalakeNameA); + assertEquals(metalakeNameA, metalake.name()); + assertEquals(null, metalake.comment()); + + MetalakeChange[] changes = + new MetalakeChange[] {MetalakeChange.updateComment("new metalake comment")}; + GravitinoMetalake updatedMetalake = client.alterMetalake(metalakeNameA, changes); + assertEquals("new metalake comment", updatedMetalake.comment()); + client.dropMetalake(metalakeNameA); + } + public void dropMetalakes() { GravitinoMetalake[] metaLakes = client.listMetalakes(); for (GravitinoMetalake metalake : metaLakes) {