Copilot commented on code in PR #10917: URL: https://github.com/apache/gravitino/pull/10917#discussion_r3166723384
########## catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/integration/test/MotoGlueCatalogIT.java: ########## @@ -0,0 +1,80 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.catalog.glue.integration.test; + +import java.util.Map; +import org.apache.gravitino.catalog.glue.GlueConstants; +import org.apache.gravitino.integration.test.container.GravitinoMotoContainer; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.TestInstance; + +/** + * Runs {@link AbstractGlueCatalogIT} scenarios against a Moto server (free AWS mock). + * + * <p>Requires Docker. Skipped by default when the {@code gravitino-docker-test} tag is excluded. + * Override the container image with the {@code GRAVITINO_CI_MOTO_DOCKER_IMAGE} environment + * variable. + */ +@Tag("gravitino-docker-test") +@TestInstance(TestInstance.Lifecycle.PER_CLASS) +class MotoGlueCatalogIT extends AbstractGlueCatalogIT { + + private static final String FALLBACK_IMAGE = "motoserver/moto:latest"; + Review Comment: Using `motoserver/moto:latest` as a fallback image makes the integration tests non-reproducible and can break unexpectedly when the upstream image changes. Please pin to a known-good Moto version tag (or a digest) and keep the env var override for CI flexibility. ########## integration-test-common/src/test/java/org/apache/gravitino/integration/test/container/GravitinoMotoContainer.java: ########## @@ -0,0 +1,69 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.integration.test.container; + +import com.google.common.collect.ImmutableSet; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import org.testcontainers.containers.Network; + +/** Container wrapping {@code motoserver/moto} — a free AWS mock that supports Glue and more. */ +public class GravitinoMotoContainer extends BaseContainer { + + public static final String DEFAULT_IMAGE = System.getenv("GRAVITINO_CI_MOTO_DOCKER_IMAGE"); + public static final String HOST_NAME = "gravitino-ci-moto"; + public static final int PORT = 5000; + + public GravitinoMotoContainer( + String image, + String hostName, + Set<Integer> ports, + Map<String, String> extraHosts, + Map<String, String> filesToMount, + Map<String, String> envVars, + Optional<Network> network) { + super(image, hostName, ports, extraHosts, filesToMount, envVars, network); + } + + public static Builder builder() { + return new Builder(); + } + + @Override + protected boolean checkContainerStatus(int retryLimit) { + return true; + } + + public static class Builder + extends BaseContainer.Builder<GravitinoMotoContainer.Builder, GravitinoMotoContainer> { + public Builder() { + super(); + this.image = DEFAULT_IMAGE; + this.hostName = HOST_NAME; + this.exposePorts = ImmutableSet.of(PORT); + } Review Comment: `Builder` defaults `image` to `DEFAULT_IMAGE` (env var) which can be null/blank; `BaseContainer` requires a non-null image and will throw at runtime if someone calls `GravitinoMotoContainer.builder().build()` without explicitly setting `withImage(...)`. Consider providing a non-null default image in `GravitinoMotoContainer` itself (or validating and throwing a clear error message in `build()`). ########## catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/integration/test/AbstractGlueCatalogIT.java: ########## @@ -0,0 +1,648 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.catalog.glue.integration.test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.Schema; +import org.apache.gravitino.SchemaChange; +import org.apache.gravitino.catalog.glue.GlueCatalogOperations; +import org.apache.gravitino.catalog.glue.GlueConstants; +import org.apache.gravitino.exceptions.NonEmptySchemaException; +import org.apache.gravitino.rel.Column; +import org.apache.gravitino.rel.SupportsPartitions; +import org.apache.gravitino.rel.Table; +import org.apache.gravitino.rel.TableChange; +import org.apache.gravitino.rel.expressions.NamedReference; +import org.apache.gravitino.rel.expressions.distributions.Distributions; +import org.apache.gravitino.rel.expressions.literals.Literal; +import org.apache.gravitino.rel.expressions.literals.Literals; +import org.apache.gravitino.rel.expressions.sorts.SortDirection; +import org.apache.gravitino.rel.expressions.sorts.SortOrder; +import org.apache.gravitino.rel.expressions.sorts.SortOrders; +import org.apache.gravitino.rel.expressions.transforms.Transform; +import org.apache.gravitino.rel.expressions.transforms.Transforms; +import org.apache.gravitino.rel.indexes.Index; +import org.apache.gravitino.rel.partitions.Partition; +import org.apache.gravitino.rel.partitions.Partitions; +import org.apache.gravitino.rel.types.Types; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInstance; + +/** + * Abstract base class for Glue catalog integration tests. + * + * <p>Subclasses provide backend-specific configuration (LocalStack or real AWS) via {@link + * #catalogConfig()}. All test scenarios are defined here and shared between backends. + */ +@TestInstance(TestInstance.Lifecycle.PER_CLASS) +abstract class AbstractGlueCatalogIT { + + protected GlueCatalogOperations ops; + private String currentSchema; + + private static final Namespace SCHEMA_NS = Namespace.of("ml", "cat"); + private static final String INPUT_FMT = "org.apache.hadoop.mapred.TextInputFormat"; + private static final String OUTPUT_FMT = + "org.apache.hadoop.hive.ql.io.HiveIgnoreKeyTextOutputFormat"; + private static final String SERDE = "org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe"; + + protected abstract Map<String, String> catalogConfig(); + + @BeforeAll + void initOps() { + ops = new GlueCatalogOperations(); + ops.initialize(catalogConfig(), null, null); + } + + @AfterAll + void closeOps() throws Exception { + if (ops != null) { + ops.close(); + } + } + + @AfterEach + void cleanupSchema() { + if (currentSchema != null) { + try { + ops.dropSchema(schemaIdent(currentSchema), true); + } catch (Exception ignored) { + } + currentSchema = null; + } + } + + // ------------------------------------------------------------------------- + // Helpers + // ------------------------------------------------------------------------- + + private String newSchema() { + String name = "glue_it_" + System.nanoTime(); + currentSchema = name; + return name; + } + + private NameIdentifier schemaIdent(String name) { + return NameIdentifier.of("ml", "cat", name); + } + + private Namespace tableNs(String schema) { + return Namespace.of("ml", "cat", schema); + } + + private NameIdentifier tableIdent(String schema, String table) { + return NameIdentifier.of("ml", "cat", schema, table); + } + + private Column[] hiveColumns() { + return new Column[] { + Column.of("id", Types.LongType.get(), "primary key", false, false, null), + Column.of("name", Types.StringType.get(), null), + }; + } + + private Map<String, String> hiveTableProps() { + Map<String, String> props = new HashMap<>(); + props.put(GlueConstants.TABLE_TYPE, "EXTERNAL_TABLE"); + props.put(GlueConstants.INPUT_FORMAT, INPUT_FMT); + props.put(GlueConstants.OUTPUT_FORMAT, OUTPUT_FMT); + props.put(GlueConstants.SERDE_LIB, SERDE); + return props; + } + + private Table createHiveTable(String schema, String table) { + return ops.createTable( + tableIdent(schema, table), + hiveColumns(), + null, + hiveTableProps(), + new Transform[0], + Distributions.NONE, + new SortOrder[0], + new Index[0]); + } + + private Column[] partitionedColumns() { + return new Column[] { + Column.of("id", Types.LongType.get(), null, false, false, null), + Column.of("dt", Types.DateType.get(), null), + }; + } + + private SupportsPartitions createPartitionedTable(String schema, String table) { + Map<String, String> props = hiveTableProps(); + props.put(GlueConstants.LOCATION, "s3://gravitino-test-bucket/" + schema + "/" + table); + ops.createTable( + tableIdent(schema, table), + partitionedColumns(), + null, + props, + new Transform[] {Transforms.identity("dt")}, + Distributions.NONE, + new SortOrder[0], + new Index[0]); + return ops.loadTable(tableIdent(schema, table)).supportPartitions(); + } + + private Partition identityPartition(String dateValue) { + return Partitions.identity( + "dt=" + dateValue, + new String[][] {{"dt"}}, + new Literal[] {Literals.stringLiteral(dateValue)}, + Collections.emptyMap()); + } + + // ------------------------------------------------------------------------- + // Schema tests + // ------------------------------------------------------------------------- + + @Test + void testSchemaProperties() { + String schema = newSchema(); + ops.createSchema(schemaIdent(schema), "test comment", Map.of("k1", "v1")); + + Schema loaded = ops.loadSchema(schemaIdent(schema)); + assertEquals(schema, loaded.name()); + assertEquals("test comment", loaded.comment()); + assertTrue(loaded.properties().containsKey("k1")); + assertEquals("v1", loaded.properties().get("k1")); + } + + @Test + void testListSchemas() { + String schema1 = newSchema(); + String schema2 = "glue_it_" + System.nanoTime() + "_b"; + ops.createSchema(schemaIdent(schema1), null, Collections.emptyMap()); + ops.createSchema(schemaIdent(schema2), null, Collections.emptyMap()); + try { + List<String> names = + Arrays.stream(ops.listSchemas(SCHEMA_NS)) + .map(NameIdentifier::name) + .collect(Collectors.toList()); + assertTrue(names.contains(schema1)); + assertTrue(names.contains(schema2)); + } finally { + try { + ops.dropSchema(schemaIdent(schema2), false); + } catch (Exception ignored) { + } + } + } + + @Test + void testAlterSchema() { + String schema = newSchema(); + ops.createSchema(schemaIdent(schema), null, Collections.emptyMap()); + + ops.alterSchema(schemaIdent(schema), SchemaChange.setProperty("newKey", "newVal")); + Schema loaded = ops.loadSchema(schemaIdent(schema)); + assertEquals("newVal", loaded.properties().get("newKey")); + + ops.alterSchema(schemaIdent(schema), SchemaChange.removeProperty("newKey")); + Schema reloaded = ops.loadSchema(schemaIdent(schema)); + assertFalse(reloaded.properties().containsKey("newKey")); + } + + @Test + void testDropSchemaEmpty() { + String schema = newSchema(); + ops.createSchema(schemaIdent(schema), null, Collections.emptyMap()); + boolean dropped = ops.dropSchema(schemaIdent(schema), false); + assertTrue(dropped); + currentSchema = null; + } + + @Test + void testDropSchemaNonEmpty() { + String schema = newSchema(); + ops.createSchema(schemaIdent(schema), null, Collections.emptyMap()); + createHiveTable(schema, "tbl"); + + assertThrows(NonEmptySchemaException.class, () -> ops.dropSchema(schemaIdent(schema), false)); + + assertTrue(ops.dropSchema(schemaIdent(schema), true)); + currentSchema = null; Review Comment: `testDropSchemaNonEmpty` assumes `dropSchema(..., true)` can successfully drop a schema containing tables. Real AWS Glue typically requires deleting tables before deleting the database (see existing AWS tests that explicitly delete tables before deleting the database), so this can fail (and leak resources) when running `AwsGlueCatalogIT`. Consider explicitly dropping the created table(s) before dropping the schema here, or adjust `cleanupSchema()`/test logic to ensure tables are removed when cascade is requested. ```suggestion try { assertThrows(NonEmptySchemaException.class, () -> ops.dropSchema(schemaIdent(schema), false)); assertTrue(ops.dropTable(tableIdent(schema, "tbl"))); assertTrue(ops.dropSchema(schemaIdent(schema), false)); currentSchema = null; } finally { if (currentSchema != null) { try { ops.dropTable(tableIdent(schema, "tbl")); } catch (Exception ignored) { } try { ops.dropSchema(schemaIdent(schema), false); } catch (Exception ignored) { } currentSchema = null; } } ``` ########## catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/integration/test/AbstractGlueCatalogIT.java: ########## @@ -0,0 +1,648 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.catalog.glue.integration.test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.Schema; +import org.apache.gravitino.SchemaChange; +import org.apache.gravitino.catalog.glue.GlueCatalogOperations; +import org.apache.gravitino.catalog.glue.GlueConstants; +import org.apache.gravitino.exceptions.NonEmptySchemaException; +import org.apache.gravitino.rel.Column; +import org.apache.gravitino.rel.SupportsPartitions; +import org.apache.gravitino.rel.Table; +import org.apache.gravitino.rel.TableChange; +import org.apache.gravitino.rel.expressions.NamedReference; +import org.apache.gravitino.rel.expressions.distributions.Distributions; +import org.apache.gravitino.rel.expressions.literals.Literal; +import org.apache.gravitino.rel.expressions.literals.Literals; +import org.apache.gravitino.rel.expressions.sorts.SortDirection; +import org.apache.gravitino.rel.expressions.sorts.SortOrder; +import org.apache.gravitino.rel.expressions.sorts.SortOrders; +import org.apache.gravitino.rel.expressions.transforms.Transform; +import org.apache.gravitino.rel.expressions.transforms.Transforms; +import org.apache.gravitino.rel.indexes.Index; +import org.apache.gravitino.rel.partitions.Partition; +import org.apache.gravitino.rel.partitions.Partitions; +import org.apache.gravitino.rel.types.Types; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInstance; + +/** + * Abstract base class for Glue catalog integration tests. + * + * <p>Subclasses provide backend-specific configuration (LocalStack or real AWS) via {@link + * #catalogConfig()}. All test scenarios are defined here and shared between backends. Review Comment: The class-level Javadoc still refers to a LocalStack backend, but the shared base is now used by Moto (`MotoGlueCatalogIT`) and real AWS (`AwsGlueCatalogIT`). Please update the wording to avoid stale guidance for future readers. ```suggestion * <p>Subclasses provide backend-specific configuration via {@link #catalogConfig()}. All * test scenarios are defined here and shared between backend implementations. ``` ########## catalogs/catalog-glue/build.gradle.kts: ########## @@ -88,12 +91,7 @@ tasks { } tasks.test { - val skipITs = project.hasProperty("skipITs") - if (skipITs) { - exclude("**/integration/test/**") - } else { - dependsOn(tasks.jar) - } + dependsOn(tasks.jar) Review Comment: This module no longer honors the repository-wide `-PskipITs` convention used by other modules to exclude `**/integration/test/**` (e.g., `catalogs/catalog-hive/build.gradle.kts`). If that’s unintentional, please restore the `skipITs` gate so users/CI can still reliably skip Glue integration tests. ```suggestion dependsOn(tasks.jar) if (project.hasProperty("skipITs")) { exclude("**/integration/test/**") } ``` ########## catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogOperationsTable.java: ########## @@ -0,0 +1,399 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.catalog.glue; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.time.Instant; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.exceptions.NoSuchSchemaException; +import org.apache.gravitino.exceptions.NoSuchTableException; +import org.apache.gravitino.exceptions.TableAlreadyExistsException; +import org.apache.gravitino.rel.Column; +import org.apache.gravitino.rel.TableChange; +import org.apache.gravitino.rel.expressions.distributions.Distributions; +import org.apache.gravitino.rel.expressions.sorts.SortOrders; +import org.apache.gravitino.rel.expressions.transforms.Transforms; +import org.apache.gravitino.rel.indexes.Indexes; +import org.apache.gravitino.rel.types.Types; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; +import software.amazon.awssdk.services.glue.GlueClient; +import software.amazon.awssdk.services.glue.model.AlreadyExistsException; +import software.amazon.awssdk.services.glue.model.CreateTableRequest; +import software.amazon.awssdk.services.glue.model.DeleteTableRequest; +import software.amazon.awssdk.services.glue.model.EntityNotFoundException; +import software.amazon.awssdk.services.glue.model.GetTableRequest; +import software.amazon.awssdk.services.glue.model.GetTableResponse; +import software.amazon.awssdk.services.glue.model.GetTablesRequest; +import software.amazon.awssdk.services.glue.model.GetTablesResponse; +import software.amazon.awssdk.services.glue.model.StorageDescriptor; +import software.amazon.awssdk.services.glue.model.Table; +import software.amazon.awssdk.services.glue.model.UpdateTableRequest; +import software.amazon.awssdk.services.glue.model.UpdateTableResponse; + +class TestGlueCatalogOperationsTable { + + private GlueCatalogOperations ops; + private GlueClient mockClient; + + @BeforeEach + void setup() { + mockClient = mock(GlueClient.class); + ops = new GlueCatalogOperations(); + ops.glueClient = mockClient; + } + + // ------------------------------------------------------------------------- + // listTables + // ------------------------------------------------------------------------- + + @Test + void testListTables_paginated() { + Namespace ns = Namespace.of("metalake", "catalog", "mydb"); + Table t1 = Table.builder().name("t1").build(); + Table t2 = Table.builder().name("t2").build(); + Table t3 = Table.builder().name("t3").build(); + + when(mockClient.getTables(any(GetTablesRequest.class))) + .thenReturn(GetTablesResponse.builder().tableList(t1, t2).nextToken("tok").build()) + .thenReturn(GetTablesResponse.builder().tableList(t3).nextToken(null).build()); + + NameIdentifier[] result = ops.listTables(ns); + + assertEquals(3, result.length); + assertEquals("t1", result[0].name()); + assertEquals("t3", result[2].name()); + } + + @Test + void testListTables_schemaNotFound() { + Namespace ns = Namespace.of("metalake", "catalog", "missing"); + when(mockClient.getTables(any(GetTablesRequest.class))) + .thenThrow(EntityNotFoundException.builder().message("not found").build()); + + assertThrows(NoSuchSchemaException.class, () -> ops.listTables(ns)); + } + + @Test + void testListTables_formatFilter() { + ops.tableFormatFilter = java.util.Set.of("iceberg"); + Namespace ns = Namespace.of("metalake", "catalog", "mydb"); + + Table icebergTable = + Table.builder() + .name("ice_tbl") + .parameters(Map.of(GlueConstants.TABLE_FORMAT, "ICEBERG")) + .build(); + Table hiveTable = Table.builder().name("hive_tbl").parameters(Collections.emptyMap()).build(); + + when(mockClient.getTables(any(GetTablesRequest.class))) + .thenReturn( + GetTablesResponse.builder().tableList(icebergTable, hiveTable).nextToken(null).build()); + + NameIdentifier[] result = ops.listTables(ns); + + assertEquals(1, result.length); + assertEquals("ice_tbl", result[0].name()); + } + + // ------------------------------------------------------------------------- + // loadTable + // ------------------------------------------------------------------------- + + @Test + void testLoadTable_success() { + NameIdentifier ident = NameIdentifier.of("metalake", "catalog", "mydb", "mytable"); + Table glueTable = + Table.builder() + .name("mytable") + .description("desc") + .storageDescriptor( + StorageDescriptor.builder() + .columns( + software.amazon.awssdk.services.glue.model.Column.builder() + .name("id") + .type("bigint") + .build()) + .build()) + .createTime(Instant.now()) + .build(); + when(mockClient.getTable(any(GetTableRequest.class))) + .thenReturn(GetTableResponse.builder().table(glueTable).build()); + + GlueTable result = ops.loadTable(ident); + + assertEquals("mytable", result.name()); + assertEquals("desc", result.comment()); + assertEquals(1, result.columns().length); + assertEquals("id", result.columns()[0].name()); + } + + @Test + void testLoadTable_notFound() { + NameIdentifier ident = NameIdentifier.of("metalake", "catalog", "mydb", "missing"); + when(mockClient.getTable(any(GetTableRequest.class))) + .thenThrow(EntityNotFoundException.builder().message("not found").build()); + + assertThrows(NoSuchTableException.class, () -> ops.loadTable(ident)); + } + + // ------------------------------------------------------------------------- + // createTable + // ------------------------------------------------------------------------- + + @Test + void testCreateTable_success() { + NameIdentifier ident = NameIdentifier.of("metalake", "catalog", "mydb", "mytable"); + Column[] columns = { + GlueColumn.builder().withName("id").withType(Types.LongType.get()).withNullable(true).build(), + GlueColumn.builder() + .withName("name") + .withType(Types.StringType.get()) + .withNullable(true) + .build() + }; + + GlueTable result = + ops.createTable( + ident, + columns, + "my comment", + Map.of(GlueConstants.LOCATION, "s3://bucket/path"), + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + SortOrders.NONE, + Indexes.EMPTY_INDEXES); + + verify(mockClient).createTable(any(CreateTableRequest.class)); + assertEquals("mytable", result.name()); + assertEquals("my comment", result.comment()); + assertEquals(2, result.columns().length); + } + + @Test + void testCreateTable_alreadyExists() { + NameIdentifier ident = NameIdentifier.of("metalake", "catalog", "mydb", "mytable"); + when(mockClient.createTable(any(CreateTableRequest.class))) + .thenThrow(AlreadyExistsException.builder().message("exists").build()); + + assertThrows( + TableAlreadyExistsException.class, + () -> + ops.createTable( + ident, + new Column[0], + null, + Collections.emptyMap(), + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + SortOrders.NONE, + Indexes.EMPTY_INDEXES)); + } + + @Test + void testCreateTable_indexesRejected() { + NameIdentifier ident = NameIdentifier.of("metalake", "catalog", "mydb", "mytable"); + + assertThrows( + IllegalArgumentException.class, + () -> + ops.createTable( + ident, + new Column[0], + null, + Collections.emptyMap(), + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + SortOrders.NONE, + new org.apache.gravitino.rel.indexes.Index[] { + mock(org.apache.gravitino.rel.indexes.Index.class) + })); Review Comment: The fully-qualified `org.apache.gravitino.rel.indexes.Index` usage isn’t needed here (there’s no name collision in this test). Import `Index` and use `new Index[] { ... }` to match the project’s standard import style. ########## catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogOperationsTable.java: ########## @@ -0,0 +1,399 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.catalog.glue; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.time.Instant; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.exceptions.NoSuchSchemaException; +import org.apache.gravitino.exceptions.NoSuchTableException; +import org.apache.gravitino.exceptions.TableAlreadyExistsException; +import org.apache.gravitino.rel.Column; +import org.apache.gravitino.rel.TableChange; +import org.apache.gravitino.rel.expressions.distributions.Distributions; +import org.apache.gravitino.rel.expressions.sorts.SortOrders; +import org.apache.gravitino.rel.expressions.transforms.Transforms; +import org.apache.gravitino.rel.indexes.Indexes; +import org.apache.gravitino.rel.types.Types; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; +import software.amazon.awssdk.services.glue.GlueClient; +import software.amazon.awssdk.services.glue.model.AlreadyExistsException; +import software.amazon.awssdk.services.glue.model.CreateTableRequest; +import software.amazon.awssdk.services.glue.model.DeleteTableRequest; +import software.amazon.awssdk.services.glue.model.EntityNotFoundException; +import software.amazon.awssdk.services.glue.model.GetTableRequest; +import software.amazon.awssdk.services.glue.model.GetTableResponse; +import software.amazon.awssdk.services.glue.model.GetTablesRequest; +import software.amazon.awssdk.services.glue.model.GetTablesResponse; +import software.amazon.awssdk.services.glue.model.StorageDescriptor; +import software.amazon.awssdk.services.glue.model.Table; +import software.amazon.awssdk.services.glue.model.UpdateTableRequest; +import software.amazon.awssdk.services.glue.model.UpdateTableResponse; + +class TestGlueCatalogOperationsTable { + + private GlueCatalogOperations ops; + private GlueClient mockClient; + + @BeforeEach + void setup() { + mockClient = mock(GlueClient.class); + ops = new GlueCatalogOperations(); + ops.glueClient = mockClient; + } + + // ------------------------------------------------------------------------- + // listTables + // ------------------------------------------------------------------------- + + @Test + void testListTables_paginated() { + Namespace ns = Namespace.of("metalake", "catalog", "mydb"); + Table t1 = Table.builder().name("t1").build(); + Table t2 = Table.builder().name("t2").build(); + Table t3 = Table.builder().name("t3").build(); + + when(mockClient.getTables(any(GetTablesRequest.class))) + .thenReturn(GetTablesResponse.builder().tableList(t1, t2).nextToken("tok").build()) + .thenReturn(GetTablesResponse.builder().tableList(t3).nextToken(null).build()); + + NameIdentifier[] result = ops.listTables(ns); + + assertEquals(3, result.length); + assertEquals("t1", result[0].name()); + assertEquals("t3", result[2].name()); + } + + @Test + void testListTables_schemaNotFound() { + Namespace ns = Namespace.of("metalake", "catalog", "missing"); + when(mockClient.getTables(any(GetTablesRequest.class))) + .thenThrow(EntityNotFoundException.builder().message("not found").build()); + + assertThrows(NoSuchSchemaException.class, () -> ops.listTables(ns)); + } + + @Test + void testListTables_formatFilter() { + ops.tableFormatFilter = java.util.Set.of("iceberg"); + Namespace ns = Namespace.of("metalake", "catalog", "mydb"); Review Comment: Avoid using fully-qualified `java.util.Set` here; it can be a normal import (`import java.util.Set;`) and `Set.of(...)` like the rest of the file. This keeps the test consistent with the project’s Java style and makes the line easier to read. -- 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]
