[#3699] refactor(API): Refactor client side FilesetCatalog to use rel…

…ative path in NameIdentifier (#3802) ### What changes were proposed in this pull request? Currently, in the FilesetCatalog.java, the methods like "loadFileset", "createFileset", "updateFileset" all need a NameIdentifier parameter, which needs to be a fully-qualified (metalake.catalog.schema.fileset) name. But the "metalake" and "catalog" are not needed, as they already be provided when load the catalog. To make the API clear and easier to use, we will change it to use a relative NameIdentifier object (which is "schema.fileset") as the fileset's ID, so that the user doesn't need to provide the metalake and catalog names repeatedly. Please note, this only affects the client side. ### Why are the changes needed? To make the API simple and easy to understand. Fix: #3699 ### Does this PR introduce _any_ user-facing change? No behavior change, just method parameter. ### How was this patch tested? No introduce new class or method, so the change will be covered by all existing test cases. --------- Co-authored-by: qqqttt123 <[email protected]> Co-authored-by: Heng Qin <[email protected]> Co-authored-by: Liang Chun <[email protected]> Co-authored-by: Lance Lin <[email protected]> Co-authored-by: LanceLin <[email protected]> Co-authored-by: Rory <[email protected]> Co-authored-by: mchades <[email protected]> Co-authored-by: Yuhui <[email protected]> Co-authored-by: Qi Yu <[email protected]> Co-authored-by: Junshen Tao <[email protected]> Co-authored-by: noidname01 <[email protected]> Co-authored-by: TimWang <[email protected]> Co-authored-by: Ikko Eltociear Ashimine <[email protected]> Co-authored-by: cai can <[email protected]> Co-authored-by: XiaoZ <[email protected]> Co-authored-by: zhanghan18 <[email protected]> Co-authored-by: rich7420 <[email protected]> Co-authored-by: hanwxx <[email protected]> Co-authored-by: Eric Chang <[email protected]> Co-authored-by: Jerry Shao <[email protected]> Co-authored-by: FANNG <[email protected]> Co-authored-by: Ziva Li <[email protected]> Co-authored-by: xloya <[email protected]> Co-authored-by: xiaojiebao <[email protected]> Co-authored-by: caican <[email protected]> SteNicholas <[email protected]> Co-authored-by: Kang <[email protected]> Co-authored-by: Xun Liu <[email protected]> Co-authored-by: Qian Xia <[email protected]> Co-authored-by: caican <[email protected]>
apache · Jun 28, 2024 · e5b3eb9 · e5b3eb9
1 parent ef91358
commit e5b3eb9
Show file tree

Hide file tree

Showing 8 changed files with 102 additions and 103 deletions.
diff --git a/...c/test/java/com/datastrato/gravitino/catalog/hadoop/integration/test/HadoopCatalogIT.java b/...c/test/java/com/datastrato/gravitino/catalog/hadoop/integration/test/HadoopCatalogIT.java
@@ -6,6 +6,7 @@
 
 import com.datastrato.gravitino.Catalog;
 import com.datastrato.gravitino.NameIdentifier;
+import com.datastrato.gravitino.Namespace;
 import com.datastrato.gravitino.Schema;
 import com.datastrato.gravitino.client.GravitinoMetalake;
 import com.datastrato.gravitino.exceptions.FilesetAlreadyExistsException;
@@ -16,7 +17,6 @@
 import com.datastrato.gravitino.integration.test.container.HiveContainer;
 import com.datastrato.gravitino.integration.test.util.AbstractIT;
 import com.datastrato.gravitino.integration.test.util.GravitinoITUtils;
-import com.datastrato.gravitino.utils.NamespaceUtil;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Maps;
 import java.io.IOException;
@@ -258,8 +258,7 @@ public void testExternalFileset() throws IOException {
   void testNameSpec() {
     String illegalName = "/%~?*";
 
-    NameIdentifier nameIdentifier =
-        NameIdentifier.of(metalakeName, catalogName, schemaName, illegalName);
+    NameIdentifier nameIdentifier = NameIdentifier.of(schemaName, illegalName);
 
     Assertions.assertThrows(
         NoSuchFilesetException.class, () -> catalog.asFilesetCatalog().loadFileset(nameIdentifier));
@@ -282,9 +281,7 @@ public void testLoadFileset() throws IOException {
 
     // test load fileset
     Fileset loadFileset =
-        catalog
-            .asFilesetCatalog()
-            .loadFileset(NameIdentifier.of(metalakeName, catalogName, schemaName, filesetName));
+        catalog.asFilesetCatalog().loadFileset(NameIdentifier.of(schemaName, filesetName));
     Assertions.assertEquals(fileset.name(), loadFileset.name(), "fileset should be loaded");
     Assertions.assertEquals(fileset.comment(), loadFileset.comment(), "comment should be loaded");
     Assertions.assertEquals(fileset.type(), loadFileset.type(), "type should be loaded");
@@ -298,10 +295,7 @@ public void testLoadFileset() throws IOException {
     // test load a fileset that not exist
     Assertions.assertThrows(
         NoSuchFilesetException.class,
-        () ->
-            catalog
-                .asFilesetCatalog()
-                .loadFileset(NameIdentifier.of(metalakeName, catalogName, schemaName, "not_exist")),
+        () -> catalog.asFilesetCatalog().loadFileset(NameIdentifier.of(schemaName, "not_exist")),
         "Should throw NoSuchFilesetException when fileset does not exist");
   }
 
@@ -320,16 +314,12 @@ public void testDropManagedFileset() throws IOException {
 
     // drop fileset
     boolean dropped =
-        catalog
-            .asFilesetCatalog()
-            .dropFileset(NameIdentifier.of(metalakeName, catalogName, schemaName, filesetName));
+        catalog.asFilesetCatalog().dropFileset(NameIdentifier.of(schemaName, filesetName));
     Assertions.assertTrue(dropped, "fileset should be dropped");
 
     // verify fileset is dropped
     Assertions.assertFalse(
-        catalog
-            .asFilesetCatalog()
-            .filesetExists(NameIdentifier.of(metalakeName, catalogName, schemaName, filesetName)),
+        catalog.asFilesetCatalog().filesetExists(NameIdentifier.of(schemaName, filesetName)),
         "fileset should not be exists");
     Assertions.assertFalse(
         hdfs.exists(new Path(storageLocation)), "storage location should be dropped");
@@ -351,16 +341,12 @@ public void testDropExternalFileset() throws IOException {
 
     // drop fileset
     boolean dropped =
-        catalog
-            .asFilesetCatalog()
-            .dropFileset(NameIdentifier.of(metalakeName, catalogName, schemaName, filesetName));
+        catalog.asFilesetCatalog().dropFileset(NameIdentifier.of(schemaName, filesetName));
     Assertions.assertTrue(dropped, "fileset should be dropped");
 
     // verify fileset is dropped
     Assertions.assertFalse(
-        catalog
-            .asFilesetCatalog()
-            .filesetExists(NameIdentifier.of(metalakeName, catalogName, schemaName, filesetName)),
+        catalog.asFilesetCatalog().filesetExists(NameIdentifier.of(schemaName, filesetName)),
         "fileset should not be exists");
     Assertions.assertTrue(
         hdfs.exists(new Path(storageLocation)), "storage location should not be dropped");
@@ -374,9 +360,7 @@ public void testListFilesets() throws IOException {
 
     // test no fileset exists
     NameIdentifier[] nameIdentifiers =
-        catalog
-            .asFilesetCatalog()
-            .listFilesets(NamespaceUtil.ofFileset(metalakeName, catalogName, schemaName));
+        catalog.asFilesetCatalog().listFilesets(Namespace.of(schemaName));
     Assertions.assertEquals(0, nameIdentifiers.length, "should have no fileset");
 
     // create fileset1
@@ -407,9 +391,7 @@ public void testListFilesets() throws IOException {
 
     // list filesets
     NameIdentifier[] nameIdentifiers1 =
-        catalog
-            .asFilesetCatalog()
-            .listFilesets(NamespaceUtil.ofFileset(metalakeName, catalogName, schemaName));
+        catalog.asFilesetCatalog().listFilesets(Namespace.of(schemaName));
     Arrays.sort(nameIdentifiers1, Comparator.comparing(NameIdentifier::name));
     Assertions.assertEquals(2, nameIdentifiers1.length, "should have 2 filesets");
     Assertions.assertEquals(fileset1.name(), nameIdentifiers1[0].name());
@@ -432,8 +414,7 @@ public void testRenameFileset() throws IOException {
         catalog
             .asFilesetCatalog()
             .alterFileset(
-                NameIdentifier.of(metalakeName, catalogName, schemaName, filesetName),
-                FilesetChange.rename(newFilesetName));
+                NameIdentifier.of(schemaName, filesetName), FilesetChange.rename(newFilesetName));
 
     // verify fileset is updated
     Assertions.assertNotNull(newFileset, "fileset should be created");
@@ -463,7 +444,7 @@ public void testFilesetUpdateComment() throws IOException {
         catalog
             .asFilesetCatalog()
             .alterFileset(
-                NameIdentifier.of(metalakeName, catalogName, schemaName, filesetName),
+                NameIdentifier.of(schemaName, filesetName),
                 FilesetChange.updateComment(newComment));
     assertFilesetExists(filesetName);
 
@@ -493,8 +474,7 @@ public void testFilesetSetProperties() throws IOException {
         catalog
             .asFilesetCatalog()
             .alterFileset(
-                NameIdentifier.of(metalakeName, catalogName, schemaName, filesetName),
-                FilesetChange.setProperty("k1", "v2"));
+                NameIdentifier.of(schemaName, filesetName), FilesetChange.setProperty("k1", "v2"));
     assertFilesetExists(filesetName);
 
     // verify fileset is updated
@@ -523,8 +503,7 @@ public void testFilesetRemoveProperties() throws IOException {
         catalog
             .asFilesetCatalog()
             .alterFileset(
-                NameIdentifier.of(metalakeName, catalogName, schemaName, filesetName),
-                FilesetChange.removeProperty("k1"));
+                NameIdentifier.of(schemaName, filesetName), FilesetChange.removeProperty("k1"));
     assertFilesetExists(filesetName);
 
     // verify fileset is updated
@@ -580,18 +559,12 @@ private Fileset createFileset(
     return catalog
         .asFilesetCatalog()
         .createFileset(
-            NameIdentifier.of(metalakeName, catalogName, schemaName, filesetName),
-            comment,
-            type,
-            storageLocation,
-            properties);
+            NameIdentifier.of(schemaName, filesetName), comment, type, storageLocation, properties);
   }
 
   private void assertFilesetExists(String filesetName) throws IOException {
     Assertions.assertTrue(
-        catalog
-            .asFilesetCatalog()
-            .filesetExists(NameIdentifier.of(metalakeName, catalogName, schemaName, filesetName)),
+        catalog.asFilesetCatalog().filesetExists(NameIdentifier.of(schemaName, filesetName)),
         "fileset should be exists");
     Assertions.assertTrue(
         hdfs.exists(new Path(storageLocation(filesetName))), "storage location should be exists");

diff --git a/.../com/datastrato/gravitino/catalog/hadoop/integration/test/HadoopUserAuthenticationIT.java b/.../com/datastrato/gravitino/catalog/hadoop/integration/test/HadoopUserAuthenticationIT.java
@@ -197,15 +197,13 @@ public void testUserAuthentication() {
     catalog
         .asFilesetCatalog()
         .createFileset(
-            NameIdentifier.of(METALAKE_NAME, CATALOG_NAME, SCHEMA_NAME, TABLE_NAME),
+            NameIdentifier.of(SCHEMA_NAME, TABLE_NAME),
             "comment",
             Fileset.Type.MANAGED,
             null,
             ImmutableMap.of());
 
-    catalog
-        .asFilesetCatalog()
-        .dropFileset(NameIdentifier.of(METALAKE_NAME, CATALOG_NAME, SCHEMA_NAME, TABLE_NAME));
+    catalog.asFilesetCatalog().dropFileset(NameIdentifier.of(SCHEMA_NAME, TABLE_NAME));
 
     catalog.asSchemas().alterSchema(SCHEMA_NAME, SchemaChange.setProperty("k1", "value1"));
 

diff --git a/...a/com/datastrato/gravitino/catalog/hadoop/integration/test/HadoopUserImpersonationIT.java b/...a/com/datastrato/gravitino/catalog/hadoop/integration/test/HadoopUserImpersonationIT.java
@@ -293,11 +293,7 @@ private Fileset createFileset(
     return catalog
         .asFilesetCatalog()
         .createFileset(
-            NameIdentifier.of(metalakeName, catalogName, schemaName, filesetName),
-            comment,
-            type,
-            storageLocation,
-            properties);
+            NameIdentifier.of(schemaName, filesetName), comment, type, storageLocation, properties);
   }
 
   private boolean checkFilePathExists(String pathString) throws Exception {

diff --git a/clients/client-java/src/main/java/com/datastrato/gravitino/client/FilesetCatalog.java b/clients/client-java/src/main/java/com/datastrato/gravitino/client/FilesetCatalog.java
@@ -59,40 +59,45 @@ public com.datastrato.gravitino.file.FilesetCatalog asFilesetCatalog()
   /**
    * List the filesets in a schema namespace from the catalog.
    *
-   * @param namespace A schema namespace.
-   * @return An array of fileset identifiers in the namespace.
+   * @param namespace A schema namespace. This namespace should have 1 level, which is the schema
+   *     name;
+   * @return An array of {@link NameIdentifier} of filesets under the given namespace.
    * @throws NoSuchSchemaException If the schema does not exist.
    */
   @Override
   public NameIdentifier[] listFilesets(Namespace namespace) throws NoSuchSchemaException {
     checkFilesetNamespace(namespace);
 
+    Namespace fullNamespace = getFilesetFullNamespace(namespace);
     EntityListResponse resp =
         restClient.get(
-            formatFilesetRequestPath(namespace),
+            formatFilesetRequestPath(fullNamespace),
             EntityListResponse.class,
             Collections.emptyMap(),
             ErrorHandlers.filesetErrorHandler());
     resp.validate();
 
-    return resp.identifiers();
+    return Arrays.stream(resp.identifiers())
+        .map(ident -> NameIdentifier.of(ident.namespace().level(2), ident.name()))
+        .toArray(NameIdentifier[]::new);
   }
 
   /**
-   * Load fileset metadata by {@link NameIdentifier} from the catalog.
+   * Load fileset metadata by {@link NameIdentifier} from the catalog, which should be a
+   * "schema.fileset" style.
    *
    * @param ident A fileset identifier.
    * @return The fileset metadata.
    * @throws NoSuchFilesetException If the fileset does not exist.
    */
   @Override
   public Fileset loadFileset(NameIdentifier ident) throws NoSuchFilesetException {
-    checkFilesetNameIdentifer(ident);
+    checkFilesetNameIdentifier(ident);
+
+    Namespace fullNamespace = getFilesetFullNamespace(ident.namespace());
     FilesetResponse resp =
         restClient.get(
-            formatFilesetRequestPath(ident.namespace())
-                + "/"
-                + RESTUtils.encodeString(ident.name()),
+            formatFilesetRequestPath(fullNamespace) + "/" + RESTUtils.encodeString(ident.name()),
             FilesetResponse.class,
             Collections.emptyMap(),
             ErrorHandlers.filesetErrorHandler());
@@ -109,7 +114,7 @@ public Fileset loadFileset(NameIdentifier ident) throws NoSuchFilesetException {
    *
    * <p>If the type of the fileset object is "EXTERNAL", the underlying storageLocation must be set.
    *
-   * @param ident A fileset identifier.
+   * @param ident A fileset identifier, which should be a "schema.fileset" style.
    * @param comment The comment of the fileset.
    * @param type The type of the fileset.
    * @param storageLocation The storage location of the fileset.
@@ -126,7 +131,9 @@ public Fileset createFileset(
       String storageLocation,
       Map<String, String> properties)
       throws NoSuchSchemaException, FilesetAlreadyExistsException {
-    checkFilesetNameIdentifer(ident);
+    checkFilesetNameIdentifier(ident);
+
+    Namespace fullNamespace = getFilesetFullNamespace(ident.namespace());
     FilesetCreateRequest req =
         FilesetCreateRequest.builder()
             .name(RESTUtils.encodeString(ident.name()))
@@ -138,7 +145,7 @@ public Fileset createFileset(
 
     FilesetResponse resp =
         restClient.post(
-            formatFilesetRequestPath(ident.namespace()),
+            formatFilesetRequestPath(fullNamespace),
             req,
             FilesetResponse.class,
             Collections.emptyMap(),
@@ -151,7 +158,7 @@ public Fileset createFileset(
   /**
    * Update a fileset metadata in the catalog.
    *
-   * @param ident A fileset identifier.
+   * @param ident A fileset identifier, which should be a "schema.fileset" style.
    * @param changes The changes to apply to the fileset.
    * @return The updated fileset metadata.
    * @throws NoSuchFilesetException If the fileset does not exist.
@@ -160,7 +167,9 @@ public Fileset createFileset(
   @Override
   public Fileset alterFileset(NameIdentifier ident, FilesetChange... changes)
       throws NoSuchFilesetException, IllegalArgumentException {
-    checkFilesetNameIdentifer(ident);
+    checkFilesetNameIdentifier(ident);
+
+    Namespace fullNamespace = getFilesetFullNamespace(ident.namespace());
     List<FilesetUpdateRequest> updates =
         Arrays.stream(changes)
             .map(DTOConverters::toFilesetUpdateRequest)
@@ -170,7 +179,7 @@ public Fileset alterFileset(NameIdentifier ident, FilesetChange... changes)
 
     FilesetResponse resp =
         restClient.put(
-            formatFilesetRequestPath(ident.namespace()) + "/" + ident.name(),
+            formatFilesetRequestPath(fullNamespace) + "/" + ident.name(),
             req,
             FilesetResponse.class,
             Collections.emptyMap(),
@@ -186,15 +195,17 @@ public Fileset alterFileset(NameIdentifier ident, FilesetChange... changes)
    * <p>The underlying files will be deleted if this fileset type is managed, otherwise, only the
    * metadata will be dropped.
    *
-   * @param ident A fileset identifier.
+   * @param ident A fileset identifier, which should be a "schema.fileset" style.
    * @return true If the fileset is dropped, false the fileset did not exist.
    */
   @Override
   public boolean dropFileset(NameIdentifier ident) {
-    checkFilesetNameIdentifer(ident);
+    checkFilesetNameIdentifier(ident);
+
+    Namespace fullNamespace = getFilesetFullNamespace(ident.namespace());
     DropResponse resp =
         restClient.delete(
-            formatFilesetRequestPath(ident.namespace()) + "/" + ident.name(),
+            formatFilesetRequestPath(fullNamespace) + "/" + ident.name(),
             DropResponse.class,
             Collections.emptyMap(),
             ErrorHandlers.filesetErrorHandler());
@@ -221,8 +232,8 @@ static String formatFilesetRequestPath(Namespace ns) {
    */
   static void checkFilesetNamespace(Namespace namespace) {
     Namespace.check(
-        namespace != null && namespace.length() == 3,
-        "Fileset namespace must be non-null and have 3 level, the input namespace is %s",
+        namespace != null && namespace.length() == 1,
+        "Fileset namespace must be non-null and have 1 level, the input namespace is %s",
         namespace);
   }
 
@@ -231,13 +242,17 @@ static void checkFilesetNamespace(Namespace namespace) {
    *
    * @param ident The NameIdentifier to check
    */
-  static void checkFilesetNameIdentifer(NameIdentifier ident) {
-    NameIdentifier.check(ident != null, "NameIdentifer must not be null");
+  static void checkFilesetNameIdentifier(NameIdentifier ident) {
+    NameIdentifier.check(ident != null, "NameIdentifier must not be null");
     NameIdentifier.check(
-        ident.name() != null && !ident.name().isEmpty(), "NameIdentifer name must not be empty");
+        ident.name() != null && !ident.name().isEmpty(), "NameIdentifier name must not be empty");
     checkFilesetNamespace(ident.namespace());
   }
 
+  private Namespace getFilesetFullNamespace(Namespace tableNamespace) {
+    return Namespace.of(this.catalogNamespace().level(0), this.name(), tableNamespace.level(0));
+  }
+
   /**
    * Create a new builder for the fileset catalog.
    *