[Cherry-pick to branch-1.2] [#10454] feat(trino-connector): Add multi-version integration test automation (#10455)#10505
Conversation
…est automation (apache#10455) ### What changes were proposed in this pull request? - Add three incremental patch files (`trino-478-473.patch`, `trino-473-452.patch`, `trino-452-446.patch`) under `trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/` to adapt test resources for older Trino versions - Enhance `trino_integration_test.sh` with `--auto_patch` and `--trino_version` parameters: auto-applies cumulative patches before tests and restores files via `trap EXIT` - Add `.github/workflows/trino-multi-version-test.yml`: manually-triggered workflow with 7 explicit version steps (435, 440, 446, 452, 469, 473, 478) - Remove obsolete patch files (`trino-435-445.patch`, `trino-446-451.patch`, `trino-469-478.patch`) ### Why are the changes needed? The Gravitino Trino connector supports 6 connector modules covering Trino 435–478. Without automated multi-version testing, compatibility regressions across version boundaries go undetected. This PR enables one-click validation of all supported Trino versions. Fix: apache#10454 ### Does this PR introduce _any_ user-facing change? No. All changes are in CI tooling and test infrastructure. ### How was this patch tested? Each Trino version (478, 473, 452, 446) was manually validated locally using `trino_integration_test.sh --auto=all --auto_patch --trino_version=<ver>` with Docker-based test environments. All tests passed.
There was a problem hiding this comment.
Pull request overview
Cherry-picks the Trino connector multi-version integration test automation into branch-1.2, adding CI support for running the same integration test suite across multiple Trino versions by applying version-specific resource patches on demand.
Changes:
- Extend
trino_integration_test.shto support--auto_patchand apply cumulative version-specific patches based on--trino_version. - Add/adjust version patchsets (
trino-478-473.patch,trino-473-452.patch,trino-452-446.patch) to keep test resources compatible across Trino versions. - Introduce a new manual GitHub Actions workflow to run Trino integration tests across multiple Trino versions; also speed up the existing Trino IT workflow build step by excluding unrelated modules.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
trino-connector/integration-test/trino-test-tools/trino_integration_test.sh |
Adds arg parsing and optional auto-patching + restoration logic for multi-version test runs. |
trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/trino-478-473.patch |
Patchset for compatibility adjustments needed for Trino ≤ 473. |
trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/trino-473-452.patch |
Patchset for compatibility adjustments needed for Trino ≤ 452 (incl. docker jvm.config and testset moves). |
trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/trino-452-446.patch |
Patchset for compatibility adjustments needed for Trino ≤ 446. |
.github/workflows/trino-multi-version-test.yml |
Adds a workflow_dispatch job to run ITs for multiple Trino versions in one job. |
.github/workflows/trino-integration-test.yml |
Optimizes build step by excluding unrelated module builds to reduce CI time/disk usage. |
| 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 |
There was a problem hiding this 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.
| 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 |
There was a problem hiding this 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.
| for arg in "$@"; do | ||
| case "$arg" in | ||
| --auto_patch) | ||
| auto_patch=true | ||
| ;; | ||
| --trino_version=*) | ||
| trino_version="${arg#*=}" | ||
| args="$args $arg" | ||
| ;; | ||
| *) | ||
| args="$args $arg" |
There was a problem hiding this 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).
| for arg in "$@"; do | |
| case "$arg" in | |
| --auto_patch) | |
| auto_patch=true | |
| ;; | |
| --trino_version=*) | |
| trino_version="${arg#*=}" | |
| args="$args $arg" | |
| ;; | |
| *) | |
| args="$args $arg" | |
| 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 |
| 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 |
There was a problem hiding this 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.
Cherry-pick Information:
branch-1.2