Copilot commented on code in PR #10188: URL: https://github.com/apache/gravitino/pull/10188#discussion_r2882192416
########## trino-connector/trino-connector-473-478/src/main/java/org/apache/gravitino/trino/connector/GravitinoPlugin478.java: ########## @@ -0,0 +1,38 @@ +/* + * 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 org.apache.gravitino.client.GravitinoAdminClient; + +/** Trino plugin endpoint, using java spi mechanism */ +public class GravitinoPlugin478 extends GravitinoPlugin { + + public GravitinoPlugin478() { + super(); + } + + public GravitinoPlugin478(GravitinoAdminClient client) { + super(client); + } + + @Override + protected GravitinoConnectorFactory createConnectorFactory(GravitinoAdminClient client) { + return new GravitinoConnectorFactory478(client); + } Review Comment: The class/module naming is inconsistent with the established pattern in other version-segment modules (e.g., `GravitinoPlugin469` for `469-472`, `GravitinoPlugin452` for `452-468`): the plugin/factory/connector types are suffixed with `478` even though the module declares support starting at 473. Either rename these types to use the minimum supported version (473), or adjust the supported version range if this code truly requires Trino 478 SPI. ########## trino-connector/trino-connector-473-478/src/main/resources/META-INF/services/io.trino.spi.Plugin: ########## @@ -0,0 +1,19 @@ +# +# 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. +# +org.apache.gravitino.trino.connector.GravitinoPlugin478 Review Comment: If the plugin class is renamed to align with the module’s min supported version, remember to update this SPI service entry as well. As-is, the service file hard-codes `GravitinoPlugin478`, which couples the artifact to the current naming choice. ########## docs/trino-connector/requirements.md: ########## @@ -7,7 +7,7 @@ license: "This software is licensed under the Apache License version 2." To install and deploy the Apache Gravitino Trino connector, The following environmental setup is necessary: -- Trino server version should be between Trino-server-435 and Trino-server-472. +- Trino server version should be between Trino-server-435 and Trino-server-478. The examples in this document use Trino `469` by default. Review Comment: This line still says the docs use Trino 469 by default, but the integration-test Docker Compose in this PR now defaults to Trino 478. Update the documentation to match the new default (or revert the default image tag change if 469 should remain the default example). ```suggestion The examples in this document use Trino `478` by default. ``` ########## integration-test-common/docker-script/docker-compose.yaml: ########## @@ -94,7 +94,7 @@ services: entrypoint: /bin/bash /tmp/trino/init.sh volumes: - ./init/trino:/tmp/trino - - ${GRAVITINO_TRINO_CONNECTOR_DIR:-../../trino-connector/trino-connector-469-472/build/libs}:/usr/lib/trino/plugin/gravitino + - ${GRAVITINO_TRINO_CONNECTOR_DIR:-../../trino-connector/trino-connector-473-478/build/libs}:/usr/lib/trino/plugin/gravitino extra_hosts: Review Comment: Changing the default Trino image from `469` to `478` affects local dev/CI resource requirements and also conflicts with `docs/trino-connector/requirements.md` still stating examples use 469 by default. If 478 is the new default, consider documenting it (and any new minimum host memory requirements) alongside this change. ########## trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/trino-469-478.patch: ########## @@ -0,0 +1,384 @@ +diff --git a/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/hive/catalog_hive_prepare.sql b/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/hive/catalog_hive_prepare.sql +index 67ea924ba..b392ae230 100644 +--- a/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/hive/catalog_hive_prepare.sql ++++ b/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/hive/catalog_hive_prepare.sql +@@ -2,7 +2,7 @@ call gravitino.system.create_catalog( + 'gt_hive', + 'hive', + map( +- array['metastore.uris', 'trino.bypass.fs.hadoop.enabled'], +- array['${hive_uri}', 'true'] ++ array['metastore.uris'], ++ array['${hive_uri}'] + ) + ); +\ No newline at end of file +diff --git a/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/jdbc-postgresql/00004_query_pushdown.txt b/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/jdbc-postgresql/00004_query_pushdown.txt +index edd8257ad..c4f271add 100644 +--- a/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/jdbc-postgresql/00004_query_pushdown.txt ++++ b/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/jdbc-postgresql/00004_query_pushdown.txt +@@ -12,26 +12,26 @@ INSERT: 15000 rows + + "Trino version: % + % +- └─ TableScan[table = gt_postgresql:gt_db1.customer->gt_db1.customer gt_db.gt_db1.customer limit=10 columns=[custkey:bigint:int8]] ++ %TableScan[table = gt_postgresql:gt_db1.customer->gt_db1.customer gt_db1.customer limit=10 columns=[custkey:bigint:int8]] + Layout: [custkey:bigint] + % + " + + "Trino version: % + % +- └─ TableScan[table = gt_postgresql:gt_db1.customer->gt_db1.customer gt_db.gt_db1.customer%constraints=%phone%LIKE%] ++ %TableScan[table = gt_postgresql:gt_db1.customer->gt_db1.customer gt_db1.customer%constraints=%phone%LIKE%] + % + " + + "Trino version: % + % +- └─ TableScan[table = gt_postgresql:gt_db1.orders->Query[SELECT sum(""totalprice"") AS ""_pfgnrtd_0"" FROM ""gt_db"".""gt_db1"".""orders""]%columns=[_pfgnrtd_0:decimal(38,2):decimal]] ++ %TableScan[table = gt_postgresql:gt_db1.orders->Query[SELECT sum(""totalprice"") AS ""_pfgnrtd_0"" FROM ""gt_db1"".""orders""]%columns=[_pfgnrtd_0:decimal(38,2):decimal]] + % + " + + "Trino version: % + % +- └─ TableScan[table = gt_postgresql:gt_db1.%->Query[SELECT ""orderdate"", sum(""totalprice"") AS ""_pfgnrtd_0"" FROM ""gt_db"".""gt_db1"".""orders"" GROUP BY ""orderdate""]%sortOrder=[orderdate:date:date ASC NULLS LAST]%limit=10%columns=[orderdate:date:date, _pfgnrtd_0:decimal(38,2):decimal]] ++ %TableScan[table = gt_postgresql:gt_db1.%->Query[SELECT ""orderdate"", sum(""totalprice"") AS ""_pfgnrtd_0"" FROM ""gt_db1"".""orders"" GROUP BY ""orderdate""]%sortOrder=[orderdate:date:date ASC NULLS LAST]%limit=10%columns=[orderdate:date:date, _pfgnrtd_0:decimal(38,2):decimal]] + % + " + +diff --git a/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/lakehouse-iceberg/catalog_iceberg_prepare.sql b/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/lakehouse-iceberg/catalog_iceberg_prepare.sql +index e310c051d..46ad6a38c 100644 +--- a/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/lakehouse-iceberg/catalog_iceberg_prepare.sql ++++ b/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/lakehouse-iceberg/catalog_iceberg_prepare.sql +@@ -2,8 +2,8 @@ call gravitino.system.create_catalog( + 'gt_iceberg', + 'lakehouse-iceberg', + map( +- array['uri', 'catalog-backend', 'warehouse', 'trino.bypass.fs.hadoop.enabled'], +- array['${hive_uri}', 'hive', '${hdfs_uri}/user/iceberg/warehouse/TrinoQueryIT', 'true'] ++ array['uri', 'catalog-backend', 'warehouse'], ++ array['${hive_uri}', 'hive', '${hdfs_uri}/user/iceberg/warehouse/TrinoQueryIT'] + ) + ); + +diff --git a/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/tpcds/00002.sql b/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/tpcds/00002.sql +index 7ed8fbf18..0e797b639 100644 +--- a/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/tpcds/00002.sql ++++ b/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/tpcds/00002.sql +@@ -36,13 +36,13 @@ UNION ALL ( + ) + SELECT + "d_week_seq1" +-, CAST(("sun_sales1" / "sun_sales2") as DECIMAL(38,2)) +-, CAST(("mon_sales1" / "mon_sales2") as DECIMAL(38,2)) +-, CAST(("tue_sales1" / "tue_sales2") as DECIMAL(38,2)) +-, CAST(("wed_sales1" / "wed_sales2") as DECIMAL(38,2)) +-, CAST(("thu_sales1" / "thu_sales2") as DECIMAL(38,2)) +-, CAST(("fri_sales1" / "fri_sales2") as DECIMAL(38,2)) +-, CAST(("sat_sales1" / "sat_sales2") as DECIMAL(38,2)) ++, "round"(("sun_sales1" / "sun_sales2"), 2) ++, "round"(("mon_sales1" / "mon_sales2"), 2) ++, "round"(("tue_sales1" / "tue_sales2"), 2) ++, "round"(("wed_sales1" / "wed_sales2"), 2) ++, "round"(("thu_sales1" / "thu_sales2"), 2) ++, "round"(("fri_sales1" / "fri_sales2"), 2) ++, "round"(("sat_sales1" / "sat_sales2"), 2) Review Comment: This patch file’s hunks for several testset SQLs/plans are the opposite of the direct updates made to those same files in this PR (e.g., here it changes `CAST(... AS DECIMAL(38,2))` to `round(..., 2)`, while `testsets/tpcds/00002.sql` in this PR changes `round` to `CAST`). If these patch files are meant to be applied to switch expected outputs per Trino version, this inconsistency will make the patch fail to apply cleanly or produce the wrong baseline. Please reconcile the patch contents with the actual canonical testset files (or drop the conflicting hunks if this patch isn’t used). ```suggestion -, "round"(("mon_sales1" / "mon_sales2"), 2) -, "round"(("tue_sales1" / "tue_sales2"), 2) -, "round"(("wed_sales1" / "wed_sales2"), 2) -, "round"(("thu_sales1" / "thu_sales2"), 2) -, "round"(("fri_sales1" / "fri_sales2"), 2) -, "round"(("sat_sales1" / "sat_sales2"), 2) +, CAST(("sun_sales1" / "sun_sales2") as DECIMAL(38,2)) +, CAST(("mon_sales1" / "mon_sales2") as DECIMAL(38,2)) +, CAST(("tue_sales1" / "tue_sales2") as DECIMAL(38,2)) +, CAST(("wed_sales1" / "wed_sales2") as DECIMAL(38,2)) +, CAST(("thu_sales1" / "thu_sales2") as DECIMAL(38,2)) +, CAST(("fri_sales1" / "fri_sales2") as DECIMAL(38,2)) +, CAST(("sat_sales1" / "sat_sales2") as DECIMAL(38,2)) ``` ########## trino-connector/trino-connector-473-478/build.gradle.kts: ########## @@ -0,0 +1,168 @@ +/* + * 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. + */ + +import com.diffplug.gradle.spotless.SpotlessExtension +import net.ltgt.gradle.errorprone.errorprone +import org.gradle.internal.hash.ChecksumService +import org.gradle.kotlin.dsl.support.serviceOf + +plugins { + `java-library` + `maven-publish` +} + +// This module supports Trino versions 473-478 +val minTrinoVersion = 473 +val maxTrinoVersion = 478 +val otelSemconvVersion = "1.32.0" + +val trinoVersion = providers.gradleProperty("trinoVersion") + .map { it.trim().toInt() } + .orElse(maxTrinoVersion) Review Comment: `trinoVersion` defaults to `maxTrinoVersion` here, unlike other Trino version-segment modules that default to the minimum supported version. Defaulting to the max increases the risk of accidentally compiling against SPI APIs added after 473, which could lead to `NoSuchMethodError` when the same artifact is used with Trino 473-477. Consider defaulting to `minTrinoVersion` (and only overriding via `-PtrinoVersion=...` when needed). ```suggestion .orElse(minTrinoVersion) ``` ########## integration-test-common/docker-script/init/trino/config/jvm.config: ########## @@ -16,22 +16,15 @@ # specific language governing permissions and limitations # under the License. # --XX:InitialRAMPercentage=30 --XX:MaxRAMPercentage=60 +-server +-Xms3G +-Xmx3G +-XX:+UseG1GC -XX:G1HeapRegionSize=32M +-XX:MaxGCPauseMillis=500 + Review Comment: Switching from percentage-based heap sizing to a fixed `-Xms3G/-Xmx3G` makes the Trino containers much less flexible and can cause startup failures/OOM on smaller CI runners or dev machines. If the goal is stability for Trino 478, consider keeping the percentage-based settings and only pinning heap size via an env var/override for CI, or document the required container memory explicitly. ########## trino-connector/trino-connector-473-478/src/test/java/org/apache/gravitino/trino/connector/TestGravitinoConnector478.java: ########## @@ -0,0 +1,58 @@ +/* + * 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; Review Comment: In the existing version-segment modules, the `TestGravitinoConnectorXXX` entrypoint tests are in the default package (e.g., `trino-connector-469-472/src/test/java/TestGravitinoConnector469.java`). This new test is placed under `org.apache...` and declares a package, which is inconsistent and can make test discovery/navigation uneven across modules. Consider following the same layout (default package + top-level `src/test/java/TestGravitinoConnector478.java`) for consistency. -- 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]
