Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[#3701]refactor(API): Refactoring SupportsSchemas.listSchemas() method to return String[] #3722

Merged
merged 4 commits into from
Jun 4, 2024
Merged

[#3701]refactor(API): Refactoring SupportsSchemas.listSchemas() method to return String[] #3722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bc65127
[#3701]Refactoring SupportsSchemas.listSchemas() method
shaofengshi Jun 3, 2024
5c22b05
fix integration test
shaofengshi Jun 3, 2024
86e0c94
polish the code
shaofengshi Jun 3, 2024
1982197
fix javadoc
shaofengshi Jun 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions api/src/main/java/com/datastrato/gravitino/SupportsSchemas.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,12 @@ public interface SupportsSchemas {
*
* <p>If an entity such as a table, view exists, its parent schemas must also exist and must be
* returned by this discovery method. For example, if table a.b.t exists, this method invoked as
* listSchemas(a) must return [a.b] in the result array
* listSchemas(a) must return [b] in the result array
*
* @return An array of schema identifier under the namespace.
* @return An array of schema names under the namespace.
mchades marked this conversation as resolved.
Show resolved Hide resolved
* @throws NoSuchCatalogException If the catalog does not exist.
*/
NameIdentifier[] listSchemas() throws NoSuchCatalogException;
String[] listSchemas() throws NoSuchCatalogException;

/**
* Check if a schema exists.
Expand Down
6 changes: 3 additions & 3 deletions ...e/src/test/java/com/datastrato/gravitino/catalog/hive/integration/test/CatalogHiveIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,10 +200,10 @@ public static void startup() throws Exception {
@AfterAll
public static void stop() throws IOException {
Arrays.stream(catalog.asSchemas().listSchemas())
.filter(ident -> !ident.name().equals("default"))
.filter(schema -> !schema.equals("default"))
.forEach(
(ident -> {
catalog.asSchemas().dropSchema(ident.name(), true);
(schema -> {
catalog.asSchemas().dropSchema(schema, true);
}));
Arrays.stream(metalake.listCatalogs())
.forEach(
Expand Down
21 changes: 7 additions & 14 deletions ...src/test/java/com/datastrato/gravitino/catalog/doris/integration/test/CatalogDorisIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,9 @@
import java.io.IOException;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Assertions;
Expand Down Expand Up @@ -185,20 +184,16 @@ void testDorisSchemaBasicOperation() {
SupportsSchemas schemas = catalog.asSchemas();

// test list schemas
NameIdentifier[] nameIdentifiers = schemas.listSchemas();
Set<String> schemaNames =
Arrays.stream(nameIdentifiers).map(NameIdentifier::name).collect(Collectors.toSet());
Assertions.assertTrue(schemaNames.contains(schemaName));
String[] schemaNames = schemas.listSchemas();
Assertions.assertTrue(Arrays.asList(schemaNames).contains(schemaName));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you can check if you can use ArrayUtils.contains() without introducing extra dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand; Arrays is the JDK utility class, while ArrayUtils is apache commons lang; As this is so simple a case, I don't think it is necessary to use ArrayUtils here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a personal preference and the current implementation looks good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks~


// test create schema already exists
String testSchemaName = GravitinoITUtils.genRandomName("create_schema_test");
NameIdentifier schemaIdent = NameIdentifier.of(metalakeName, catalogName, testSchemaName);
schemas.createSchema(schemaIdent.name(), schema_comment, Collections.emptyMap());

nameIdentifiers = schemas.listSchemas();
Map<String, NameIdentifier> schemaMap =
Arrays.stream(nameIdentifiers).collect(Collectors.toMap(NameIdentifier::name, v -> v));
Assertions.assertTrue(schemaMap.containsKey(testSchemaName));
List<String> schemaNameList = Arrays.asList(schemas.listSchemas());
Assertions.assertTrue(schemaNameList.contains(testSchemaName));

Assertions.assertThrows(
SchemaAlreadyExistsException.class,
Expand All @@ -215,10 +210,8 @@ void testDorisSchemaBasicOperation() {
NoSuchSchemaException.class, () -> schemas.loadSchema(schemaIdent.name()));

// 2. check by list schema
nameIdentifiers = schemas.listSchemas();
schemaMap =
Arrays.stream(nameIdentifiers).collect(Collectors.toMap(NameIdentifier::name, v -> v));
Assertions.assertFalse(schemaMap.containsKey(testSchemaName));
schemaNameList = Arrays.asList(schemas.listSchemas());
Assertions.assertFalse(schemaNameList.contains(testSchemaName));

// test drop schema not exists
NameIdentifier notExistsSchemaIdent = NameIdentifier.of(metalakeName, catalogName, "no-exits");
Expand Down
26 changes: 10 additions & 16 deletions ...src/test/java/com/datastrato/gravitino/catalog/mysql/integration/test/CatalogMysqlIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import com.datastrato.gravitino.rel.types.Types;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import java.io.IOException;
import java.sql.SQLException;
import java.util.Arrays;
Expand Down Expand Up @@ -243,9 +244,8 @@ void testOperationMysqlSchema() {
SupportsSchemas schemas = catalog.asSchemas();
Namespace namespace = Namespace.of(metalakeName, catalogName);
// list schema check.
NameIdentifier[] nameIdentifiers = schemas.listSchemas();
Set<String> schemaNames =
Arrays.stream(nameIdentifiers).map(NameIdentifier::name).collect(Collectors.toSet());
String[] nameIdentifiers = schemas.listSchemas();
Set<String> schemaNames = Sets.newHashSet(nameIdentifiers);
Assertions.assertTrue(schemaNames.contains(schemaName));

NameIdentifier[] mysqlNamespaces = mysqlService.listSchemas(namespace);
Expand All @@ -258,9 +258,8 @@ void testOperationMysqlSchema() {
NameIdentifier schemaIdent = NameIdentifier.of(metalakeName, catalogName, testSchemaName);
schemas.createSchema(schemaIdent.name(), schema_comment, Collections.emptyMap());
nameIdentifiers = schemas.listSchemas();
Map<String, NameIdentifier> schemaMap =
Arrays.stream(nameIdentifiers).collect(Collectors.toMap(NameIdentifier::name, v -> v));
Assertions.assertTrue(schemaMap.containsKey(testSchemaName));
schemaNames = Sets.newHashSet(nameIdentifiers);
Assertions.assertTrue(schemaNames.contains(testSchemaName));

mysqlNamespaces = mysqlService.listSchemas(namespace);
schemaNames =
Expand All @@ -282,9 +281,8 @@ void testOperationMysqlSchema() {
NoSuchSchemaException.class, () -> mysqlService.loadSchema(schemaIdent));

nameIdentifiers = schemas.listSchemas();
schemaMap =
Arrays.stream(nameIdentifiers).collect(Collectors.toMap(NameIdentifier::name, v -> v));
Assertions.assertFalse(schemaMap.containsKey(testSchemaName));
schemaNames = Sets.newHashSet(nameIdentifiers);
Assertions.assertFalse(schemaNames.contains(testSchemaName));
Assertions.assertFalse(schemas.dropSchema("no-exits", false));
TableCatalog tableCatalog = catalog.asTableCatalog();

Expand Down Expand Up @@ -1327,9 +1325,8 @@ void testNameSpec() {
Schema schema = catalog.asSchemas().loadSchema(testSchemaName);
Assertions.assertEquals(testSchemaName, schema.name());

NameIdentifier[] schemaIdents = catalog.asSchemas().listSchemas();
Assertions.assertTrue(
Arrays.stream(schemaIdents).anyMatch(s -> s.name().equals(testSchemaName)));
String[] schemaIdents = catalog.asSchemas().listSchemas();
Assertions.assertTrue(Arrays.stream(schemaIdents).anyMatch(s -> s.equals(testSchemaName)));

Assertions.assertTrue(catalog.asSchemas().dropSchema(testSchemaName, false));
Assertions.assertFalse(catalog.asSchemas().schemaExists(testSchemaName));
Expand Down Expand Up @@ -1374,10 +1371,7 @@ void testMySQLSchemaNameCaseSensitive() {
Assertions.assertNotNull(schemaSupport.loadSchema(schema));
}

Set<String> schemaNames =
Arrays.stream(schemaSupport.listSchemas())
.map(NameIdentifier::name)
.collect(Collectors.toSet());
Set<String> schemaNames = Sets.newHashSet(schemaSupport.listSchemas());

Assertions.assertTrue(schemaNames.containsAll(Arrays.asList(schemas)));

Expand Down
33 changes: 13 additions & 20 deletions ...ava/com/datastrato/gravitino/catalog/postgresql/integration/test/CatalogPostgreSqlIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import com.datastrato.gravitino.rel.types.Types.IntegerType;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import java.io.IOException;
import java.sql.DriverManager;
import java.sql.SQLException;
Expand Down Expand Up @@ -108,9 +109,9 @@ public void startup() throws IOException, SQLException {
@AfterAll
public void stop() {
clearTableAndSchema();
NameIdentifier[] schemaIdentifiers = catalog.asSchemas().listSchemas();
for (NameIdentifier nameIdentifier : schemaIdentifiers) {
catalog.asSchemas().dropSchema(nameIdentifier.name(), true);
String[] schemaNames = catalog.asSchemas().listSchemas();
for (String schemaName : schemaNames) {
catalog.asSchemas().dropSchema(schemaName, true);
}
metalake.dropCatalog(catalogName);
client.dropMetalake(metalakeName);
Expand Down Expand Up @@ -289,9 +290,8 @@ void testOperationPostgreSqlSchema() {
SupportsSchemas schemas = catalog.asSchemas();
Namespace namespace = Namespace.of(metalakeName, catalogName);
// list schema check.
NameIdentifier[] nameIdentifiers = schemas.listSchemas();
Set<String> schemaNames =
Arrays.stream(nameIdentifiers).map(NameIdentifier::name).collect(Collectors.toSet());
String[] nameIdentifiers = schemas.listSchemas();
Set<String> schemaNames = Sets.newHashSet(nameIdentifiers);
Assertions.assertTrue(schemaNames.contains(schemaName));

NameIdentifier[] postgreSqlNamespaces = postgreSqlService.listSchemas(namespace);
Expand All @@ -304,9 +304,8 @@ void testOperationPostgreSqlSchema() {
NameIdentifier schemaIdent = NameIdentifier.of(metalakeName, catalogName, testSchemaName);
schemas.createSchema(schemaIdent.name(), schema_comment, Collections.emptyMap());
nameIdentifiers = schemas.listSchemas();
Map<String, NameIdentifier> schemaMap =
Arrays.stream(nameIdentifiers).collect(Collectors.toMap(NameIdentifier::name, v -> v));
Assertions.assertTrue(schemaMap.containsKey(testSchemaName));
schemaNames = Sets.newHashSet(nameIdentifiers);
Assertions.assertTrue(schemaNames.contains(testSchemaName));

postgreSqlNamespaces = postgreSqlService.listSchemas(namespace);
schemaNames =
Expand All @@ -328,9 +327,8 @@ void testOperationPostgreSqlSchema() {
NoSuchSchemaException.class, () -> postgreSqlService.loadSchema(schemaIdent));

nameIdentifiers = schemas.listSchemas();
schemaMap =
Arrays.stream(nameIdentifiers).collect(Collectors.toMap(NameIdentifier::name, v -> v));
Assertions.assertFalse(schemaMap.containsKey(testSchemaName));
schemaNames = Sets.newHashSet(nameIdentifiers);
Assertions.assertFalse(schemaNames.contains(testSchemaName));
Assertions.assertFalse(schemas.dropSchema("no_exits", false));
TableCatalog tableCatalog = catalog.asTableCatalog();

Expand Down Expand Up @@ -545,9 +543,8 @@ void testCreateAndLoadSchema() {

@Test
void testListSchema() {
NameIdentifier[] nameIdentifiers = catalog.asSchemas().listSchemas();
Set<String> schemaNames =
Arrays.stream(nameIdentifiers).map(NameIdentifier::name).collect(Collectors.toSet());
String[] nameIdentifiers = catalog.asSchemas().listSchemas();
Set<String> schemaNames = Sets.newHashSet(nameIdentifiers);
Assertions.assertTrue(schemaNames.contains("public"));
}

Expand Down Expand Up @@ -1216,11 +1213,7 @@ void testPostgreSQLSchemaNameCaseSensitive() {
Assertions.assertNotNull(schemaSupport.loadSchema(schema));
}

Set<String> schemaNames =
Arrays.stream(schemaSupport.listSchemas())
.map(NameIdentifier::name)
.collect(Collectors.toSet());

Set<String> schemaNames = Sets.newHashSet(schemaSupport.listSchemas());
Assertions.assertTrue(schemaNames.containsAll(Arrays.asList(schemas)));

String tableName = "test1";
Expand Down
4 changes: 2 additions & 2 deletions ...va/com/datastrato/gravitino/catalog/postgresql/integration/test/TestMultipleJDBCLoad.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,11 @@ public void testCreateMultipleJdbc() throws URISyntaxException, SQLException {
metalake.createCatalog(
mysqlCatalogName, Catalog.Type.RELATIONAL, "jdbc-mysql", "comment", mysqlConf);

NameIdentifier[] nameIdentifiers = mysqlCatalog.asSchemas().listSchemas();
String[] nameIdentifiers = mysqlCatalog.asSchemas().listSchemas();
Assertions.assertNotEquals(0, nameIdentifiers.length);
nameIdentifiers = postgreSqlCatalog.asSchemas().listSchemas();
Assertions.assertEquals(1, nameIdentifiers.length);
Assertions.assertEquals("public", nameIdentifiers[0].name());
Assertions.assertEquals("public", nameIdentifiers[0]);

String schemaName = RandomNameUtils.genRandomName("it_schema");
mysqlCatalog.asSchemas().createSchema(schemaName, null, Collections.emptyMap());
Expand Down
12 changes: 6 additions & 6 deletions ...src/test/java/com/datastrato/gravitino/catalog/kafka/integration/test/CatalogKafkaIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,10 +101,10 @@ public static void startUp() throws ExecutionException, InterruptedException {
public static void shutdown() {
Catalog catalog = metalake.loadCatalog(CATALOG_NAME);
Arrays.stream(catalog.asSchemas().listSchemas())
.filter(ident -> !ident.name().equals("default"))
.filter(ident -> !ident.equals("default"))
.forEach(
(ident -> {
catalog.asSchemas().dropSchema(ident.name(), true);
catalog.asSchemas().dropSchema(ident, true);
}));
Arrays.stream(metalake.listCatalogs())
.forEach(
Expand Down Expand Up @@ -223,9 +223,9 @@ public void testCatalogException() {

@Test
public void testDefaultSchema() {
NameIdentifier[] schemas = catalog.asSchemas().listSchemas();
String[] schemas = catalog.asSchemas().listSchemas();
Assertions.assertEquals(1, schemas.length);
Assertions.assertEquals(DEFAULT_SCHEMA_NAME, schemas[0].name());
Assertions.assertEquals(DEFAULT_SCHEMA_NAME, schemas[0]);

Schema loadSchema = catalog.asSchemas().loadSchema(DEFAULT_SCHEMA_NAME);
Assertions.assertEquals(
Expand Down Expand Up @@ -262,9 +262,9 @@ public void testCreateSchema() {

@Test
public void testListSchema() {
NameIdentifier[] schemas = catalog.asSchemas().listSchemas();
String[] schemas = catalog.asSchemas().listSchemas();
Assertions.assertEquals(1, schemas.length);
Assertions.assertEquals(DEFAULT_SCHEMA_NAME, schemas[0].name());
Assertions.assertEquals(DEFAULT_SCHEMA_NAME, schemas[0]);
}

@Test
Expand Down
18 changes: 7 additions & 11 deletions ...datastrato/gravitino/catalog/lakehouse/iceberg/integration/test/CatalogIcebergBaseIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand Down Expand Up @@ -251,9 +252,7 @@ private Map<String, String> createProperties() {
void testOperationIcebergSchema() {
SupportsSchemas schemas = catalog.asSchemas();
// list schema check.
NameIdentifier[] nameIdentifiers = schemas.listSchemas();
Set<String> schemaNames =
Arrays.stream(nameIdentifiers).map(NameIdentifier::name).collect(Collectors.toSet());
Set<String> schemaNames = new HashSet<>(Arrays.asList(schemas.listSchemas()));
Assertions.assertTrue(schemaNames.contains(schemaName));

List<org.apache.iceberg.catalog.Namespace> icebergNamespaces =
Expand All @@ -266,10 +265,9 @@ void testOperationIcebergSchema() {
String testSchemaName = GravitinoITUtils.genRandomName("test_schema_1");
NameIdentifier schemaIdent = NameIdentifier.of(metalakeName, catalogName, testSchemaName);
schemas.createSchema(schemaIdent.name(), schema_comment, Collections.emptyMap());
nameIdentifiers = schemas.listSchemas();
Map<String, NameIdentifier> schemaMap =
Arrays.stream(nameIdentifiers).collect(Collectors.toMap(NameIdentifier::name, v -> v));
Assertions.assertTrue(schemaMap.containsKey(testSchemaName));

schemaNames = new HashSet<>(Arrays.asList(schemas.listSchemas()));
Assertions.assertTrue(schemaNames.contains(testSchemaName));

icebergNamespaces =
icebergSupportsNamespaces.listNamespaces(IcebergTableOpsHelper.getIcebergNamespace());
Expand Down Expand Up @@ -305,10 +303,8 @@ void testOperationIcebergSchema() {
icebergSupportsNamespaces.loadNamespaceMetadata(icebergNamespace);
});

nameIdentifiers = schemas.listSchemas();
schemaMap =
Arrays.stream(nameIdentifiers).collect(Collectors.toMap(NameIdentifier::name, v -> v));
Assertions.assertFalse(schemaMap.containsKey(testSchemaName));
schemaNames = new HashSet<>(Arrays.asList(schemas.listSchemas()));
Assertions.assertFalse(schemaNames.contains(testSchemaName));
Assertions.assertFalse(schemas.dropSchema("no-exits", false));
TableCatalog tableCatalog = catalog.asTableCatalog();

Expand Down
2 changes: 1 addition & 1 deletion ...datastrato/gravitino/catalog/lakehouse/iceberg/integration/test/TestMultipleJDBCLoad.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public void testCreateMultipleJdbcInIceberg() throws URISyntaxException, SQLExce
"comment",
icebergMysqlConf);

NameIdentifier[] nameIdentifiers = mysqlCatalog.asSchemas().listSchemas();
String[] nameIdentifiers = mysqlCatalog.asSchemas().listSchemas();
Assertions.assertEquals(0, nameIdentifiers.length);
nameIdentifiers = postgreSqlCatalog.asSchemas().listSchemas();
Assertions.assertEquals(0, nameIdentifiers.length);
Expand Down
6 changes: 3 additions & 3 deletions clients/client-java/src/main/java/com/datastrato/gravitino/client/BaseSchemaCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,11 @@ public SupportsSchemas asSchemas() throws UnsupportedOperationException {
/**
* List all the schemas under the given catalog namespace.
*
* @return A list of {@link NameIdentifier} of the schemas under the given catalog namespace.
* @return A list of the schema names under the given catalog namespace.
* @throws NoSuchCatalogException if the catalog with specified namespace does not exist.
*/
@Override
public NameIdentifier[] listSchemas() throws NoSuchCatalogException {
public String[] listSchemas() throws NoSuchCatalogException {

EntityListResponse resp =
restClient.get(
Expand All @@ -81,7 +81,7 @@ public NameIdentifier[] listSchemas() throws NoSuchCatalogException {
ErrorHandlers.schemaErrorHandler());
resp.validate();

return resp.identifiers();
return Arrays.stream(resp.identifiers()).map(NameIdentifier::name).toArray(String[]::new);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, will the server side of this interface remain the same?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you respond to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, server side won't change; The "SupportsSchemas" has two version, one is for client, the other is for server side. This change has already been made in the main branch.

}

/**
Expand Down
8 changes: 4 additions & 4 deletions clients/client-java/src/test/java/com/datastrato/gravitino/client/TestRelationalCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,16 +139,16 @@ public void testListSchemas() throws JsonProcessingException {

EntityListResponse resp = new EntityListResponse(new NameIdentifier[] {schema1, schema2});
buildMockResource(Method.GET, schemaPath, null, resp, SC_OK);
NameIdentifier[] schemas = catalog.asSchemas().listSchemas();
String[] schemas = catalog.asSchemas().listSchemas();

Assertions.assertEquals(2, schemas.length);
Assertions.assertEquals(schema1, schemas[0]);
Assertions.assertEquals(schema2, schemas[1]);
Assertions.assertEquals(schema1.name(), schemas[0]);
Assertions.assertEquals(schema2.name(), schemas[1]);

// Test return empty schema list
EntityListResponse emptyResp = new EntityListResponse(new NameIdentifier[] {});
buildMockResource(Method.GET, schemaPath, null, emptyResp, SC_OK);
NameIdentifier[] emptySchemas = catalog.asSchemas().listSchemas();
String[] emptySchemas = catalog.asSchemas().listSchemas();
Assertions.assertEquals(0, emptySchemas.length);

// Test throw NoSuchCatalogException
Expand Down
Loading
Loading