Copilot commented on code in PR #10505:
URL: https://github.com/apache/gravitino/pull/10505#discussion_r2972684581
##########
trino-connector/integration-test/trino-test-tools/trino_integration_test.sh:
##########
@@ -26,6 +26,84 @@ 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
Review Comment:
`apply_version_patches` uses `[[ $version -le $max_ver ]]` which expects an
integer. If `--trino_version` is missing or non-numeric, bash will emit an
“integer expression expected” error and patch selection becomes unreliable.
Please validate `trino_version` is a non-negative integer before calling
`apply_version_patches` and fail with a clear message if not.
##########
trino-connector/integration-test/trino-test-tools/trino_integration_test.sh:
##########
@@ -26,6 +26,84 @@ 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"
Review Comment:
`--trino_version` is only recognized in the `--trino_version=<n>` form. If
someone invokes this script as `--trino_version 452` (space-separated), the
value will be forwarded to Gradle/Java but `trino_version` here stays empty, so
version patches won’t be applied and tests may run with incompatible resources.
Consider handling the two-arg form as well (e.g., detect `--trino_version` and
consume the next argument).
```suggestion
while [ "$#" -gt 0 ]; do
case "$1" in
--auto_patch)
auto_patch=true
shift
;;
--trino_version=*)
trino_version="${1#*=}"
args="$args $1"
shift
;;
--trino_version)
shift
if [ "$#" -eq 0 ]; then
echo "ERROR: --trino_version requires a version argument."
>&2
exit 1
fi
trino_version="$1"
args="$args --trino_version=$trino_version"
shift
;;
*)
args="$args $1"
shift
```
##########
trino-connector/integration-test/trino-test-tools/trino_integration_test.sh:
##########
@@ -26,6 +26,84 @@ 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/docker-script/
+ git -C "$GRAVITINO_ROOT_DIR" clean -fd \
+ trino-connector/integration-test/src/test/resources/ \
+ integration-test-common/docker-script/
+}
+
+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"
+ INTEGRATION_TEST_COMMON_REL="integration-test-common/docker-script"
+ for dir in "$TESTSETS_REL" "$INTEGRATION_TEST_COMMON_REL"; do
+ if ! git -C "$GRAVITINO_ROOT_DIR" diff --quiet -- "$dir" || \
+ ! git -C "$GRAVITINO_ROOT_DIR" diff --cached --quiet -- "$dir"; then
+ echo "ERROR: Uncommitted changes detected in $dir."
+ echo "Please commit or stash your changes before running with
--auto_patch."
+ exit 1
+ fi
Review Comment:
The pre-check for local changes only uses `git diff` / `git diff --cached`,
which won’t detect untracked files. Since `restore_test_files` runs `git clean
-fd` on these directories, any pre-existing untracked files there could be
deleted. Please also check for untracked files (e.g., `git status --porcelain`
or `git ls-files --others --exclude-standard`) and abort when present to avoid
accidental data loss.
##########
trino-connector/integration-test/trino-test-tools/trino_integration_test.sh:
##########
@@ -26,6 +26,84 @@ 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/docker-script/
+ git -C "$GRAVITINO_ROOT_DIR" clean -fd \
+ trino-connector/integration-test/src/test/resources/ \
+ integration-test-common/docker-script/
+}
+
+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"
+ INTEGRATION_TEST_COMMON_REL="integration-test-common/docker-script"
+ for dir in "$TESTSETS_REL" "$INTEGRATION_TEST_COMMON_REL"; do
+ if ! git -C "$GRAVITINO_ROOT_DIR" diff --quiet -- "$dir" || \
+ ! git -C "$GRAVITINO_ROOT_DIR" diff --cached --quiet -- "$dir"; then
+ echo "ERROR: Uncommitted changes detected in $dir."
+ echo "Please commit or stash your changes before running with
--auto_patch."
+ exit 1
+ fi
+ done
+ # Register trap to ensure test files are always restored on exit,
+ # even if the script is interrupted or exits abnormally.
+ if [ -n "$trino_version" ]; then
+ trap 'restore_test_files' EXIT
+ apply_version_patches "$trino_version"
+ fi
Review Comment:
`--auto_patch` assumes this is a git working tree and that `git` is
available. Without a `.git` directory (e.g., source tarball checkout) or
without git installed, `git apply/checkout/clean` will fail with unclear
errors. Consider adding an explicit guard (e.g., `command -v git` and `git
rev-parse --is-inside-work-tree`) and a clearer error message when
`--auto_patch` is requested.
--
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]
