ci: run TPC-DS with AQE on/off at 1 and 16 partitions#2182
Draft
andygrove wants to merge 3 commits into
Draft
Conversation
…lumn names tpcgen-cli names three columns differently from the TPC-DS spec, while the vendored DataFusion query text follows the spec, so q64, q81, q84, q85 and q93 failed at plan time and were excluded from the correctness gate: ib_income_band_sk -> ib_income_band_id (q64, q84) r_reason_desc -> r_reason_description (q85, q93) cr_return_amt_inc_tax -> cr_return_amount_inc_tax (q81) These are pure renames, the data is present under the other name, so rewriting the reference at load time is enough. The same rewrite is applied to both the Ballista run and the DataFusion oracle, so the comparison stays apples-to-apples. Rewrite at load time rather than editing benchmarks/queries-tpcds/, because dev/vendor-tpcds-queries.sh re-downloads all 99 queries and would clobber a local edit. The rewrite is whole-word, since r_reason_desc is a prefix of its own replacement and a substring replace would corrupt an already correct reference. This takes the gate from 92 to 97 of 99 queries; only the two non-deterministic ones (q31, q71) remain skipped.
A TPC-DS run that hangs currently sits until GitHub's 6-hour default before reporting anything. That is not hypothetical: the adaptive planner can hang outright when a replan cancels an in-flight stage (apache#2063), and a hung leg was observed sitting for over three hours against ~7 minutes for a passing run. Put the primary timeout on the query step rather than the job, so a hang still reaches the log-upload step and leaves scheduler/executor logs to diagnose, and keep a job-level cap as a backstop. Widen the upload condition from `failure()` to `!success()` so a timed-out or cancelled run uploads its logs too, which is exactly the case where they matter most. Also drop the note about apache#2047, which is fixed.
The TPC-DS gate ran a single configuration: the static planner at 16 partitions. The adaptive planner was left out because it failed many queries with an `EmptyExec invalid partition` assertion, but that issue (apache#2047) is now fixed, so AQE was untested against TPC-DS for no reason. Replace the single job with a four-leg matrix covering AQE off/on at 1 and 16 partitions, following the shape tpch.yml already uses: per-leg label, planner_args and slug, `fail-fast: false` so one red leg does not mask the others, and a per-leg name on the cluster-log artifact. All legs share one `swatinem/rust-cache` entry via `shared-key: tpcds-sf1`, so the binaries are compiled once and restored by the remaining legs rather than rebuilt four times. Three of the four legs currently fail on pre-existing bugs, which is what this coverage is meant to expose: AQE off, 1 partition q5/q80 inflated, q49 crashes, q47/q72 exhaust the per-task memory share AQE on, 1 partition q5 inflated, q49 crashes, q61 returns 0 rows AQE on, 16 partitions hangs on q16 (apache#2063) The single-partition legs are the new signal: q5, q80 and q49 are exactly the queries containing UNION, the path apache#2070 rewrote and verified at partition counts 4, 5, 6, 8, 16, 20 and 32 but never 1.
andygrove
force-pushed
the
ci/tpcds-aqe-partition-matrix
branch
from
July 26, 2026 20:08
e489128 to
93b970a
Compare
andygrove
changed the title
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
1 participant
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.
Which issue does this PR close?
Part of #2092.
Rationale for this change
The TPC-DS gate ran exactly one configuration: the static planner at 16 partitions. The adaptive planner was deliberately left out because it failed many TPC-DS queries with an
EmptyExec invalid partitionassertion, tracked as #2047 — which is now closed. So AQE has no TPC-DS coverage at all, even though #2092 aims to enable it by default. That is the configuration most in need of a correctness gate.Single-partition execution was also never covered, and it turns out to be where the interesting failures are.
What changes are included in this PR?
Replaces the single job with a four-leg matrix, following the shape
tpch.ymlalready uses:--partitionsaqe-off-p16aqe-off-p1aqe-on-p16aqe-on-p1label/planner_args/partitions/slug, matchingtpch.yml's convention.fail-fast: false, so one red leg does not mask the others.swatinem/rust-cacheentry viashared-key: tpcds-sf1, so the binaries are compiled once and restored by the remaining legs rather than rebuilt four times. This mirrorstpch.yml'sshared-key: tpch-sf10.What the new coverage found
All of these reproduce on
mainand are independent of this PR.1. UNION scan restriction is wrong at
target_partitions=1(#2184)73724.99, got294899.96— exactly 4x11343.60, got22687.20— exactly 2xInternal("FileStreamBuilder invalid partition index: 0")q5, q80 and q49 are exactly the three affected queries containing
UNION. #2070 ("map a task's partition through UnionExec when restricting file scans") fixed this precise inflation symptom and states it was verified correct "at every partition count tried (4, 5, 6, 8, 16, 20, 32)" — 1 was never tried.A candidate mechanism, in
restrict_plan_to_partitions(ballista/scheduler/src/state/task_builder.rs): the walker passes the parent's partition indices unchanged through interior nodes, then applies them to leaf file scans viaconfig.file_groups.get(i). When an index exceeds the scan's file-group count,filter_mapsilently drops it. An emptyfile_groupsis exactly what makesFileStreamBuilderreject partition 0, and a missed restriction leaves a scan free to read its whole table and inflate the answer. The comment at that call site already notes the non-empty branch is "currently safe because the only shape emitted ispartitions == 1".2. AQE hangs on q16 at 16 partitions
The leg produced
Query 15 verified against DataFusion: OKand then no further output for over three hours before being cancelled, against ~7 minutes for a passing leg. Consistent with #2063.3. q61 returns 0 rows instead of 1 under AQE at 1 partition (#2185).
Not bugs: q47 and q72 exhaust memory at 1 partition.
--memory-pool-size 2GBsplit across--concurrent-tasks 4gives each task a 476.8 MB fair share, and collapsing to one partition pushes the whole table through a single task. That is workflow tuning, to be addressed when the legs are made green.Are there any user-facing changes?
No. CI coverage only.