Copilot commented on code in PR #10455:
URL: https://github.com/apache/gravitino/pull/10455#discussion_r2958521351
##########
trino-connector/integration-test/trino-test-tools/trino_integration_test.sh:
##########
@@ -26,6 +26,80 @@ export HADOOP_USER_NAME=anonymous
echo $GRAVITINO_ROOT_DIR
cd $GRAVITINO_ROOT_DIR
-args="\"$@\""
+# Parse --auto_patch and --trino_version from arguments.
+# --auto_patch is consumed here and not forwarded to Gradle.
+# --trino_version is forwarded to Gradle and also used for patch selection.
+auto_patch=false
+trino_version=""
+args=""
+for arg in "$@"; do
+ case "$arg" in
+ --auto_patch)
+ auto_patch=true
+ ;;
+ --trino_version=*)
+ trino_version="${arg#*=}"
+ args="$args $arg"
+ ;;
+ *)
+ args="$args $arg"
+ ;;
+ esac
+done
+args="${args# }"
-./gradlew :trino-connector:integration-test:TrinoTest -PappArgs="$args"
+TESTSETS_DIR="$GRAVITINO_ROOT_DIR/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets"
+
+# Apply version-specific patches cumulatively based on the target Trino
version.
+# Patches are applied in descending order (newest first):
+# version <= 473: apply trino-478-473.patch
+# version <= 452: also apply trino-473-452.patch
+# version <= 446: also apply trino-452-446.patch
+apply_version_patches() {
+ local version=$1
+ # Each entry is "max_version:patch_file"; patches are applied in order.
+ local patches=(
+ "473:trino-478-473.patch"
+ "452:trino-473-452.patch"
+ "446:trino-452-446.patch"
+ )
+ for entry in "${patches[@]}"; do
+ local max_ver="${entry%%:*}"
+ local patch="${entry##*:}"
+ if [[ $version -le $max_ver ]]; then
+ echo "Applying patch: $patch"
+ git -C "$GRAVITINO_ROOT_DIR" apply "$TESTSETS_DIR/$patch" \
+ || { echo "ERROR: Failed to apply $patch"; exit 1; }
+ fi
+ done
+}
+
+# Restore test resources to their original state by reverting any patch
changes.
+restore_test_files() {
+ echo "Restoring test files..."
+ git -C "$GRAVITINO_ROOT_DIR" checkout -- \
+ trino-connector/integration-test/src/test/resources/ \
+ integration-test-common/
+ git -C "$GRAVITINO_ROOT_DIR" clean -f \
+
trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/lakehouse-iceberg/
+}
+
+if [ "$auto_patch" = true ]; then
+ # Abort if the testsets directory has uncommitted changes, to avoid
+ # patch conflicts or accidental loss of in-progress work.
+
TESTSETS_REL="trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets"
+ if ! git -C "$GRAVITINO_ROOT_DIR" diff --quiet -- "$TESTSETS_REL" || \
+ ! git -C "$GRAVITINO_ROOT_DIR" diff --cached --quiet --
"$TESTSETS_REL"; then
+ echo "ERROR: Uncommitted changes detected in $TESTSETS_REL."
+ echo "Please commit or stash your changes before running with
--auto_patch."
+ exit 1
+ fi
+ # Register trap to ensure test files are always restored on exit,
+ # even if the script is interrupted or exits abnormally.
+ trap 'restore_test_files' EXIT
+ if [ -n "$trino_version" ]; then
+ apply_version_patches "$trino_version"
+ fi
Review Comment:
When `--auto_patch` is set but `--trino_version` is omitted, no patches are
applied even though the Java test tool defaults to Trino version 435. This
makes it easy to accidentally run older-version tests with unpatched (latest)
resources. Consider failing fast when `--auto_patch=true` and `trino_version`
is empty (or alternatively infer the version from args/default and apply
patches accordingly).
##########
trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/trino-473-452.patch:
##########
@@ -0,0 +1,180 @@
+diff --git
a/integration-test-common/docker-script/init/trino/config/jvm.config
b/integration-test-common/docker-script/init/trino/config/jvm.config
+index 77a930a36..e1089ee4b 100644
+--- a/integration-test-common/docker-script/init/trino/config/jvm.config
++++ b/integration-test-common/docker-script/init/trino/config/jvm.config
+@@ -26,5 +26,8 @@
+ -XX:+ExplicitGCInvokesConcurrent
+ -XX:+HeapDumpOnOutOfMemoryError
+
++-XX:+UnlockDiagnosticVMOptions
++-XX:G1NumCollectionsKeepPinned=10000000
++
+ -DHADOOP_USER_NAME=hive
+ -Dlog4j.configurationFile=/etc/trino/log4j2.properties
+diff --git
a/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/lakehouse-iceberg/00011_table_statistics.sql
b/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/lakehouse-iceberg/00011_table_statistics.sql
+new file mode 100644
+index 000000000..a3154a68e
+--- /dev/null
++++
b/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/lakehouse-iceberg/00011_table_statistics.sql
+@@ -0,0 +1,14 @@
++CREATE SCHEMA gt_iceberg_test_table_statistics;
++
++CREATE TABLE gt_iceberg_test_table_statistics.tb01 (
++ key1 int,
++ value1 int
++);
++
++INSERT INTO gt_iceberg_test_table_statistics.tb01(key1, value1) VALUES (1,
1), (2, 2);
++
++SHOW STATS FOR gt_iceberg_test_table_statistics.tb01;
++
++DROP TABLE gt_iceberg_test_table_statistics.tb01;
++
++DROP SCHEMA gt_iceberg_test_table_statistics;
+diff --git
a/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/lakehouse-iceberg/00011_table_statistics.txt
b/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/lakehouse-iceberg/00011_table_statistics.txt
+new file mode 100644
+index 000000000..ce9f3101b
+--- /dev/null
++++
b/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/lakehouse-iceberg/00011_table_statistics.txt
+@@ -0,0 +1,13 @@
++CREATE SCHEMA
++
++CREATE TABLE
++
++INSERT: 2 rows
++
++"key1","","","0.0","","1","2"
++"value1","","","0.0","","1","2"
++"","","","","2.0","",""
++
++DROP TABLE
++
++DROP SCHEMA
+diff --git
a/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/lakehouse-iceberg/catalog_iceberg_cleanup.sql
b/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/lakehouse-iceberg/catalog_iceberg_cleanup.sql
+new file mode 100644
+index 000000000..051d2a503
+--- /dev/null
++++
b/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/lakehouse-iceberg/catalog_iceberg_cleanup.sql
+@@ -0,0 +1 @@
++CALL gravitino.system.drop_catalog('gt_iceberg');
+diff --git
a/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/lakehouse-iceberg/catalog_iceberg_mysql_cleanup.sql
b/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/lakehouse-iceberg/catalog_iceberg_mysql_cleanup.sql
+new file mode 100644
+index 000000000..19d744acd
+--- /dev/null
++++
b/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/lakehouse-iceberg/catalog_iceberg_mysql_cleanup.sql
+@@ -0,0 +1 @@
++CALL gravitino.system.drop_catalog('gt_iceberg_mysql');
+diff --git
a/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/lakehouse-iceberg/catalog_iceberg_mysql_prepare.sql
b/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/lakehouse-iceberg/catalog_iceberg_mysql_prepare.sql
+new file mode 100644
+index 000000000..607be5081
+--- /dev/null
++++
b/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/lakehouse-iceberg/catalog_iceberg_mysql_prepare.sql
+@@ -0,0 +1,17 @@
++call gravitino.system.create_catalog(
++ 'gt_iceberg_mysql',
++ 'lakehouse-iceberg',
++ map(
++ array['uri', 'catalog-backend', 'warehouse', 'jdbc-user',
'jdbc-password', 'jdbc-driver'],
++
array['${mysql_uri}/iceberg_db?createDatabaseIfNotExist=true&useSSL=false',
'jdbc',
++ '${hdfs_uri}/user/iceberg/warehouse/TrinoQueryIT', 'trino',
'ds123', 'com.mysql.cj.jdbc.Driver']
++ )
++);
++
++show catalogs;
++
++CREATE SCHEMA gt_iceberg_mysql.gt_db2;
++
++USE gt_iceberg_mysql.gt_db2;
++
++DROP SCHEMA gt_iceberg_mysql.gt_db2;
+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
+new file mode 100644
+index 000000000..46ad6a38c
+--- /dev/null
++++
b/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/lakehouse-iceberg/catalog_iceberg_prepare.sql
+@@ -0,0 +1,16 @@
++call gravitino.system.create_catalog(
++ 'gt_iceberg',
++ 'lakehouse-iceberg',
++ map(
++ array['uri', 'catalog-backend', 'warehouse'],
++ array['${hive_uri}', 'hive',
'${hdfs_uri}/user/iceberg/warehouse/TrinoQueryIT']
++ )
++);
++
++show catalogs;
++
++CREATE SCHEMA gt_iceberg.gt_db2;
++
Review Comment:
This patch adds `catalog_iceberg_prepare.sql`/`catalog_iceberg_cleanup.sql`,
but the base testset already has
`catalog_iceberg_hive_prepare.sql`/`catalog_iceberg_hive_cleanup.sql` that
create/drop the same `gt_iceberg` catalog. For `--auto=all`, TrinoQueryIT will
treat these as separate catalogs (`iceberg` and `iceberg_hive`) and run the
same lakehouse-iceberg testset twice, increasing runtime and complexity.
Consider avoiding duplicate catalog variants here (e.g., rename/move the
existing files for <=452 instead of adding another pair, or delete one variant
in this patch).
##########
trino-connector/integration-test/trino-test-tools/trino_integration_test.sh:
##########
@@ -26,6 +26,80 @@ export HADOOP_USER_NAME=anonymous
echo $GRAVITINO_ROOT_DIR
cd $GRAVITINO_ROOT_DIR
-args="\"$@\""
+# Parse --auto_patch and --trino_version from arguments.
+# --auto_patch is consumed here and not forwarded to Gradle.
+# --trino_version is forwarded to Gradle and also used for patch selection.
+auto_patch=false
+trino_version=""
+args=""
+for arg in "$@"; do
+ case "$arg" in
+ --auto_patch)
+ auto_patch=true
+ ;;
+ --trino_version=*)
+ trino_version="${arg#*=}"
+ args="$args $arg"
+ ;;
+ *)
+ args="$args $arg"
+ ;;
+ esac
+done
+args="${args# }"
-./gradlew :trino-connector:integration-test:TrinoTest -PappArgs="$args"
+TESTSETS_DIR="$GRAVITINO_ROOT_DIR/trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets"
+
+# Apply version-specific patches cumulatively based on the target Trino
version.
+# Patches are applied in descending order (newest first):
+# version <= 473: apply trino-478-473.patch
+# version <= 452: also apply trino-473-452.patch
+# version <= 446: also apply trino-452-446.patch
+apply_version_patches() {
+ local version=$1
+ # Each entry is "max_version:patch_file"; patches are applied in order.
+ local patches=(
+ "473:trino-478-473.patch"
+ "452:trino-473-452.patch"
+ "446:trino-452-446.patch"
+ )
+ for entry in "${patches[@]}"; do
+ local max_ver="${entry%%:*}"
+ local patch="${entry##*:}"
+ if [[ $version -le $max_ver ]]; then
+ echo "Applying patch: $patch"
+ git -C "$GRAVITINO_ROOT_DIR" apply "$TESTSETS_DIR/$patch" \
+ || { echo "ERROR: Failed to apply $patch"; exit 1; }
+ fi
+ done
+}
+
+# Restore test resources to their original state by reverting any patch
changes.
+restore_test_files() {
+ echo "Restoring test files..."
+ git -C "$GRAVITINO_ROOT_DIR" checkout -- \
+ trino-connector/integration-test/src/test/resources/ \
+ integration-test-common/
+ git -C "$GRAVITINO_ROOT_DIR" clean -f \
+
trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/lakehouse-iceberg/
+}
Review Comment:
`--auto_patch` checks for uncommitted changes only under the testsets
directory, but the patches also modify files under `integration-test-common/`
(e.g., `docker-script/init/trino/config/jvm.config`). With the current
`restore_test_files()` doing a broad `git checkout` of
`integration-test-common/`, running with local uncommitted changes there can
silently discard work. Consider either (a) expanding the pre-flight dirty-check
to include every path that will be modified/restored (at least
`integration-test-common/`), and/or (b) narrowing the restore to only the
specific files/dirs that patches touch.
--
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]
