Copilot commented on code in PR #10455:
URL: https://github.com/apache/gravitino/pull/10455#discussion_r2945887672
##########
trino-connector/integration-test/trino-test-tools/trino_integration_test.sh:
##########
@@ -26,6 +26,68 @@ 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
+ if [[ $version -le 473 ]]; then
+ echo "Applying patch: trino-478-473.patch"
+ git -C "$GRAVITINO_ROOT_DIR" apply "$TESTSETS_DIR/trino-478-473.patch"
+ fi
+ if [[ $version -le 452 ]]; then
+ echo "Applying patch: trino-473-452.patch"
+ git -C "$GRAVITINO_ROOT_DIR" apply "$TESTSETS_DIR/trino-473-452.patch"
+ fi
+ if [[ $version -le 446 ]]; then
+ echo "Applying patch: trino-452-446.patch"
+ git -C "$GRAVITINO_ROOT_DIR" apply "$TESTSETS_DIR/trino-452-446.patch"
+ fi
+}
+
+# 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
+ # 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
+fi
Review Comment:
When `--auto_patch` is set, the EXIT trap always runs `git checkout`/`git
clean` on broad paths, which can discard a developer’s local changes under
`trino-connector/integration-test/src/test/resources/` and
`integration-test-common/` even if no patches were applied (e.g.,
missing/invalid `--trino_version`). Please guard this behavior (e.g., require a
valid numeric `--trino_version`, verify the repo is clean before patching,
and/or restore only if patches were successfully applied).
##########
trino-connector/integration-test/trino-test-tools/trino_integration_test.sh:
##########
@@ -26,6 +26,68 @@ 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
+ if [[ $version -le 473 ]]; then
+ echo "Applying patch: trino-478-473.patch"
+ git -C "$GRAVITINO_ROOT_DIR" apply "$TESTSETS_DIR/trino-478-473.patch"
+ fi
+ if [[ $version -le 452 ]]; then
+ echo "Applying patch: trino-473-452.patch"
+ git -C "$GRAVITINO_ROOT_DIR" apply "$TESTSETS_DIR/trino-473-452.patch"
+ fi
+ if [[ $version -le 446 ]]; then
+ echo "Applying patch: trino-452-446.patch"
+ git -C "$GRAVITINO_ROOT_DIR" apply "$TESTSETS_DIR/trino-452-446.patch"
+ fi
Review Comment:
`git apply` failures are not checked and the script does not use a fail-fast
mode, so a patch that applies partially/incorrectly (or is already applied) can
still lead to running the Gradle tests against an unintended working tree
state. Consider enabling `set -euo pipefail` (or explicitly checking each `git
apply` exit status) and aborting immediately on patch-application errors.
##########
trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/trino-473-452.patch:
##########
@@ -0,0 +1,181 @@
+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;
++
++USE gt_iceberg.gt_db2;
++
++DROP SCHEMA gt_iceberg.gt_db2;
+\ No newline at end of file
Review Comment:
This patch creates `lakehouse-iceberg/catalog_iceberg_prepare.sql` without a
trailing newline (`\ No newline at end of file`). Consider adding a final
newline in the patched file content to avoid repeated diff noise and tooling
issues on some platforms/editors.
##########
trino-connector/integration-test/trino-test-tools/trino_integration_test.sh:
##########
@@ -26,6 +26,68 @@ 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
Review Comment:
`case $arg in` uses an unquoted variable, which can trigger pathname
expansion if an argument contains glob characters (e.g., `*`, `?`, `[]`).
Quoting the variable in the `case` statement (and ideally building/forwarding
args as an array rather than a concatenated string) avoids surprising behavior.
--
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]
