villebro opened a new pull request, #2790:
URL: https://github.com/apache/datafusion-ballista/pull/2790
# Which issue does this PR close?
Not related to an issue. It fixes the failing `Ballista Rust` workflow on
`main` after #2765.
# Rationale for this change
`state::aqe::test::coalesce_rule::should_resolve_every_declared_partition_of_coalesced_passthrough_stage`
fails on `main` (`assert_eq!(2, stages.len())`, got 1).
The test runs `select count(*) from t1 join t2 on t1.c = t2.c`. Projection
pushdown leaves both
scans reading only the join key, and `t1` and `t2` hold identical data, so
the two exchanges are
the same subplan. Exchange reuse is enabled by default since #2765, so the
planner runs them as
one shared stage and only one leaf stage is runnable. The test counts two.
The other join tests
in the file select `t1.a` and `t2.b`, so their scans read different columns
and stay separate
stages.
Neither change is wrong alone: the test came from #2782 and the default from
#2765. Two PRs that
each passed CI landed a failure between them.
## Why CI did not catch it
- #2765's last push was at 21:19 UTC on 5 Nov, and `Ballista Rust` ran on
the pull request
at that time and passed.
- #2782, which added this test, merged at 21:56 UTC on 5 Nov, 37 minutes
after that run. A
pull request run tests the PR merged with `main` as it was when the run
started, and it is not
repeated when `main` moves.
- #2765 merged on 6 Nov through the merge queue, which builds the exact tree
that lands. The
queue ran `TPC benchmarks` and `Dependencies` on that tree, and nothing
else. `rust.yml` triggers
on `push`, `pull_request` and `workflow_dispatch`, but not on
`merge_group`, so the unit tests
never ran on the queue's tree.
- The status checks required for `main` (`.asf.yaml`) are `TPC Required
Checks` only, so the Rust
jobs would not have gated the queue even if they had run there.
- `Ballista Rust` ran for the first time on the combined tree after the
merge, on the push to
`main`, and failed.
A change that is not part of this PR would close that gap: add
`merge_group:` to the triggers of
`.github/workflows/rust.yml`, and require a Rust aggregate job next to `TPC
Required Checks` in
`.asf.yaml` (a queue only waits for required checks). It is left out here
because a required context
that never reports blocks every merge to `main`, as the comment in
`.asf.yaml` warns.
# What changes are included in this PR?
- The query of that test also selects `t1.a` and `t2.b` (`select count(*),
sum(t1.a + t2.b) ...`),
so its two scans read different columns, like the other join tests, and
stay two stages with
exchange reuse on. It is still a global aggregate over a join, and its
assertions are unchanged.
Exchange reuse stays enabled in the test fixture.
- A new test, `should_coalesce_the_readers_of_a_shared_stage`, covers what
the old query exercised
under the default configuration: with the two identical scans merged into
one shared stage,
coalescing attaches `coalesce=2 of 8` to both readers, and the join stage
reads 2 partitions.
No test covered coalescing together with a reused stage before.
# Are there any user-facing changes?
No. Test-only change.
--
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]
