Copilot commented on code in PR #9493:
URL: https://github.com/apache/gravitino/pull/9493#discussion_r2638414349
##########
docs/trino-connector/configuration.md:
##########
@@ -15,6 +15,7 @@ license: "This software is licensed under the Apache License
version 2."
| gravitino.metadata.refresh-interval-seconds | integer | 10
| The `gravitino.metadata.refresh-interval-seconds` defines the interval in
seconds to refresh metadata from Gravitino server, the default value is 10
seconds.
| No | 0.9.0 |
| gravitino.trino.skip-version-validation | boolean | false
| The `gravitino.trino.skip-version-validation` defines whether skip Trino
version validation or not. Note that Gravitino only supports Trino which
version between 435 and 439, other versions of Trino have not undergone
thorough testing, so there may be compatiablity problem if true.
| No | 1.0.0 |
| gravitino.client. | string | (none)
| The configuration key prefix for the Gravitino client config.
| No
| 1.0.0 |
+| gravitino.trino.skip-catalog-patterns | string | (none)
| The `gravitino.trino.skip-catalog-patterns` defines a comma-separated list
of catalog name regex patterns that should be excluded from loading.
| No
| 1.2.0 |
Review Comment:
The documentation should clarify that the patterns use Java regex syntax and
provide an example showing common use cases such as prefix matching, suffix
matching, or exact matches. For instance, add examples like: "Skip all catalogs
starting with 'test_': test_.*" or "Skip specific catalogs: catalog1,catalog2".
```suggestion
| gravitino.trino.skip-catalog-patterns | string | (none)
| The `gravitino.trino.skip-catalog-patterns` defines a comma-separated list
of catalog name patterns (using Java regular expression syntax) for catalogs
that should be excluded from loading. For example: skip all catalogs starting
with `test_`: `test_.*`; skip catalogs ending with `_tmp`: `.*_tmp`; skip
specific catalogs: `catalog1,catalog2`. | No | 1.2.0 |
```
##########
trino-connector/trino-connector/src/test/java/org/apache/gravitino/trino/connector/TestGravitinoConnectorWithSkipCatalog.java:
##########
@@ -0,0 +1,115 @@
+/*
+ * 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.trino.connector;
+
+import static io.trino.testing.TestingSession.testSessionBuilder;
+import static org.apache.gravitino.Catalog.Type.RELATIONAL;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import io.trino.Session;
+import io.trino.plugin.memory.MemoryPlugin;
+import io.trino.testing.AbstractTestQueryFramework;
+import io.trino.testing.DistributedQueryRunner;
+import io.trino.testing.MaterializedResult;
+import io.trino.testing.QueryRunner;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import org.apache.gravitino.client.GravitinoAdminClient;
+import org.apache.gravitino.client.GravitinoMetalake;
+import org.apache.gravitino.trino.connector.catalog.CatalogConnectorManager;
+import org.awaitility.Awaitility;
+import org.junit.jupiter.api.Test;
+
+public class TestGravitinoConnectorWithSkipCatalog extends
AbstractTestQueryFramework {
+
+ GravitinoMockServer server;
+
+ @Override
+ protected QueryRunner createQueryRunner() throws Exception {
+ GravitinoAdminClient gravitinoClient = initGravitinoMockServer();
+ Session session = testSessionBuilder().setCatalog("gravitino").build();
+
+ try {
+ DistributedQueryRunner queryRunner =
+ DistributedQueryRunner.builder(session).setNodeCount(1).build();
+
+ TestGravitinoPlugin gravitinoPlugin = new
TestGravitinoPlugin(gravitinoClient);
+ queryRunner.installPlugin(gravitinoPlugin);
+
+ {
+ // create a gravitino connector with single metalake
+ HashMap<String, String> properties = new HashMap<>();
+ properties.put("gravitino.metalake", "test1");
+ properties.put("gravitino.uri", "http://127.0.0.1:8090";);
+ properties.put("gravitino.trino.skip-catalog-patterns", "a.*, b1");
+ properties.put(
+ "catalog.config-dir",
queryRunner.getCoordinator().getBaseDataDir().toString());
+ properties.put("discovery.uri",
queryRunner.getCoordinator().getBaseUrl().toString());
+ queryRunner.createCatalog("gravitino", "gravitino", properties);
+ }
+
+
GravitinoConnectorPluginManager.instance(this.getClass().getClassLoader())
+ .installPlugin("memory", new MemoryPlugin());
+ CatalogConnectorManager catalogConnectorManager =
+ gravitinoPlugin.getCatalogConnectorManager();
+ server.setCatalogConnectorManager(catalogConnectorManager);
+
+ // Wait for the catalog to be created. Wait for at least 30 seconds.
+ Awaitility.await()
+ .atMost(30, TimeUnit.SECONDS)
+ .pollInterval(1, TimeUnit.SECONDS)
+ .until(() -> !catalogConnectorManager.getCatalogs().isEmpty());
+ return queryRunner;
+
+ } catch (Exception e) {
+ throw new RuntimeException("Create query runner failed", e);
+ }
+ }
+
+ @Test
+ public void showCatalogs() throws Exception {
+ MaterializedResult expectedResult = computeActual("show catalogs");
+ assertEquals(expectedResult.getMaterializedRows().size(), 3);
+ List<String> catalogs =
+ expectedResult.getMaterializedRows().stream()
+ .map(row -> (String) row.getField(0))
+ .collect(Collectors.toList());
+ assertFalse(catalogs.contains("a1"));
+ assertFalse(catalogs.contains("b1"));
+ assertTrue(catalogs.contains("b2"));
+ }
+
+ private GravitinoAdminClient initGravitinoMockServer() {
+ GravitinoMockServer gravitinoMockServer = new GravitinoMockServer();
+ server = closeAfterClass(gravitinoMockServer);
+ GravitinoAdminClient gravitinoClient = server.createGravitinoClient();
+
+ gravitinoClient.createMetalake("test1", "", Collections.EMPTY_MAP);
+ GravitinoMetalake metalake = gravitinoClient.loadMetalake("test1");
+ metalake.createCatalog("a1", RELATIONAL, "", "", Collections.EMPTY_MAP);
+ metalake.createCatalog("b1", RELATIONAL, "", "", Collections.EMPTY_MAP);
+ metalake.createCatalog("b2", RELATIONAL, "", "", Collections.EMPTY_MAP);
Review Comment:
Using the deprecated Collections.EMPTY_MAP constant. Replace with
Collections.emptyMap() which is the recommended approach since Java 5.
```suggestion
gravitinoClient.createMetalake("test1", "", Collections.emptyMap());
GravitinoMetalake metalake = gravitinoClient.loadMetalake("test1");
metalake.createCatalog("a1", RELATIONAL, "", "", Collections.emptyMap());
metalake.createCatalog("b1", RELATIONAL, "", "", Collections.emptyMap());
metalake.createCatalog("b2", RELATIONAL, "", "", Collections.emptyMap());
```
##########
trino-connector/trino-connector/src/test/java/org/apache/gravitino/trino/connector/TestGravitinoConnectorWithSkipCatalog.java:
##########
@@ -0,0 +1,115 @@
+/*
+ * 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.trino.connector;
+
+import static io.trino.testing.TestingSession.testSessionBuilder;
+import static org.apache.gravitino.Catalog.Type.RELATIONAL;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import io.trino.Session;
+import io.trino.plugin.memory.MemoryPlugin;
+import io.trino.testing.AbstractTestQueryFramework;
+import io.trino.testing.DistributedQueryRunner;
+import io.trino.testing.MaterializedResult;
+import io.trino.testing.QueryRunner;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import org.apache.gravitino.client.GravitinoAdminClient;
+import org.apache.gravitino.client.GravitinoMetalake;
+import org.apache.gravitino.trino.connector.catalog.CatalogConnectorManager;
+import org.awaitility.Awaitility;
+import org.junit.jupiter.api.Test;
+
+public class TestGravitinoConnectorWithSkipCatalog extends
AbstractTestQueryFramework {
+
+ GravitinoMockServer server;
+
+ @Override
+ protected QueryRunner createQueryRunner() throws Exception {
+ GravitinoAdminClient gravitinoClient = initGravitinoMockServer();
+ Session session = testSessionBuilder().setCatalog("gravitino").build();
+
+ try {
+ DistributedQueryRunner queryRunner =
+ DistributedQueryRunner.builder(session).setNodeCount(1).build();
+
+ TestGravitinoPlugin gravitinoPlugin = new
TestGravitinoPlugin(gravitinoClient);
+ queryRunner.installPlugin(gravitinoPlugin);
+
+ {
+ // create a gravitino connector with single metalake
+ HashMap<String, String> properties = new HashMap<>();
+ properties.put("gravitino.metalake", "test1");
+ properties.put("gravitino.uri", "http://127.0.0.1:8090";);
+ properties.put("gravitino.trino.skip-catalog-patterns", "a.*, b1");
+ properties.put(
+ "catalog.config-dir",
queryRunner.getCoordinator().getBaseDataDir().toString());
+ properties.put("discovery.uri",
queryRunner.getCoordinator().getBaseUrl().toString());
+ queryRunner.createCatalog("gravitino", "gravitino", properties);
+ }
+
+
GravitinoConnectorPluginManager.instance(this.getClass().getClassLoader())
+ .installPlugin("memory", new MemoryPlugin());
+ CatalogConnectorManager catalogConnectorManager =
+ gravitinoPlugin.getCatalogConnectorManager();
+ server.setCatalogConnectorManager(catalogConnectorManager);
+
+ // Wait for the catalog to be created. Wait for at least 30 seconds.
+ Awaitility.await()
+ .atMost(30, TimeUnit.SECONDS)
+ .pollInterval(1, TimeUnit.SECONDS)
+ .until(() -> !catalogConnectorManager.getCatalogs().isEmpty());
+ return queryRunner;
+
+ } catch (Exception e) {
+ throw new RuntimeException("Create query runner failed", e);
+ }
+ }
+
+ @Test
+ public void showCatalogs() throws Exception {
+ MaterializedResult expectedResult = computeActual("show catalogs");
+ assertEquals(expectedResult.getMaterializedRows().size(), 3);
+ List<String> catalogs =
+ expectedResult.getMaterializedRows().stream()
+ .map(row -> (String) row.getField(0))
+ .collect(Collectors.toList());
+ assertFalse(catalogs.contains("a1"));
+ assertFalse(catalogs.contains("b1"));
+ assertTrue(catalogs.contains("b2"));
+ }
Review Comment:
The test method name 'showCatalogs' doesn't clearly describe what is being
tested. Consider renaming to something more descriptive like
'testSkipCatalogPatternsExcludeMatchingCatalogs' or
'testCatalogsFilteredBySkipPatterns' to better indicate that this test verifies
the skip-catalog-patterns functionality.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConfig.java:
##########
@@ -329,6 +340,40 @@ public Boolean isSkipTrinoVersionValidation() {
GRAVITINO_TRINO_SKIP_VERSION_VALIDATION.defaultValue));
}
+ /**
+ * Whether skip loading catalog or not
+ *
+ * @param catalogName catalog name
+ * @return whether skip loading catalog or not
+ */
+ public Boolean skipCatalog(String catalogName) {
+ for (Pattern pattern : skipCatalogPatternList) {
+ if (pattern.matcher(catalogName).matches()) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * Retrieves a comma-separated list of catalog name regex patterns that
should be excluded from
+ * loading
+ *
+ * @return a list of catalog name regex patterns
+ */
+ private List<Pattern> getSkipCatalogPatterns() {
+ String skipCatalogConfig =
+ config.getOrDefault(
+ GRAVITINO_TRINO_SKIP_CATALOG_PATTERNS.key,
+ GRAVITINO_TRINO_SKIP_CATALOG_PATTERNS.defaultValue);
+ return Splitter.on(',')
+ .trimResults()
+ .omitEmptyStrings()
+ .splitToStream(skipCatalogConfig)
+ .map(Pattern::compile)
+ .collect(Collectors.toList());
+ }
Review Comment:
The Pattern.compile() call on line 373 can throw PatternSyntaxException if
the user provides an invalid regex pattern. This would cause the connector to
fail initialization with an unclear error. Consider wrapping the pattern
compilation in a try-catch block and throwing a TrinoException with a clear
error message indicating which pattern is invalid, or validate patterns during
configuration loading.
##########
trino-connector/trino-connector/src/test/java/org/apache/gravitino/trino/connector/TestGravitinoConfig.java:
##########
@@ -112,4 +113,30 @@ public void testGravitinoConfigWithClientConfig() {
assertEquals(clientConfig.get("gravitino.client.socketTimeoutMs"),
"10000");
assertEquals(clientConfig.get("gravitino.client.connectionTimeoutMs"),
"20000");
}
+
+ @Test
+ public void testGravitinoConfigWithSkipCatalogPatterns() {
+ String gravitinoUrl = "http://127.0.0.1:8000";;
+ String metalake = "user_001";
+ ImmutableMap<String, String> configMap =
+ ImmutableMap.of("gravitino.uri", gravitinoUrl, "gravitino.metalake",
metalake);
+ GravitinoConfig config = new GravitinoConfig(configMap);
+
+ assertFalse(config.skipCatalog("test_catalog"));
+
+ ImmutableMap<String, String> configMapWithSkipCatalogList =
+ ImmutableMap.of(
+ "gravitino.uri",
+ gravitinoUrl,
+ "gravitino.metalake",
+ metalake,
+ "gravitino.trino.skip-catalog-patterns",
+ "test_.*, test1\\.c.*");
+ GravitinoConfig configWithSkipCatalogPatterns =
+ new GravitinoConfig(configMapWithSkipCatalogList);
+ assertTrue(configWithSkipCatalogPatterns.skipCatalog("test_catalog"));
+ assertTrue(configWithSkipCatalogPatterns.skipCatalog("test1.catalog"));
+ assertFalse(configWithSkipCatalogPatterns.skipCatalog("test1_catalog"));
+ assertFalse(configWithSkipCatalogPatterns.skipCatalog("test2_catalog"));
+ }
Review Comment:
Missing test coverage for invalid regex patterns in skip-catalog-patterns
configuration. Consider adding a test case that verifies the behavior when an
invalid regex pattern is provided, such as an unclosed bracket or other syntax
errors. This will ensure users receive helpful error messages when they
misconfigure this setting.
##########
trino-connector/trino-connector/src/test/java/org/apache/gravitino/trino/connector/TestGravitinoConnectorWithSkipCatalog.java:
##########
@@ -0,0 +1,115 @@
+/*
+ * 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.trino.connector;
+
+import static io.trino.testing.TestingSession.testSessionBuilder;
+import static org.apache.gravitino.Catalog.Type.RELATIONAL;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import io.trino.Session;
+import io.trino.plugin.memory.MemoryPlugin;
+import io.trino.testing.AbstractTestQueryFramework;
+import io.trino.testing.DistributedQueryRunner;
+import io.trino.testing.MaterializedResult;
+import io.trino.testing.QueryRunner;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import org.apache.gravitino.client.GravitinoAdminClient;
+import org.apache.gravitino.client.GravitinoMetalake;
+import org.apache.gravitino.trino.connector.catalog.CatalogConnectorManager;
+import org.awaitility.Awaitility;
+import org.junit.jupiter.api.Test;
+
+public class TestGravitinoConnectorWithSkipCatalog extends
AbstractTestQueryFramework {
+
+ GravitinoMockServer server;
+
+ @Override
+ protected QueryRunner createQueryRunner() throws Exception {
+ GravitinoAdminClient gravitinoClient = initGravitinoMockServer();
+ Session session = testSessionBuilder().setCatalog("gravitino").build();
+
+ try {
+ DistributedQueryRunner queryRunner =
+ DistributedQueryRunner.builder(session).setNodeCount(1).build();
+
+ TestGravitinoPlugin gravitinoPlugin = new
TestGravitinoPlugin(gravitinoClient);
+ queryRunner.installPlugin(gravitinoPlugin);
+
+ {
+ // create a gravitino connector with single metalake
+ HashMap<String, String> properties = new HashMap<>();
+ properties.put("gravitino.metalake", "test1");
+ properties.put("gravitino.uri", "http://127.0.0.1:8090";);
+ properties.put("gravitino.trino.skip-catalog-patterns", "a.*, b1");
+ properties.put(
+ "catalog.config-dir",
queryRunner.getCoordinator().getBaseDataDir().toString());
+ properties.put("discovery.uri",
queryRunner.getCoordinator().getBaseUrl().toString());
+ queryRunner.createCatalog("gravitino", "gravitino", properties);
+ }
+
+
GravitinoConnectorPluginManager.instance(this.getClass().getClassLoader())
+ .installPlugin("memory", new MemoryPlugin());
+ CatalogConnectorManager catalogConnectorManager =
+ gravitinoPlugin.getCatalogConnectorManager();
+ server.setCatalogConnectorManager(catalogConnectorManager);
+
+ // Wait for the catalog to be created. Wait for at least 30 seconds.
+ Awaitility.await()
+ .atMost(30, TimeUnit.SECONDS)
+ .pollInterval(1, TimeUnit.SECONDS)
+ .until(() -> !catalogConnectorManager.getCatalogs().isEmpty());
+ return queryRunner;
+
+ } catch (Exception e) {
+ throw new RuntimeException("Create query runner failed", e);
+ }
+ }
+
+ @Test
+ public void showCatalogs() throws Exception {
+ MaterializedResult expectedResult = computeActual("show catalogs");
+ assertEquals(expectedResult.getMaterializedRows().size(), 3);
+ List<String> catalogs =
+ expectedResult.getMaterializedRows().stream()
+ .map(row -> (String) row.getField(0))
+ .collect(Collectors.toList());
+ assertFalse(catalogs.contains("a1"));
+ assertFalse(catalogs.contains("b1"));
+ assertTrue(catalogs.contains("b2"));
+ }
+
+ private GravitinoAdminClient initGravitinoMockServer() {
+ GravitinoMockServer gravitinoMockServer = new GravitinoMockServer();
+ server = closeAfterClass(gravitinoMockServer);
+ GravitinoAdminClient gravitinoClient = server.createGravitinoClient();
+
+ gravitinoClient.createMetalake("test1", "", Collections.EMPTY_MAP);
+ GravitinoMetalake metalake = gravitinoClient.loadMetalake("test1");
+ metalake.createCatalog("a1", RELATIONAL, "", "", Collections.EMPTY_MAP);
+ metalake.createCatalog("b1", RELATIONAL, "", "", Collections.EMPTY_MAP);
+ metalake.createCatalog("b2", RELATIONAL, "", "", Collections.EMPTY_MAP);
Review Comment:
Using the deprecated Collections.EMPTY_MAP constant. Replace with
Collections.emptyMap() which is the recommended approach since Java 5.
```suggestion
gravitinoClient.createMetalake("test1", "", Collections.emptyMap());
GravitinoMetalake metalake = gravitinoClient.loadMetalake("test1");
metalake.createCatalog("a1", RELATIONAL, "", "", Collections.emptyMap());
metalake.createCatalog("b1", RELATIONAL, "", "", Collections.emptyMap());
metalake.createCatalog("b2", RELATIONAL, "", "", Collections.emptyMap());
```
--
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]
