Copilot commented on code in PR #12388: URL: https://github.com/apache/gluten/pull/12388#discussion_r3660828919
########## .github/workflows/util/delta-spark-ut/setup-delta.sh: ########## @@ -0,0 +1,238 @@ +#!/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. + +# +# Prepares a delta-io/delta clone for running its `spark` module tests with the +# Gluten (Velox) bundle jar on the classpath. +# +# Usage: +# setup-delta.sh <delta_ref> <delta_dir> <gluten_bundle_jar> <gluten_repo_root> +# +# Arguments: +# delta_ref - git ref (tag/branch/sha) to check out (e.g. v4.2.0) +# delta_dir - destination directory for the Delta clone +# gluten_bundle_jar - path to the gluten-velox-bundle fat jar +# gluten_repo_root - path to the Gluten repository root (used to locate +# backends-velox/src-delta40/.../DeltaSQLCommandTest.scala) +# + +set -euo pipefail + +if [ "$#" -ne 4 ]; then + echo "Usage: $0 <delta_ref> <delta_dir> <gluten_bundle_jar> <gluten_repo_root>" >&2 + exit 1 +fi + +DELTA_REF="$1" +DELTA_DIR="$2" +GLUTEN_BUNDLE_JAR="$3" +GLUTEN_ROOT="$4" + +if [ ! -f "$GLUTEN_BUNDLE_JAR" ]; then + echo "Gluten bundle jar not found: $GLUTEN_BUNDLE_JAR" >&2 + exit 1 +fi + +# Reuse the existing DeltaSQLCommandTest from Gluten's backends-velox module +# rather than maintaining a separate copy. This file is compiled as part of the +# unified `spark` project's Test scope, which has the Gluten bundle on its +# classpath (via spark-unified/lib/), so the typed GlutenConfig / VeloxDeltaConfig +# imports resolve correctly. +PATCH_SOURCE="$GLUTEN_ROOT/backends-velox/src-delta40/test/scala/org/apache/spark/sql/delta/test/DeltaSQLCommandTest.scala" +if [ ! -f "$PATCH_SOURCE" ]; then + echo "Gluten DeltaSQLCommandTest not found: $PATCH_SOURCE" >&2 + exit 1 +fi + +echo "::group::Cloning delta-io/delta @ ${DELTA_REF}" +# init + shallow fetch resolves a tag, branch OR commit SHA in a single path +# (`git clone --branch` rejects SHAs). Avoids a full-clone fallback and the +# destructive `rm -rf "$DELTA_DIR"` it required. `--` terminates options so a +# DELTA_REF starting with `-` can't be misread as a git flag (this script is +# workflow_dispatch-runnable with a user-supplied ref). +# +# Every step here is idempotent so a local re-run (or a CI re-run on a runner +# that kept the workspace) resumes instead of dying: `git init` re-initializes +# an existing repo harmlessly, but `remote add` errors out when `origin` already +# exists, so drop it first; and `checkout -f` discards leftovers from a previous +# partial run. Nothing worth keeping exists here yet -- the bundle jar and the +# source patches below are applied *after* this block. +git init -q "$DELTA_DIR" +git -C "$DELTA_DIR" remote remove origin 2>/dev/null || true +git -C "$DELTA_DIR" remote add origin https://github.com/delta-io/delta.git +git -C "$DELTA_DIR" fetch -q --depth 1 origin -- "$DELTA_REF" +git -C "$DELTA_DIR" checkout -qf FETCH_HEAD +git -C "$DELTA_DIR" --no-pager log -1 --oneline +echo "::endgroup::" + +echo "::group::Injecting Gluten bundle jar onto the spark project's TEST classpath" +# The Gluten bundle jar must be on the spark project's TEST runtime classpath +# (so DeltaSQLCommandTest can load org.apache.gluten.GlutenPlugin by name) but +# NOT on the COMPILE classpath of `sparkV1`, which is the project that holds +# Delta's main sources. The bundle's transitive contents include extra symbols +# under `org.apache.spark.sql` that collide with Delta's main sources -- e.g. +# MergeOutputGeneration.scala imports both `org.apache.spark.sql._` and +# `org.apache.spark.sql.delta.ClassicColumnConversions._`, and would then fail +# with `reference to expression is ambiguous`. +# +# sbt auto-scans `<baseDirectory>/lib` via `unmanagedBase`. Two relevant +# projects in Delta v4.2.0 have a `lib/` baseDirectory: +# - sparkV1: `project in file("spark")` -> spark/lib +# - spark : `project in file("spark-unified")` -> spark-unified/lib +# unmanagedJars are project-scoped (NOT inherited by dependents), so dropping +# the bundle into spark-unified/lib/ adds it to the unified `spark` project's +# Compile *and* Test classpaths -- but NOT to sparkV1's. That's exactly what +# we want: +# * sparkV1/Compile sees ONLY Delta's regular deps -> Delta main compiles. +# * spark/Test/fullClasspath sees the bundle -> tests load GlutenPlugin. +# (Verified empirically: with bundle only in spark-unified/lib/, sbt's +# `show sparkV1/Compile/dependencyClasspath` excludes the bundle and +# `show spark/Test/fullClasspath` includes it.) +# +# We deliberately do NOT also drop the bundle into spark/lib/, which is what +# caused the previous compile failure: spark/lib/ is sparkV1's unmanagedBase, +# and putting the bundle there would re-introduce the ambiguity errors. +SPARK_UNIFIED_LIB="$DELTA_DIR/spark-unified/lib" +mkdir -p "$SPARK_UNIFIED_LIB" +cp "$GLUTEN_BUNDLE_JAR" "$SPARK_UNIFIED_LIB/gluten-velox-bundle.jar" +ls -lh "$SPARK_UNIFIED_LIB" +echo "::endgroup::" + +echo "::group::Patching DeltaSQLCommandTest to enable Gluten plugin" +TARGET="$DELTA_DIR/spark/src/test/scala/org/apache/spark/sql/delta/test/DeltaSQLCommandTest.scala" +if [ ! -f "$TARGET" ]; then + echo "Expected file not found in Delta clone: $TARGET" >&2 + echo "The Delta directory layout for ref '${DELTA_REF}' may have changed." + exit 1 +fi +cp "$PATCH_SOURCE" "$TARGET" +echo "Patched $TARGET" +echo "--- diff vs. upstream ---" +git -C "$DELTA_DIR" --no-pager diff -- "spark/src/test/scala/org/apache/spark/sql/delta/test/DeltaSQLCommandTest.scala" || true +echo "::endgroup::" + +# Delta's tests collect file-source scans by matching the concrete +# `FileSourceScanExec` case class; Gluten offloads the scan to +# DeltaScanTransformer, a `FileSourceScanLike` sibling, so those matches miss +# (`scala.MatchError: List()`, empty partition filters, broken column-pruning / +# scan-metric checks across many suites). delta-io/delta#7104 and #7105 widen the +# matches to the shared `FileSourceScanLike` interface that both the vanilla and +# Gluten scans implement (behavior-preserving for vanilla). Both are merged +# upstream but land after the pinned DELTA_REF (v4.2.0), so apply them here; once +# DELTA_REF includes a fix, cherry_pick_delta_fix detects it and skips (see below). +# +# Depth-2 fetch brings each fix commit and its parent, which cherry-pick needs to +# diff against (a depth-1 fetch grafts the parent away); `-n` stages the change +# without requiring a committer identity. +cherry_pick_delta_fix() { + local sha="$1" pr="$2" + git -C "$DELTA_DIR" fetch --quiet --depth 2 origin "$sha" + echo "Cherry-picking delta-io/delta${pr}" + if git -C "$DELTA_DIR" cherry-pick -n "$sha"; then + return 0 + fi + # The cherry-pick did not apply. The usual cause is that the pinned DELTA_REF + # already contains this fix (e.g. after a version bump), which makes the patch + # empty/conflicting and would -- under `set -e` -- abort the whole setup. We + # can't use ancestry to tell "already contained" from a genuine conflict here + # (the clone is shallow, so `merge-base --is-ancestor` can't see past the graft), + # so recover the exact paths this fix touches -- leaving other setup such as the + # DeltaSQLCommandTest patch intact -- and continue. This is self-correcting: if + # the fix is genuinely still needed, the FileSourceScanLike failures it prevents + # resurface as gate regressions rather than being hidden by a hard abort here. + echo "Cherry-pick of delta-io/delta${pr} did not apply cleanly" \ + "(most likely already contained in ${DELTA_REF}); skipping it." + local f + while IFS= read -r f; do + [ -n "$f" ] || continue + git -C "$DELTA_DIR" reset -q -- "$f" 2>/dev/null || true + git -C "$DELTA_DIR" checkout -q -- "$f" 2>/dev/null || true + done < <(git -C "$DELTA_DIR" diff-tree --no-commit-id --name-only -r "$sha") + # Clear any leftover sequencer state (harmless if none exists). + git -C "$DELTA_DIR" cherry-pick --quit 2>/dev/null || true + return 0 +} + +echo "::group::Cherry-picking upstream Delta FileSourceScanLike test fixes" +cherry_pick_delta_fix 46bd45d57eadd7e528002a0ae7bd36ce5a456eca "#7104 (ScanReportHelper.collectScans)" +cherry_pick_delta_fix 959e00e15f41f56afc1c9bb95d160c55c6dc7068 "#7105 (9 more test suites)" +echo "::endgroup::" + +echo "::group::Force-failing memory-hog DeletionVectorsSuite 2B-row tests" +# Two DeletionVectorsSuite tests read from / delete from a 2-billion-row table. +# Under the Gluten Velox bundle they balloon the forked test JVM to ~13G of +# NATIVE memory (row-index materialization) and the kernel/cgroup OOM-kills it. +# The dead fork then wedges sbt, hanging the whole shard until the workflow's +# hang-watchdog dumps threads and kills it (~16 min wasted, and every suite +# QUEUED AFTER it in that fork is skipped) -- see delta_spark_ut.yml. +# +# Rather than silently `ignore` these (easy to forget), we make them FAIL FAST +# with a clear message: the gap stays visible in the test reports / baseline +# until the native memory blow-up is fixed, at which point this patch should be +# removed. NOTE: making the suite complete also un-skips the rest of the shard's +# suite queue, so the known-failures baseline must be refreshed after this. +# +# ORDER MATTERS: keep this sed AFTER the cherry-picks above. #7105 also edits +# DeletionVectorsSuite.scala, and git cherry-pick aborts (exit 128) when the work +# tree has uncommitted edits to a file it touches. +DVS="$DELTA_DIR/spark/src/test/scala/org/apache/spark/sql/delta/deletionvectors/DeletionVectorsSuite.scala" +if [ ! -f "$DVS" ]; then + echo "Expected file not found in Delta clone: $DVS" >&2 + echo "The Delta directory layout for ref '${DELTA_REF}' may have changed." >&2 + exit 1 +fi +# Inject `fail(...)` as the first statement of each test body (the line ending +# in `) {`). Delta sets no -Xfatal-warnings / dead-code warning, so the now- +# unreachable original body compiles fine. Keep each injected line <100 chars: +# Delta's scalastyle enforces a 100-char line length on test sources. The full +# rationale lives in this comment, so the in-test message stays terse. +sed -i 's#huge table: read from tables of 2B rows with existing DV of many zeros") {#&\n fail("[Gluten CI] Force-failed: 2B-row DV read OOMs the test JVM; see setup-delta.sh")#' "$DVS" +sed -i 's#number of rows from tables of 2B rows with DVs") {#&\n fail("[Gluten CI] Force-failed: 2B-row DV delete OOMs the test JVM; see setup-delta.sh")#' "$DVS" +INJECTED=$(grep -c "Gluten CI] Force-failed" "$DVS" || true) +if [ "$INJECTED" -ne 2 ]; then Review Comment: This force-fail injection is not idempotent: re-running `setup-delta.sh` in an existing workspace will inject duplicate `fail(...)` lines, causing `INJECTED` to exceed 2 and the script to abort. To keep reruns reliable (especially for local dev / CI reruns reusing workspace), reset `DeletionVectorsSuite.scala` to upstream before applying the `sed` (e.g., `git checkout -- \"$DVS\"`) or guard each injection by checking whether the fail line is already present before inserting. ########## .github/workflows/util/delta-spark-ut/flaky-error-patterns.txt: ########## @@ -0,0 +1,43 @@ +# Flaky-error signatures for the Delta Spark UT (Gluten) gate. +# +# Each non-comment line is a Python regex matched (re.search, case-sensitive) +# against a failed test's JUnit <failure>/<error> text (message + stack). A +# failure that matches is QUARANTINED by root cause: it never counts as a +# regression, and is dropped from the shard's failures list so it can't leak +# into the regenerated baseline -- regardless of WHICH test it landed on. +# +# Use this (instead of flaky-tests.txt) when a known nondeterministic bug +# surfaces on a different test each run, so matching by test name is +# whack-a-mole. Prefer fixing the underlying bug and REMOVING the entry. +# +# --------------------------------------------------------------------------- +# Native Delta bitmap aggregator receives an INVALID row index during a MERGE +# that writes deletion vectors, and aborts. The garbage row index trips one of +# two bounds checks, so the same root cause shows up with two messages: +# +# too large (Long.MAX_VALUE): +# VeloxRuntimeError INVALID_STATE +# Reason: Delta RoaringBitmapArray row index 9223372036854775807 exceeds max +# representable value 9223372030412324864 +# Expression: value <= kMaxRepresentableValue +# Function: addSafe File: .../velox/compute/delta/RoaringBitmapArray.cpp:92 +# +# negative (garbage): +# VeloxRuntimeError INVALID_STATE +# Reason: Delta bitmap row index cannot be negative: -6254810385378525259 +# Expression: value >= 0 +# Function: addRowIndex File: .../operators/functions/delta/DeltaBitmapAggregator.cc:44 +# +# Both are the same root cause (garbage row index into the DV bitmap aggregator) +# but distinct native errors, so each has its own explicit pattern below -- +# deliberately specific (bound to the exact error string) rather than a broad +# "Delta bitmap row index" match, to avoid masking an unrelated failure. Add a +# new line if a further bounds-check variant appears. Intermittent (depends on +# the runtime plan/scan/scheduling), so it hits a different *DVs*Suite MERGE test +# on each run. Remove these once the row-index materialization is fixed in the +# native backend. Tracked upstream (DV bitmap invalid row index). +# --------------------------------------------------------------------------- +# too-large (Long.MAX_VALUE) -- RoaringBitmapArray.cpp addSafe, value <= kMaxRepresentableValue +Delta RoaringBitmapArray row index \d+ exceeds max representable value +# negative garbage -- DeltaBitmapAggregator.cc addRowIndex, value >= 0 +Delta bitmap row index cannot be negative: -?\d+ Review Comment: This regex is slightly broader than necessary: `-?\\d+` would also match a (hypothetical) positive index even though the message says 'cannot be negative'. Tightening it to `-\\d+` reduces the chance of accidentally quarantining an unrelated failure if the error message format changes. ########## .github/workflows/delta_spark_ut.yml: ########## @@ -0,0 +1,459 @@ +# 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. + +# Runs Delta Lake's `spark` sbt module unit tests against a Gluten Velox bundle +# that is built from the source in this repository. The pipeline: +# +# 1. Builds the Velox/Gluten native libraries (centos-7 + vcpkg, x86_64). +# 2. Builds the Gluten Java/Scala jars and assembles the +# `gluten-velox-bundle-spark<spark>_<scala>-linux_amd64-<version>.jar` +# fat jar for Spark 4.1 + Scala 2.13 + Java 17 with the Delta profile. +# 3. Clones delta-io/delta at the requested release tag (default `v4.2.0`), +# drops the bundle jar into `spark-unified/lib/` only (NOT `spark/lib/` +# -- see setup-delta.sh for the unmanagedJars scoping rationale), +# patches Delta's `DeltaSQLCommandTest` to register the Gluten plugin, +# and runs `sbt spark/test` sharded across the matrix. +# +# Limited to Velox + x86 to keep the matrix simple, per the pipeline's purpose +# of validating Gluten changes against the latest Delta release. + +name: Delta Spark UT (Gluten) + +on: + # Reusable workflow. velox_backend_x86.yml calls this (gated on Delta-relevant + # changes) and passes the native-lib artifact it already built, so the expensive + # native C++ build is NOT duplicated. That artifact lives in the CALLER's run (a + # called workflow runs as part of the caller run), so the jobs below download it + # by name. See velox_backend_x86.yml `delta-spark-ut`. + # + # NOTE: the `pull_request` trigger was removed so this no longer runs as its own + # workflow on PRs (which would double-run the Delta suite). velox_backend_x86.yml + # is now the single PR entry point; `workflow_dispatch` keeps manual standalone + # runs working (those build the native lib themselves -- see build-native-lib). + workflow_call: + inputs: + native_lib_artifact: + description: 'Name of the cpp/build artifact uploaded by the caller' + type: string + required: true + delta_ref: + type: string + required: false + default: 'v4.2.0' + spark_version: + description: 'Spark version driving both the Gluten bundle profile (-Pspark-<v>) and Delta -DsparkVersion.' + type: string + required: false + default: '4.1' + test_parallelism: + type: string + required: false + default: '4' + update_baseline: + type: boolean + required: false + default: false + fail_on_fixed: + type: boolean + required: false + default: true + workflow_dispatch: + inputs: + delta_ref: + description: 'delta-io/delta git ref (tag/branch/SHA) to test against' + required: true + default: 'v4.2.0' + spark_version: + description: 'Spark version: drives the Gluten bundle profile (-Pspark-<v>) and Delta -DsparkVersion together. Scala 2.13 + JDK 17 are assumed, so pair a non-4.1 value with a compatible delta_ref.' + required: true + default: '4.1' + test_parallelism: + description: 'Forked test JVMs per shard (TEST_PARALLELISM_COUNT)' + required: true + default: '4' + update_baseline: + description: 'Seed/refresh the known-failures baseline instead of enforcing it' + type: boolean + required: false + default: false + fail_on_fixed: + description: 'Fail when a baseline test now passes (keeps the baseline honest)' + type: boolean + required: false + default: true + # Nightly full run against the latest default branch. The per-PR entry point + # (velox_backend_x86.yml) now runs the Delta suite only when a PR touches + # Delta-relevant paths (or carries the opt-in label), to save GHA minutes; this + # scheduled run keeps full coverage once a day so rarer regressions are still + # caught. It builds its own native lib (build-native-lib-centos-7 below) since + # there is no caller to provide one, and uses the workflow's default inputs. + schedule: + - cron: '0 5 * * *' + +env: + ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION: true + MVN_CMD: 'build/mvn -ntp' Review Comment: `ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION: true` relaxes GitHub Actions’ Node runtime safety checks and can allow older EOL Node versions for actions, which is a supply-chain/security risk. If this isn’t strictly required for a specific legacy action, remove it; if it is required, scope it as narrowly as possible (job/step) and document which action requires it and why. ########## .github/workflows/velox_backend_x86.yml: ########## @@ -101,6 +106,70 @@ jobs: path: ./cpp/build/ if-no-files-found: error + # Gate the (expensive) Delta Spark UT suite so per-PR it runs only when the PR + # touches high-signal Delta paths -- the Delta integration code + # (backends-velox/src-delta*), the gluten-delta module, or this pipeline's own + # files -- or carries the `run-delta-ci` opt-in label. Changes to general + # Velox/core/native code can also affect Delta offload but are touched + # constantly, so per-PR they skip it; the nightly full run (delta_spark_ut.yml + # `schedule`) and the opt-in label are the safety nets. This keeps GHA usage + # down. NOTE: the label is read from the event that triggered this run, so add + # it before/with a push; labeling an already-finished run needs a new push. + delta-changes: + runs-on: ubuntu-22.04 + outputs: + run_delta: ${{ steps.filter.outputs.run_delta }} + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + - name: Detect Delta-relevant changes / opt-in label + id: filter + env: + HAS_LABEL: ${{ contains(github.event.pull_request.labels.*.name, 'run-delta-ci') }} + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} Review Comment: The expression `github.event.pull_request.labels.*.name` can fail to evaluate on non-PR events (e.g., `workflow_dispatch`, `push`) because `github.event.pull_request` is absent. This would break the gating job before your bash-level fail-open logic runs. Guard the expression with an event check (e.g., only evaluate `contains(...)` when `github.event_name == 'pull_request'`) or use a conditional expression that safely falls back when `pull_request` is missing. ########## .github/workflows/util/delta-spark-ut/compare-test-results.py: ########## @@ -0,0 +1,820 @@ +#!/usr/bin/env python3 +# +# 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. + +"""Gate / seed / aggregate the Delta-on-Gluten unit test results. + +Running delta-io/delta's ScalaTest suite against the Gluten Velox bundle +produces many *expected* failures (Gluten does not yet support every Delta +code path). To keep the red/green signal meaningful while we fix those +failures incrementally, we maintain a committed baseline of known failing +tests (``known-failures.txt``) and compare each CI run against it. + +This script has three modes: + +``enforce`` (default, per shard) + Parse the JUnit XML produced by ``sbt spark/test`` (ScalaTest ``-u`` + reporter) and compare against the baseline: + + * regression -- a test that FAILED but is NOT in the baseline. These + fail the build: a previously-passing test just started failing. + * expected -- a test that failed and IS in the baseline. Ignored. + * fixed -- a baseline test that now PASSES. By default these also + fail the build (``--fail-on-fixed true``) so the baseline stays honest + and contributors remove entries as they fix them. + + If the baseline file exists but is empty (not yet bootstrapped) the mode + automatically degrades to ``seed`` so the first run is never spuriously red. + A *missing* ``--known-failures`` file is treated as a configuration error + (the gate fails) so a mis-referenced path can't silently pass. + +``seed`` (bootstrap / ``update_baseline``) + Never fails. Just writes the current shard's failing tests so the baseline + can be (re)generated from a real run. + +``aggregate`` (final job) + Merge every shard's ``--failures-out`` / ``--ran-out`` / ``--skipped-out`` + file into a single, sorted, ready-to-commit ``known-failures.txt`` and report + stale baseline entries (tests no longer present in any shard). Skipped tests + are tracked apart from run tests so "stale" means *truly absent* rather than + merely skipped this run; they are also kept out of the now-passing set, since + a skipped test is not evidence of a fix. Pass ``--expected-shards N`` + to fail when fewer than ``N`` shards contributed gate lists (a shard that + died before writing them), so an incomplete baseline is never produced. + +Flaky quarantine (``--flaky-tests``) + Some Delta-on-Gluten failures are non-deterministic (e.g. a native bug that + only triggers on certain runtime plans), so they are neither a stable pass + nor a stable failure and cannot live in the baseline: baselining them turns + the gate red on every run where they pass, and leaving them out turns it red + on every run where they fail. ``flaky-tests.txt`` quarantines them -- a + quarantined test never counts as a regression (when it fails) nor as + now-passing (when it passes), and is excluded from the regenerated baseline. + Its SUITE is an fnmatch glob (so one line covers a root-cause family across + generated suite variants); its TEST name is matched exactly. + +Flaky quarantine by error signature (``--flaky-error-patterns``) + When a failure is caused by a known nondeterministic bug that surfaces on a + *different test each run* (e.g. the native Delta DV bitmap row-index error), + matching by test name is whack-a-mole. ``flaky-error-patterns.txt`` instead + quarantines by root cause: each line is a regex matched against a failed + test's <failure>/<error> text, and any failure that matches is treated as + flaky regardless of which test it landed on. + +Baseline file format (``known-failures.txt``):: + + # comment lines start with '#' + <fully.qualified.SuiteName>#<test display name> + +The suite is always a JVM class name (dot-separated, never starts with '#'), +so a line whose first non-space character is '#' is unambiguously a comment, +and the FIRST '#' after the suite separates suite from the (possibly +'#'-containing) test name. + +Only the Python standard library is used so the script runs in the bare +centos image used by the Delta UT pipeline with no ``pip install``. +""" + +import argparse +import fnmatch +import glob +import os +import re +import sys +import xml.etree.ElementTree as ET + +# Synthetic "test name" recorded when a whole suite aborts (e.g. beforeAll +# throws) so that the JUnit XML reports a suite-level error with no per-test +# <testcase>. Without this, a suite that used to pass but now aborts entirely +# would record zero failing testcases and the regression would be missed. +SUITE_ABORTED = "<suite aborted>" + + +class NoReportsError(RuntimeError): + """Raised when no JUnit <testsuite> elements are found under reports_dir.""" + + +class CorruptReportError(NoReportsError): + """Raised when an expected JUnit report file (TEST-*.xml) fails to parse. + + Subclasses NoReportsError so the enforce/seed handler treats a truncated + report as a hard data error (exit 2) instead of silently dropping the + suite's results and letting the gate pass on partial data. + """ + + +SEP = "#" + + +def eprint(*args, **kwargs): + print(*args, file=sys.stderr, **kwargs) + + +# --------------------------------------------------------------------------- # +# Baseline (known-failures.txt) parsing / formatting +# --------------------------------------------------------------------------- # +def format_entry(suite, test): + return "{}{}{}".format(suite, SEP, test) + + +def parse_entry(line): + """Parse a 'suite#test' line into (suite, test) or return None for blanks/comments.""" + stripped = line.strip() + if not stripped or stripped.startswith("#"): + return None + idx = stripped.find(SEP) + if idx < 0: + # No separator: treat the whole line as a suite-level entry. + return (stripped, SUITE_ABORTED) + return (stripped[:idx], stripped[idx + len(SEP) :]) + + +def normalize_key(suite, test): + """Normalize a (suite, test) key parsed from JUnit XML to match baseline keys. + + Baseline/flaky entries round-trip through write_entries (which collapses CR/LF + in the test name to spaces) and parse_entry (which strips the whole + ``suite#test`` line). A raw XML name carrying a trailing newline or + surrounding whitespace would therefore never equal its normalized baseline + entry: the gate would keep reporting it as a REGRESSION, and the + copy-pasteable line it prints could never suppress it (load strips it back). + Delta test names are freeform, so a version bump could introduce exactly that. + Applying the identical format+parse round-trip here keeps the two sides in + sync (and is a no-op for the normal, whitespace-free names). + """ + safe_test = (test or "").replace("\r", " ").replace("\n", " ") + normalized = parse_entry(format_entry(suite or "", safe_test)) + # parse_entry only returns None for a blank/comment line, which a real + # testcase key is not; fall back to a bare strip to keep this total. + if normalized is None: + return ((suite or "").strip(), safe_test.strip()) + return normalized + + +def load_entries(path): + """Load a set of (suite, test) tuples from a baseline/shard-list file.""" + entries = set() + if not path or not os.path.exists(path): + return entries + with open(path, "r", encoding="utf-8") as fh: + for line in fh: + parsed = parse_entry(line) + if parsed is not None: + entries.add(parsed) + return entries + + +def make_is_flaky(flaky_entries): + """Build a predicate that matches a (suite, test) tuple against flaky entries. + + A flaky entry quarantines a test whose failure is known to be non-deterministic + (see flaky-tests.txt). The entry's SUITE is treated as an fnmatch glob so a + single line can cover a root-cause family across generated suite variants + (e.g. ``*DVs*Suite`` matches every deletion-vector merge suite, ``*`` matches + any suite); the TEST name is matched exactly (test names are freeform and may + contain glob metacharacters, so they are never globbed). + """ + exact = set() + globbed = [] + for suite, test in flaky_entries: + if any(ch in suite for ch in "*?["): + globbed.append((suite, test)) + else: + exact.add((suite, test)) + + def is_flaky(entry): + if entry in exact: + return True + suite, test = entry + for glob_suite, glob_test in globbed: + if test == glob_test and fnmatch.fnmatchcase(suite, glob_suite): + return True + return False + + return is_flaky + + +def load_patterns(path): + """Load flaky-error regex patterns from a file (one per line). + + Blank lines and ``#`` comments are ignored. Each remaining line is compiled + as a case-sensitive regex. These match the FAILURE TEXT of a failed test + (its JUnit <failure>/<error> message + stack), so a test that fails with a + known-nondeterministic native error (e.g. the Delta DV bitmap row-index bug) + can be quarantined by root cause instead of by exact test name. + """ + patterns = [] + if not path or not os.path.exists(path): + return patterns + with open(path, encoding="utf-8") as fh: + for line in fh: + line = line.rstrip("\n") + if not line.strip() or line.lstrip().startswith("#"): + continue + patterns.append(re.compile(line)) + return patterns Review Comment: If `flaky-error-patterns.txt` contains an invalid regex, `re.compile(line)` will raise `re.error` and crash the script without a clear, actionable message about which line/pattern broke CI. Catch `re.error` here and emit a targeted error including the file path and offending pattern (and ideally its line number), then exit with a non-zero code. -- 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]
