Copilot commented on code in PR #12830:
URL: https://github.com/apache/gravitino/pull/12830#discussion_r3910053095
##########
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryTestTool.java:
##########
@@ -99,10 +99,7 @@ public static void main(String[] args) throws Exception {
+ "otherwise fall back to a single-node setup with combined
coordinator-worker roles.");
options.addOption(
- "trino_version",
- true,
- "Specify the Trino version to test. If not specified, the version
pinned in the test "
- + "docker-compose file is used.");
+ "trino_version", true, "Specify the Trino version to test, the
default value is 435.");
Review Comment:
The `--trino_version` CLI option help text says the default is 435, but the
code sets `TrinoQueryIT.trinoVersion` to null when not provided (see
`extractIntValue`), meaning the docker-compose pinned version is used. Update
the help text to match actual behavior.
##########
docs/how-to-test.md:
##########
@@ -70,6 +70,8 @@ To deploy the Gravitino server locally to run the integration
tests, follow thes
Specify the `--trino_worker_num` parameter to make the Trino test sets run
in a distributed environment.
Specify the `--trino_version` parameter to make the Trino test sets run
with specific trino version.
Specify the `--trino_connector_dir` parameter to run Trino tests using the
JAR files located in the `gravitino-trino-connector` plugin directory.
+ Alternatively, use `bash
trino-connector/integration-test/trino-test-tools/run_test_with_versions.sh`
command to execute
+ tests across different Trino versions.
Review Comment:
These newly added doc lines have trailing whitespace, which can cause
unnecessary diffs and can fail markdown linting in some setups.
##########
trino-connector/trino-connector-435-439/src/main/java/org/apache/gravitino/trino/connector/GravitinoMetadata435.java:
##########
@@ -0,0 +1,120 @@
+/*
+ * 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 io.airlift.slice.Slice;
+import io.trino.spi.connector.ColumnMetadata;
+import io.trino.spi.connector.ConnectorInsertTableHandle;
+import io.trino.spi.connector.ConnectorMergeTableHandle;
+import io.trino.spi.connector.ConnectorOutputMetadata;
+import io.trino.spi.connector.ConnectorOutputTableHandle;
+import io.trino.spi.connector.ConnectorSession;
+import io.trino.spi.connector.ConnectorTableExecuteHandle;
+import io.trino.spi.connector.ConnectorTableHandle;
+import io.trino.spi.connector.RetryMode;
+import io.trino.spi.connector.SchemaTableName;
+import io.trino.spi.statistics.ComputedStatistics;
+import java.util.Collection;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.gravitino.trino.connector.catalog.CatalogConnectorMetadata;
+import
org.apache.gravitino.trino.connector.catalog.CatalogConnectorMetadataAdapter;
+import org.apache.gravitino.trino.connector.metadata.GravitinoColumn;
+
+public class GravitinoMetadata435 extends GravitinoMetadata {
+
+ public GravitinoMetadata435(
+ CatalogConnectorMetadata catalogConnectorMetadata,
+ CatalogConnectorMetadataAdapter metadataAdapter,
+ io.trino.spi.connector.ConnectorMetadata internalMetadata) {
Review Comment:
Avoid using a fully-qualified class name in the constructor parameter type;
this codebase requires normal imports unless there is a name collision. Add an
import for `ConnectorMetadata` and use the simple name here.
##########
trino-connector/integration-test/trino-test-tools/run_test_with_versions.sh:
##########
@@ -0,0 +1,92 @@
+#!/bin/bash
+#
+# 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.
+
+# This script is used to execute tests for the Trino connector across
different Trino versions.
+# It wraps the `trino_test.sh` script and iterates through a configured map of
Trino versions
Review Comment:
This script header says it wraps `trino_test.sh`, but this directory
contains `trino_integration_test.sh` and the script actually invokes
`trino_integration_test.sh` below. Fixing the comment avoids misleading users.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConfig.java:
##########
@@ -231,7 +231,7 @@ public class GravitinoConfig {
private static final ConfigEntry GRAVITINO_TRINO_SKIP_VERSION_VALIDATION =
new ConfigEntry(
"gravitino.trino.skip-version-validation",
- "When true, skips Trino version validation and logs a warning
instead of throwing an error. Gravitino supports Trino versions 440-478; other
versions are untested.",
+ "When true, skips Trino version validation and logs a warning
instead of throwing an error. Gravitino supports Trino versions 435-439; other
versions are untested.",
Review Comment:
The description for `gravitino.trino.skip-version-validation` claims
Gravitino supports Trino versions 435-439, but the project includes connector
modules up to 473-478 and the docs state support through 478; this message
should reflect the full supported range to avoid confusing users.
--
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]
