[MINOR][CI][DOC] Document the Delta Spark UT pipeline and skip it for docs-only changes - #12745
Open
felipepessoto wants to merge 7 commits into
Open
[MINOR][CI][DOC] Document the Delta Spark UT pipeline and skip it for docs-only changes#12745felipepessoto wants to merge 7 commits into
felipepessoto wants to merge 7 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
Contributor
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
Contributor
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
felipepessoto
changed the title
Contributor
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'
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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