andygrove commented on code in PR #5974: URL: https://github.com/apache/datafusion-comet/pull/5974#discussion_r4027616725
########## docs/source/contributor-guide/ci.md: ########## @@ -246,11 +246,33 @@ nightly suites, if you need a result before the next scheduled run. ## Reproducing a suite failure locally -The Spark SQL suites outside the PR tier run Spark's own test suite against Comet, with the -version's diff from `dev/diffs/` applied. See [Spark SQL Tests](spark-sql-tests.md) for how to run -one locally, and [Iceberg Spark Tests](iceberg-spark-tests.md) for the Iceberg equivalents. For the -Comet test suites that run on macOS, `make test-jvm` on a Mac runs the same suites the workflow -does; the macOS job differs from Linux only in the platform. +`dev/local-ci.sh` builds the same sandbox a runner builds and runs the Spark SQL or Iceberg +workflow: + +```sh +dev/local-ci.sh spark # everything the Spark job runs +dev/local-ci.sh spark sql_core-1 # just the shard that failed +dev/local-ci.sh iceberg # everything the Iceberg job runs +dev/local-ci.sh iceberg shard-2 +dev/local-ci.sh spark 3.5 sql_core-1 # a nightly-tier version, named explicitly +``` + +The version defaults to the one the merge queue gates on. That, the matrix rows and the shard +count are read from the workflow files and from `dev/ci/`, so a local shard runs what the CI shard +of the same name runs. It prepares first (native build, Comet install, patched Spark or Iceberg +source under `$COMET_LOCAL_CI_HOME`, default `/tmp/comet-local-ci`), then runs the tests. Only the +sbt projects the selected rows need are compiled, so asking for `catalyst` does not build the sql +or hive test classes. `SKIP_PREPARE=1` skips to the tests when the sandbox is already current. Review Comment: "When the sandbox is already current" is doing a lot of work here. `SKIP_PREPARE=1` skips `install_comet` too, so someone who just changed the serde and re-ran a shard would be testing the previously installed JAR and getting a green that means nothing. Could that be spelled out, something like "skips the Comet install too, so don't use it after changing Comet"? ########## docs/source/contributor-guide/ci.md: ########## @@ -246,11 +246,33 @@ nightly suites, if you need a result before the next scheduled run. ## Reproducing a suite failure locally -The Spark SQL suites outside the PR tier run Spark's own test suite against Comet, with the -version's diff from `dev/diffs/` applied. See [Spark SQL Tests](spark-sql-tests.md) for how to run -one locally, and [Iceberg Spark Tests](iceberg-spark-tests.md) for the Iceberg equivalents. For the -Comet test suites that run on macOS, `make test-jvm` on a Mac runs the same suites the workflow -does; the macOS job differs from Linux only in the platform. +`dev/local-ci.sh` builds the same sandbox a runner builds and runs the Spark SQL or Iceberg +workflow: + +```sh +dev/local-ci.sh spark # everything the Spark job runs +dev/local-ci.sh spark sql_core-1 # just the shard that failed +dev/local-ci.sh iceberg # everything the Iceberg job runs +dev/local-ci.sh iceberg shard-2 +dev/local-ci.sh spark 3.5 sql_core-1 # a nightly-tier version, named explicitly +``` + +The version defaults to the one the merge queue gates on. That, the matrix rows and the shard +count are read from the workflow files and from `dev/ci/`, so a local shard runs what the CI shard +of the same name runs. It prepares first (native build, Comet install, patched Spark or Iceberg +source under `$COMET_LOCAL_CI_HOME`, default `/tmp/comet-local-ci`), then runs the tests. Only the +sbt projects the selected rows need are compiled, so asking for `catalyst` does not build the sql +or hive test classes. `SKIP_PREPARE=1` skips to the tests when the sandbox is already current. + +Three caveats. The sandbox lives under `/tmp`, so a reboot or a tmp reaper means downloading and +compiling again. Preparing runs `rm -rf ~/.m2/repository/org/apache/parquet`, as the workflows do. Review Comment: This names the parquet `rm -rf` but not `purge_partial_poms`, which walks all of `~/.m2/repository` and deletes POMs. That's 8,818 POMs on my machine. It's recoverable since Maven refetches, but AGENTS.md tells agents to run this unattended, so could the caveat mention that the sweep covers the whole shared repository and not just Comet and Spark artifacts? ########## dev/local-ci.sh: ########## @@ -0,0 +1,338 @@ +#!/usr/bin/env bash +# +# 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. +# +# Run the Spark SQL or Iceberg CI workflow locally. Mirrors +# .github/workflows/spark_sql_test_reusable.yml and +# .github/workflows/iceberg_spark_test_reusable.yml. +# +# Versions, matrix rows and the shard count are read from ci.yml and dev/ci/ at +# run time, so a version bump needs no change here. Written for bash 3.2. + +set -euo pipefail + +REPO="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +CI_YML="$REPO/.github/workflows/ci.yml" +SPARK_YML="$REPO/.github/workflows/spark_sql_test_reusable.yml" +ICEBERG_YML="$REPO/.github/workflows/iceberg_spark_test_reusable.yml" +# Sandbox default is under /tmp: the trees are rebuildable, and keeping them out +# of $HOME means a stale one is never something to clean up by hand. Point +# COMET_LOCAL_CI_HOME somewhere durable to survive a reboot or a tmp reaper. +SANDBOX="${COMET_LOCAL_CI_HOME:-/tmp/comet-local-ci}" +case "$(uname -s)" in + # bsdtar reads Spark's dot-prefixed .crc test fixtures as AppleDouble + # metadata and exits nonzero after extracting them correctly. + Darwin) LIB=libcomet.dylib TAR_FLAGS=--no-mac-metadata ;; + *) LIB=libcomet.so TAR_FLAGS= ;; +esac + +say() { printf '\n\033[1;32m[local-ci] %s\033[0m\n' "$*" >&2; } +die() { + printf '\033[1;31m[local-ci] %s\033[0m\n' "$*" >&2 + exit 1 +} + +usage() { + cat >&2 <<'EOF' +Usage: dev/local-ci.sh <spark|iceberg> [version] [target...] + + dev/local-ci.sh spark every Spark SQL matrix row + dev/local-ci.sh spark sql_core-1 one row, or all/core/hive + dev/local-ci.sh iceberg every Iceberg target + dev/local-ci.sh iceberg shard-2 one shard, or extensions/runtime + +The version defaults to the one the merge queue gates on. Name an older one to +reproduce a nightly failure: dev/local-ci.sh spark 3.5 sql_core-1 + + SKIP_PREPARE=1 run the tests only, skipping the build and fetch + COMET_LOCAL_CI_HOME where the sources live (default /tmp/comet-local-ci) +EOF + exit 2 +} + +# The queue tier is the newest version Comet fully supports; everything else is +# nightly-only. POLICY in compute-changes.py is where that is decided. +default_version() { + sed -n "s/^ \"$1_\([0-9_]*\)\": \[\"queue\".*/\1/p" "$REPO/dev/ci/compute-changes.py" | + tr _ . | sort -V | tail -1 | grep . || die "no queue-tier $1 version in compute-changes.py" +} + +# A `with:` input of a ci.yml job: `input spark_4_1 spark-full` -> 4.1.3 +input() { + awk -v job="$1" -v key="$2" ' + /^ [A-Za-z0-9_-]+:[ \t]*$/ { j = $1; sub(/:$/, "", j); w = 0; next } + j == job && /^ with:[ \t]*$/ { w = 1; next } + w && /^ [a-z][a-z0-9-]*:/ { + k = $1; sub(/:$/, "", k) + if (k == key) { v = $2; gsub(/\047/, "", v); print v; exit } + } + w && /^ [a-z]/ { w = 0 } + ' "$CI_YML" | grep . || die "no '$2' input for job '$1' in ci.yml" +} + +setup_jdk() { + if [ -x /usr/libexec/java_home ]; then + JAVA_HOME="$(/usr/libexec/java_home -v "$1")" || die "JDK $1 not installed" + export JAVA_HOME + fi + [ -n "${JAVA_HOME:-}" ] || die "export JAVA_HOME pointing at a JDK $1" + say "JDK $1: $JAVA_HOME" +} + +# The ci profile is release without LTO. Stage it where -Prelease looks. +build_native() { + say "cargo build --profile ci" + (cd "$REPO/native" && cargo build --profile ci) + mkdir -p "$REPO/native/target/release" + cp "$REPO/native/target/ci/$LIB" "$REPO/native/target/release/$LIB" +} + +# Spark needs no git repository: the only thing in its build that shells out to +# git is build/spark-build-info, which has no `set -e`, so a missing .git costs +# an empty `revision=` and nothing else. Taking the tag archive instead of a +# shallow clone drops the 58 MB of git objects and leaves no second repository +# next to Comet's. Extract beside the target and move into place, so an +# interrupted download cannot leave a tree the next run mistakes for complete. +fetch_archive() { + if [ -d "$2" ]; then return 0; fi + say "downloading $1" + rm -rf "$2.part" + mkdir -p "$2.part" + # Checked rather than left to `set -e`, which bash disables inside a function + # called from a conditional. + # shellcheck disable=SC2086 + if ! curl -fsSL "$1" | tar -xz $TAR_FLAGS -C "$2.part" --strip-components=1; then + rm -rf "$2.part" + die "could not download $1" + fi + mv "$2.part" "$2" +} + +# Iceberg does need one: its build derives the project version from the latest +# apache-iceberg-* tag through com.palantir.git-version, and the no-git fallback +# wants a release-time version.txt the tag archive does not carry. +clone_tag() { + if [ -d "$3/.git" ]; then return 0; fi + say "cloning $1 at $2" + git clone --depth 1 --branch "$2" "$1" "$3" +} + +# git apply works fine outside a repository, so this covers both. +apply_diff() { Review Comment: I think editing a diff and re-running gets stuck here. First run applies it, you edit `dev/diffs/4.1.3.diff`, and the second run's reverse check fails and then `git apply` fails with "patch does not apply". Since the Spark tree has no `.git` there's nothing to reset it to, so the only way forward is deleting the tree and paying for the download and the full sbt precompile again. AGENTS.md lists `dev/diffs/` as a reason to run this, so that's the main use case landing on the worst path. Iceberg is a real clone, so `git checkout . && git clean -fd` before applying would handle that half. For Spark, could we re-extract when the reverse check fails, or at minimum print which directory to delete? ########## dev/local-ci.sh: ########## @@ -0,0 +1,338 @@ +#!/usr/bin/env bash +# +# 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. +# +# Run the Spark SQL or Iceberg CI workflow locally. Mirrors +# .github/workflows/spark_sql_test_reusable.yml and +# .github/workflows/iceberg_spark_test_reusable.yml. +# +# Versions, matrix rows and the shard count are read from ci.yml and dev/ci/ at +# run time, so a version bump needs no change here. Written for bash 3.2. + +set -euo pipefail + +REPO="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +CI_YML="$REPO/.github/workflows/ci.yml" +SPARK_YML="$REPO/.github/workflows/spark_sql_test_reusable.yml" +ICEBERG_YML="$REPO/.github/workflows/iceberg_spark_test_reusable.yml" +# Sandbox default is under /tmp: the trees are rebuildable, and keeping them out +# of $HOME means a stale one is never something to clean up by hand. Point +# COMET_LOCAL_CI_HOME somewhere durable to survive a reboot or a tmp reaper. +SANDBOX="${COMET_LOCAL_CI_HOME:-/tmp/comet-local-ci}" +case "$(uname -s)" in + # bsdtar reads Spark's dot-prefixed .crc test fixtures as AppleDouble + # metadata and exits nonzero after extracting them correctly. + Darwin) LIB=libcomet.dylib TAR_FLAGS=--no-mac-metadata ;; + *) LIB=libcomet.so TAR_FLAGS= ;; +esac + +say() { printf '\n\033[1;32m[local-ci] %s\033[0m\n' "$*" >&2; } +die() { + printf '\033[1;31m[local-ci] %s\033[0m\n' "$*" >&2 + exit 1 +} + +usage() { + cat >&2 <<'EOF' +Usage: dev/local-ci.sh <spark|iceberg> [version] [target...] + + dev/local-ci.sh spark every Spark SQL matrix row + dev/local-ci.sh spark sql_core-1 one row, or all/core/hive + dev/local-ci.sh iceberg every Iceberg target + dev/local-ci.sh iceberg shard-2 one shard, or extensions/runtime + +The version defaults to the one the merge queue gates on. Name an older one to +reproduce a nightly failure: dev/local-ci.sh spark 3.5 sql_core-1 + + SKIP_PREPARE=1 run the tests only, skipping the build and fetch + COMET_LOCAL_CI_HOME where the sources live (default /tmp/comet-local-ci) +EOF + exit 2 +} + +# The queue tier is the newest version Comet fully supports; everything else is +# nightly-only. POLICY in compute-changes.py is where that is decided. +default_version() { + sed -n "s/^ \"$1_\([0-9_]*\)\": \[\"queue\".*/\1/p" "$REPO/dev/ci/compute-changes.py" | + tr _ . | sort -V | tail -1 | grep . || die "no queue-tier $1 version in compute-changes.py" +} + +# A `with:` input of a ci.yml job: `input spark_4_1 spark-full` -> 4.1.3 +input() { + awk -v job="$1" -v key="$2" ' + /^ [A-Za-z0-9_-]+:[ \t]*$/ { j = $1; sub(/:$/, "", j); w = 0; next } + j == job && /^ with:[ \t]*$/ { w = 1; next } + w && /^ [a-z][a-z0-9-]*:/ { + k = $1; sub(/:$/, "", k) + if (k == key) { v = $2; gsub(/\047/, "", v); print v; exit } + } + w && /^ [a-z]/ { w = 0 } + ' "$CI_YML" | grep . || die "no '$2' input for job '$1' in ci.yml" +} + +setup_jdk() { + if [ -x /usr/libexec/java_home ]; then + JAVA_HOME="$(/usr/libexec/java_home -v "$1")" || die "JDK $1 not installed" + export JAVA_HOME + fi + [ -n "${JAVA_HOME:-}" ] || die "export JAVA_HOME pointing at a JDK $1" + say "JDK $1: $JAVA_HOME" +} + +# The ci profile is release without LTO. Stage it where -Prelease looks. +build_native() { + say "cargo build --profile ci" + (cd "$REPO/native" && cargo build --profile ci) + mkdir -p "$REPO/native/target/release" + cp "$REPO/native/target/ci/$LIB" "$REPO/native/target/release/$LIB" +} + +# Spark needs no git repository: the only thing in its build that shells out to +# git is build/spark-build-info, which has no `set -e`, so a missing .git costs +# an empty `revision=` and nothing else. Taking the tag archive instead of a +# shallow clone drops the 58 MB of git objects and leaves no second repository +# next to Comet's. Extract beside the target and move into place, so an +# interrupted download cannot leave a tree the next run mistakes for complete. +fetch_archive() { + if [ -d "$2" ]; then return 0; fi + say "downloading $1" + rm -rf "$2.part" + mkdir -p "$2.part" + # Checked rather than left to `set -e`, which bash disables inside a function + # called from a conditional. + # shellcheck disable=SC2086 + if ! curl -fsSL "$1" | tar -xz $TAR_FLAGS -C "$2.part" --strip-components=1; then + rm -rf "$2.part" + die "could not download $1" + fi + mv "$2.part" "$2" +} + +# Iceberg does need one: its build derives the project version from the latest +# apache-iceberg-* tag through com.palantir.git-version, and the no-git fallback +# wants a release-time version.txt the tag archive does not carry. +clone_tag() { + if [ -d "$3/.git" ]; then return 0; fi + say "cloning $1 at $2" + git clone --depth 1 --branch "$2" "$1" "$3" +} + +# git apply works fine outside a repository, so this covers both. +apply_diff() { + (cd "$1" && git apply --check --reverse "$2") 2>/dev/null && return 0 + say "applying $(basename "$2")" + (cd "$1" && git apply "$2") +} + +install_comet() { + say "mvnw install -Prelease -DskipTests $*" + (cd "$REPO" && ./mvnw -B install -Prelease -DskipTests "$@") +} + +# The install leaves Parquet POMs without their test-classifier JARs, and +# Coursier then calls the artifact found-locally rather than falling back to +# Maven Central. Both workflows drop the tree for that reason. +purge_parquet() { + dir="${MAVEN_REPO_LOCAL:-$HOME/.m2/repository}/org/apache/parquet" Review Comment: Is `MAVEN_REPO_LOCAL` something Maven actually reads? I can't find it anywhere else in the repo, and the real overrides are `<localRepository>` in `settings.xml` and `-Dmaven.repo.local`. For anyone who has relocated their repo, the `[ -d "$dir" ] || return 0` below makes both this and `purge_partial_poms` no-ops, and they then hit the exact Coursier "found locally, no JAR" failure the purge exists to prevent. `./mvnw help:evaluate -Dexpression=settings.localRepository` would give you the real path. ########## dev/local-ci.sh: ########## @@ -0,0 +1,338 @@ +#!/usr/bin/env bash +# +# 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. +# +# Run the Spark SQL or Iceberg CI workflow locally. Mirrors +# .github/workflows/spark_sql_test_reusable.yml and +# .github/workflows/iceberg_spark_test_reusable.yml. +# +# Versions, matrix rows and the shard count are read from ci.yml and dev/ci/ at +# run time, so a version bump needs no change here. Written for bash 3.2. + +set -euo pipefail + +REPO="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +CI_YML="$REPO/.github/workflows/ci.yml" +SPARK_YML="$REPO/.github/workflows/spark_sql_test_reusable.yml" +ICEBERG_YML="$REPO/.github/workflows/iceberg_spark_test_reusable.yml" +# Sandbox default is under /tmp: the trees are rebuildable, and keeping them out +# of $HOME means a stale one is never something to clean up by hand. Point +# COMET_LOCAL_CI_HOME somewhere durable to survive a reboot or a tmp reaper. +SANDBOX="${COMET_LOCAL_CI_HOME:-/tmp/comet-local-ci}" +case "$(uname -s)" in + # bsdtar reads Spark's dot-prefixed .crc test fixtures as AppleDouble + # metadata and exits nonzero after extracting them correctly. + Darwin) LIB=libcomet.dylib TAR_FLAGS=--no-mac-metadata ;; + *) LIB=libcomet.so TAR_FLAGS= ;; +esac + +say() { printf '\n\033[1;32m[local-ci] %s\033[0m\n' "$*" >&2; } +die() { + printf '\033[1;31m[local-ci] %s\033[0m\n' "$*" >&2 + exit 1 +} + +usage() { + cat >&2 <<'EOF' +Usage: dev/local-ci.sh <spark|iceberg> [version] [target...] + + dev/local-ci.sh spark every Spark SQL matrix row + dev/local-ci.sh spark sql_core-1 one row, or all/core/hive + dev/local-ci.sh iceberg every Iceberg target + dev/local-ci.sh iceberg shard-2 one shard, or extensions/runtime + +The version defaults to the one the merge queue gates on. Name an older one to +reproduce a nightly failure: dev/local-ci.sh spark 3.5 sql_core-1 + + SKIP_PREPARE=1 run the tests only, skipping the build and fetch + COMET_LOCAL_CI_HOME where the sources live (default /tmp/comet-local-ci) +EOF + exit 2 +} + +# The queue tier is the newest version Comet fully supports; everything else is +# nightly-only. POLICY in compute-changes.py is where that is decided. +default_version() { + sed -n "s/^ \"$1_\([0-9_]*\)\": \[\"queue\".*/\1/p" "$REPO/dev/ci/compute-changes.py" | + tr _ . | sort -V | tail -1 | grep . || die "no queue-tier $1 version in compute-changes.py" +} + +# A `with:` input of a ci.yml job: `input spark_4_1 spark-full` -> 4.1.3 +input() { + awk -v job="$1" -v key="$2" ' + /^ [A-Za-z0-9_-]+:[ \t]*$/ { j = $1; sub(/:$/, "", j); w = 0; next } + j == job && /^ with:[ \t]*$/ { w = 1; next } + w && /^ [a-z][a-z0-9-]*:/ { + k = $1; sub(/:$/, "", k) + if (k == key) { v = $2; gsub(/\047/, "", v); print v; exit } + } + w && /^ [a-z]/ { w = 0 } + ' "$CI_YML" | grep . || die "no '$2' input for job '$1' in ci.yml" +} + +setup_jdk() { + if [ -x /usr/libexec/java_home ]; then + JAVA_HOME="$(/usr/libexec/java_home -v "$1")" || die "JDK $1 not installed" + export JAVA_HOME + fi + [ -n "${JAVA_HOME:-}" ] || die "export JAVA_HOME pointing at a JDK $1" + say "JDK $1: $JAVA_HOME" +} + +# The ci profile is release without LTO. Stage it where -Prelease looks. +build_native() { + say "cargo build --profile ci" + (cd "$REPO/native" && cargo build --profile ci) + mkdir -p "$REPO/native/target/release" + cp "$REPO/native/target/ci/$LIB" "$REPO/native/target/release/$LIB" +} + +# Spark needs no git repository: the only thing in its build that shells out to +# git is build/spark-build-info, which has no `set -e`, so a missing .git costs +# an empty `revision=` and nothing else. Taking the tag archive instead of a +# shallow clone drops the 58 MB of git objects and leaves no second repository +# next to Comet's. Extract beside the target and move into place, so an +# interrupted download cannot leave a tree the next run mistakes for complete. +fetch_archive() { + if [ -d "$2" ]; then return 0; fi + say "downloading $1" + rm -rf "$2.part" + mkdir -p "$2.part" + # Checked rather than left to `set -e`, which bash disables inside a function + # called from a conditional. + # shellcheck disable=SC2086 + if ! curl -fsSL "$1" | tar -xz $TAR_FLAGS -C "$2.part" --strip-components=1; then + rm -rf "$2.part" + die "could not download $1" + fi + mv "$2.part" "$2" +} + +# Iceberg does need one: its build derives the project version from the latest +# apache-iceberg-* tag through com.palantir.git-version, and the no-git fallback +# wants a release-time version.txt the tag archive does not carry. +clone_tag() { + if [ -d "$3/.git" ]; then return 0; fi + say "cloning $1 at $2" + git clone --depth 1 --branch "$2" "$1" "$3" +} + +# git apply works fine outside a repository, so this covers both. +apply_diff() { + (cd "$1" && git apply --check --reverse "$2") 2>/dev/null && return 0 + say "applying $(basename "$2")" + (cd "$1" && git apply "$2") +} + +install_comet() { + say "mvnw install -Prelease -DskipTests $*" + (cd "$REPO" && ./mvnw -B install -Prelease -DskipTests "$@") +} + +# The install leaves Parquet POMs without their test-classifier JARs, and +# Coursier then calls the artifact found-locally rather than falling back to +# Maven Central. Both workflows drop the tree for that reason. +purge_parquet() { + dir="${MAVEN_REPO_LOCAL:-$HOME/.m2/repository}/org/apache/parquet" + [ -d "$dir" ] || return 0 + say "removing $dir so sbt and gradle refetch it" + rm -rf "$dir" +} + +# The same problem one level out. Resolving Comet's dependency graph downloads +# POMs for transitive artifacts whose JARs it never needs, and sbt then dies on +# "not found: .../protobuf-java-4.28.2.jar" for something it can see a POM for. +# Only entries whose packaging is jar or bundle are partial; a POM-only parent +# or BOM is left alone. This is the sweep setup-spark-builder runs. +purge_partial_poms() { + repo="${MAVEN_REPO_LOCAL:-$HOME/.m2/repository}" + [ -d "$repo" ] || return 0 + say "dropping Maven entries that have a POM but no JAR" + find "$repo" -name '*.pom' | while read -r pom; do + [ -f "${pom%.pom}.jar" ] && continue + grep -q '<packaging>jar</packaging>\|<packaging>bundle</packaging>' "$pom" 2>/dev/null || continue + rm -f "$pom" "$pom.sha1" "${pom%.pom}.pom.lastUpdated" \ + "$(dirname "$pom")/_remote.repositories" + done +} + +# Matrix rows from the definition the workflow builds its matrix from. Unit +# separated, since a tab is IFS whitespace and `read` would collapse the empty +# args1 of an sql_core row. +spark_rows() { + python3 - "$REPO/dev/ci/spark-sql-modules.py" "$@" <<'PY' +import json, subprocess, sys +path, want = sys.argv[1], sys.argv[2:] or ["all"] +rows = json.loads(subprocess.check_output([sys.executable, path, "--modules", "all"]))["module"] +names = [r["name"] for r in rows] +picked = [] +for w in want: + if w in ("all", "core", "hive"): + picked += [r for r in rows if w == "all" or r["group"] == w] + elif w in names: + picked += [r for r in rows if r["name"] == w] + else: + sys.exit("unknown module %r; try: %s" % (w, ", ".join(names + ["all", "core", "hive"]))) +for r in picked: + print("\x1f".join([r["name"], r["args1"], r["args2"], r["heap"], r["metaspace"]])) +PY +} + +run_spark() { + short="$1" + shift + full="$(input "spark_${short//./_}" spark-full)" + java="$(input "spark_${short//./_}" java)" + dest="$SANDBOX/apache-spark-$full" + rows="$(spark_rows "$@")" + setup_jdk "$java" + + if [ -z "${SKIP_PREPARE:-}" ]; then + build_native + fetch_archive "https://github.com/apache/spark/archive/refs/tags/v$full.tar.gz"; "$dest" + apply_diff "$dest" "$REPO/dev/diffs/$full.diff" + install_comet "-Pspark-$short" + purge_parquet + purge_partial_poms + # Only the sbt projects the selected rows actually use. CI compiles all + # three because one artifact feeds seven shards, but there is no artifact + # here, and `catalyst` alone does not need sql or hive test classes. + projects="$(printf '%s\n' "$rows" | awk -F'\037' '{ print $2 $3 }' | sed 's|/.*||' | sort -u)" + compile="" + for p in $projects; do compile="$compile $p/Test/compile"; done + say "pre-compiling test classes:$compile" + # shellcheck disable=SC2086 + (cd "$dest" && NOLINT_ON_COMPILE=true build/sbt -Dsbt.log.noformat=true -mem 3072 $compile) + fi + + # The workflow process-isolates a few suites on one Spark version only. + gated="$(sed -n "s/.*DEDICATED_JVM_SBT_TESTS: .*spark-short == '\([^']*\)' && '\([^']*\)'.*/\1 \2/p" "$SPARK_YML")" + case "$gated" in "$short "*) export DEDICATED_JVM_SBT_TESTS="${gated#* }" ;; esac + + while IFS=$'\037' read -r name args1 args2 heap metaspace; do + [ -n "$name" ] || continue + say "spark-sql-$name / spark-$full-jdk$java" + # A subshell per row, so one row's HEAP_SIZE cannot leak into the next. + ( + cd "$dest" + printf -- '-J-Xms1g\n-J-Xmx4g\n-J-XX:MaxMetaspaceSize=1g\n' > .sbtopts + export LC_ALL=C.UTF-8 SERIAL_SBT_TESTS=1 NOLINT_ON_COMPILE=true + # shellcheck disable=SC2030 + export ENABLE_COMET=true ENABLE_COMET_ONHEAP=true + export SBT_OPTS="-Xss4m -XX:+UseG1GC -XX:+UseStringDeduplication -XX:MaxMetaspaceSize=384m -XX:G1HeapRegionSize=2m -XX:InitiatingHeapOccupancyPercent=35 -XX:+ParallelRefProcEnabled -XX:+ExitOnOutOfMemoryError" + [ -n "$heap" ] && export HEAP_SIZE="$heap" + [ -n "$metaspace" ] && export METASPACE_SIZE="$metaspace" + set -- -Dsbt.log.noformat=true -mem 1024 \ + "set Global / concurrentRestrictions := Seq(Tags.limit(Tags.ForkedTestGroup, 1))" + [ -n "$args1" ] && set -- "$@" "$args1" + [ -n "$args2" ] && set -- "$@" "$args2" + build/sbt "$@" + ) + done <<< "$rows" +} + +run_iceberg() { + short="$1" + shift + job="iceberg_${short//./_}" + full="$(input "$job" iceberg-full)" + spark="$(input "$job" spark-short)" + java="$(input "$job" java)" + # ci.yml leaves `scala` unset, so the reusable workflow default applies. + scala="$(awk '/^ scala:/ { f = 1 } f && /^ default:/ { gsub(/[^0-9.]/, "", $2); print $2; exit }' "$ICEBERG_YML")" + shards="$(sed -n 's/^SHARD_COUNT = //p' "$REPO/dev/ci/check-iceberg-shards.py")" Review Comment: Could `shards` get the same `| grep . || die` guard that `default_version` and `input` have? I tried it with the parse returning empty and the failure mode is worse than I expected. `while [ "$i" -le "$shards" ]` prints one stderr line, falls through, and the target list ends up as just `extensions runtime`, so the script prints `done` and exits 0 having skipped every sharded core test. A false green is the one outcome worth ruling out for a tool people will use to decide whether something is safe to queue. `scala` on the line above has the same gap and would quietly give you `iceberg-spark-4.1_:test`, and `gated` at line 224 would drop the 4.0 process isolation without saying so. ########## dev/local-ci.sh: ########## @@ -0,0 +1,338 @@ +#!/usr/bin/env bash +# +# 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. +# +# Run the Spark SQL or Iceberg CI workflow locally. Mirrors +# .github/workflows/spark_sql_test_reusable.yml and +# .github/workflows/iceberg_spark_test_reusable.yml. +# +# Versions, matrix rows and the shard count are read from ci.yml and dev/ci/ at Review Comment: This adds six new readers of CI config, and `check-ci-config.py` opens by saying it guards six invariants that are silent when broken, with five more drift-guards beside it in preflight. Requoting `spark-full: '4.1.3'` or reindenting a `with:` block breaks this script, and we don't find out until someone actually needs it, which is the worst possible moment. I agree a new CI job isn't warranted. Would a `--print-config` mode plus an assertion inside one of the existing preflight checks work? That's seconds of CI time. Worth noting too that actionlint runs with `--shellcheck=off` and only covers workflow files, so nothing lints this script either. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
