[MINOR][CI][DOC] Document the Delta Spark UT pipeline and skip it for docs-only changes - #12745
[MINOR][CI][DOC] Document the Delta Spark UT pipeline and skip it for docs-only changes#12745felipepessoto wants to merge 11 commits into
Conversation
velox-backend-CI.md describes the Velox CI workflows but did not mention delta_spark_ut.yml, added in apache#12388. A contributor whose PR suddenly runs a multi-hour Delta suite had no entry point explaining what it is or how the known-failures baseline works, short of reading the workflow. Add a section covering why the job gates against a baseline rather than on any failure, when it runs (Delta paths per PR, nightly, or workflow_dispatch), and how to refresh the baseline, linking the pipeline README and the follow-up tracker apache#12743. Generated-by: GitHub Copilot CLI Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 66a9e40f-3ac8-45be-8fee-a606a22fa098
There was a problem hiding this comment.
Pull request overview
Documents the existing Delta Spark UT GitHub Actions workflow in the Velox CI developer docs so contributors understand what the multi-hour Delta suite is, when it runs, and how its known-failures baseline gate works.
Changes:
- Add a “Delta Spark UT” section describing baseline-gated behavior (new failures vs now-passing baseline entries).
- Document workflow triggers (paths-filtered per-PR, nightly schedule, and manual
workflow_dispatch). - Add baseline refresh instructions and link to the pipeline README and follow-up tracker issue.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
"download the artifact and commit it" did not say what to do with the file, so it could be read as committing the downloaded artifact somewhere new rather than replacing the committed baseline in place. Name the destination path, matching the wording the pipeline README already uses for bootstrapping. Generated-by: GitHub Copilot CLI Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 66a9e40f-3ac8-45be-8fee-a606a22fa098
The Delta section was hard-wrapped at ~95 columns while the rest of the file puts each paragraph on a single line, which showed up as inconsistent wrapping in the rendered docs and made the diff noisier than the change warranted. Reflow it; the text itself is unchanged. Generated-by: GitHub Copilot CLI Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 66a9e40f-3ac8-45be-8fee-a606a22fa098
The section said the tests fail because Gluten "does not offload every Delta code path". That is not the reason: when Gluten cannot offload a plan it is supposed to fall back to vanilla Spark and the test should still pass, so a missing offload on its own is not a failure. The baseline entries are real gaps -- fallback not happening where it should (e.g. apache#12538, where the Delta stats tracker throws instead of falling back), metrics that differ from vanilla (apache#9003), and native-side bugs (apache#12377, apache#12387). Describe them that way, so the baseline isn't read as an inherent property of offloading. Generated-by: GitHub Copilot CLI Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 66a9e40f-3ac8-45be-8fee-a606a22fa098
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
docs/developers/velox-backend-CI.md:44
- The section says the workflow "fails only" on regressions or now‑passing baseline tests, but the gate also intentionally fails on data/infra errors (e.g., missing JUnit reports or corrupt/truncated TEST-*.xml) to avoid passing on partial/empty results. This “only” wording is too absolute and could mislead contributors debugging a red run.
`delta_spark_ut.yml` runs delta-io/delta's own `spark` test suite against a Gluten Velox bundle, so Gluten is validated against a real Delta release. A number of those tests fail today -- not because Gluten declines to offload a plan, which should fall back to vanilla Spark transparently, but because of real gaps: fallback not happening where it should, metrics that differ from vanilla, and native-side bugs. So the job does not gate on "any failure": it compares each run against a committed baseline of known failures in `.github/workflows/util/delta-spark-ut/known-failures.txt` and fails only on a **new** failure, or on a baseline test that starts **passing** (which means the baseline needs updating).
Some baseline entries are not defects at all: a Delta test that asserts on the query plan sees a different plan once Gluten offloads the scan or operators, which is by design. delta-io/delta#7104 and apache#7105 are examples that were fixed upstream by widening a FileSourceScanExec match to FileSourceScanLike; others are simply expected to stay in the baseline. Generated-by: GitHub Copilot CLI Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 66a9e40f-3ac8-45be-8fee-a606a22fa098
The Delta path filter matches `util/delta-spark-ut/**`, which includes the pipeline's own README, so fixing a typo there costs a full run: 8 shards at roughly 75-100 minutes each, about 10 runner-hours, to test a change that cannot affect the suite. Exclude Markdown under that directory. `paths` and `paths-ignore` cannot both be used, so this is a negated pattern, which must stay last because patterns are evaluated in order. Also correct the README's description of why the tests fail. It said Gluten "does not yet offload every Delta code path", but a plan Gluten cannot offload should fall back to vanilla Spark and the test should still pass, so a missing offload is not by itself a failure. The baseline entries are real gaps: fallback not happening where it should (apache#12538), metrics that differ from vanilla (apache#9003), and native-side bugs (apache#12377, apache#12387). Same correction applied to the "intentionally added a new expected failure" example. While here, say where the refreshed baseline artifact goes, matching the wording the bootstrapping section already uses. Generated-by: GitHub Copilot CLI Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 66a9e40f-3ac8-45be-8fee-a606a22fa098
Cosmetic only: the paragraph was left with a short line after an edit. The wording is unchanged. Generated-by: GitHub Copilot CLI Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 66a9e40f-3ac8-45be-8fee-a606a22fa098
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/workflows/delta_spark_ut.yml:64
- The negated
pathspattern looks like it won’t actually match Markdown files under subdirectories (orREADME.md) because**only acts as a “globstar” when it’s a full path segment. With**.md, a README edit may still trigger the workflow, defeating the intent to skip docs-only changes.
- '!.github/workflows/util/delta-spark-ut/**.md'
The section said the job fails "only" on a new failure or a now-passing baseline test. The gate also exits non-zero when a run produced no usable results -- no JUnit reports, a truncated TEST-*.xml, or fewer shards than expected -- so that a partial run cannot pass silently. Mention that, so someone debugging a red run is not looking for a test failure that is not there. Generated-by: GitHub Copilot CLI Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 66a9e40f-3ac8-45be-8fee-a606a22fa098
|
Addressed the two suppressed review comments: "fails only ... is too absolute" — correct, fixed in 015c5b5. The gate also exits non-zero when a run produced no usable results (no JUnit reports, a truncated " Worth noting for reviewers what this does and doesn't buy: for |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/workflows/util/delta-spark-ut/README.md:129
- The README explains earlier that an unoffloadable plan should fall back to vanilla Spark and the test should still pass, so using “known fallback” here as an example reason for adding an expected failure is internally inconsistent/misleading. Consider rephrasing this example to focus on plan-assertion tests (legit plan differences once Gluten offloads) or intentionally-accepted tracked gaps/bugs.
- **You intentionally added a new expected failure** (e.g. a test that hits a
known fallback or native-side gap). Add the exact `Suite#test` line(s) the gate prints under
*Regressions* to `known-failures.txt`, ideally with a comment explaining why.
The README opens by saying a plan Gluten cannot offload should fall back to vanilla Spark and the test should still pass, then offered "a test that hits a known fallback" as a reason to add an expected failure to the baseline. Those contradict: a fallback that works is not a failure. Use the two cases that actually belong in the baseline instead -- a test that asserts on a query plan which legitimately differs once Gluten offloads, or one that hits a tracked bug the author is not fixing in that PR. Generated-by: GitHub Copilot CLI Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 66a9e40f-3ac8-45be-8fee-a606a22fa098
…aths The README says the filter covers `util/delta-spark-ut/**`, which is no longer quite true now that Markdown under that directory is excluded. Say so, since this is the file a contributor reads to find out whether their change runs the suite. Generated-by: GitHub Copilot CLI Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 66a9e40f-3ac8-45be-8fee-a606a22fa098
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/workflows/delta_spark_ut.yml:64
- The negated path filter
!.github/workflows/util/delta-spark-ut/**.mddoes not exclude Markdown files at arbitrary depth under that directory (it won’t match e.g..github/workflows/util/delta-spark-ut/subdir/notes.md). If the intent is to skip all Markdown under the pipeline directory, use**/*.mdso nested docs don’t trigger the expensive workflow.
# Documentation can't change what the suite does, and this job costs ~10
# runner-hours, so don't spend that on a README edit. Negated patterns are
# evaluated in order and override the positives above, so this must stay
# last. (`paths` and `paths-ignore` cannot both be used, hence the `!`.)
- '!.github/workflows/util/delta-spark-ut/**.md'
Switch the negated pattern from `**.md` to `**/*.md`. Both should behave the same -- the filter cheat sheet says `**` "matches any character including slash", and gives `**.js` as matching `src/js/app.js` -- but `docs/**/*.md` is the form the same table documents for "a file with a .md suffix anywhere in the docs directory", and its examples cover the top-level file as well as nested ones. Being unambiguous is worth more than being terse here: if the pattern silently failed to match, every README edit would cost a full ~10 runner-hour run, which is exactly what this exclusion exists to avoid. Generated-by: GitHub Copilot CLI Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 66a9e40f-3ac8-45be-8fee-a606a22fa098
|
Changed the pattern to Per the filter pattern cheat sheet, That said, |
philo-he
left a comment
There was a problem hiding this comment.
@felipepessoto, thanks for your continued efforts. Some minor comments. Please check if they make sense.
| # Documentation can't change what the suite does, and this job costs ~10 | ||
| # runner-hours, so don't spend that on a README edit. `**/*.md` is the | ||
| # documented spelling for "a .md file anywhere under this directory" and | ||
| # covers both the README and any nested notes. Negated patterns are |
There was a problem hiding this comment.
Nit:
Can we remove "Documentation can't chang ...and any nested notes." for brevity?
| Note that GitHub secrets are not accessible in PRs from forked repos. | ||
|
|
||
| ## Delta Spark UT | ||
| `delta_spark_ut.yml` runs delta-io/delta's own `spark` test suite against a Gluten Velox bundle, so Gluten is validated against a real Delta release. A number of those tests fail today. Not because Gluten declines to offload a plan -- that should fall back to vanilla Spark and the test should still pass. Some are real gaps (fallback not happening where it should, metrics that differ from vanilla, native-side bugs), and some are expected: a test that asserts on the query plan sees a different plan once the scan or operators are offloaded, which is by design rather than a defect. So the job does not gate on "any failure": it compares each run against a committed baseline of known failures in `.github/workflows/util/delta-spark-ut/known-failures.txt` and fails on a **new** failure, or on a baseline test that starts **passing** (which means the baseline needs updating). It also fails outright if a run produced no usable results -- missing or truncated JUnit reports, or fewer shards than expected -- rather than passing on partial data. |
There was a problem hiding this comment.
Nit:
All of this text sits on line 44, which is hard to work with in an editor. It also means a future one-word edit will show the entire line as changed in the diff. Generally it's better to wrap at 100-150 characters, or put each sentence on its own line. Same applies to the other long lines in this document. Thanks!
What changes are proposed in this pull request?
Two related follow-ups to #12388.
Docs.
docs/developers/velox-backend-CI.mddescribes the Velox CI workflows but did not mentiondelta_spark_ut.yml. A contributor whose PR suddenly runs a multi-hour Delta suite had no entry point explaining what it is, or why it does not simply fail on a failing test. Adds a short section covering the baseline gate, when the job runs, and how to refresh the baseline.CI. The Delta path filter matches
util/delta-spark-ut/**, which includes the pipeline's own README, so fixing a typo there costs a full run -- 8 shards at roughly 75-100 minutes each, about 10 runner-hours, to test a change that cannot affect the suite. Markdown under that directory is now excluded.pathsandpaths-ignorecannot both be used, so this is a negated pattern, which must stay last because patterns are evaluated in order.While here, both documents said the tests fail because Gluten "does not offload every Delta code path". That is not the reason: a plan Gluten cannot offload should fall back to vanilla Spark and the test should still pass. The failures are either real gaps -- fallback not happening where it should (#12538), metrics that differ from vanilla (#9003), native-side bugs (#12377, #12387) -- or expected rather than defects, where a test asserts on a query plan that legitimately differs once Gluten offloads. Both are described that way now.
How was this patch tested?
The described behaviour was checked against
delta_spark_ut.ymlonmain: theschedulecron, thepull_requestpath filter, and thedelta-spark-ut-known-failuresartifact name. The new filter was simulated against real paths -- the README is skipped, whilerun-delta-tests.sh,known-failures.txt, the workflow itself andgluten-delta/**still trigger. This PR changes the workflow, so it exercises the suite once itself.Was this patch authored or co-authored using generative AI tooling?
Generated-by: GitHub Copilot CLI