andygrove opened a new pull request, #2182: URL: https://github.com/apache/datafusion-ballista/pull/2182
# 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. Two gaps followed from that. **The adaptive planner was untested.** AQE was deliberately left out of the workflow because it failed many TPC-DS queries with an `EmptyExec invalid partition` assertion. That was tracked as #2047, which is now closed, so the exclusion is stale and AQE has no TPC-DS coverage at all. Given #2092 aims to enable AQE by default, that is the configuration most in need of a correctness gate. **Five queries were excluded for a reason that turns out to be trivially fixable.** q64, q81, q84, q85 and q93 were skipped as "tpcgen-cli schema column-name mismatch with query text". Checking the generator's schema against the query text, the mismatch is three pure renames: | Query text (TPC-DS spec) | tpcgen-cli emits | Affects | |---|---|---| | `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 | No column is missing, the data is present under the other name, so all five can run once the reference is rewritten. # What changes are included in this PR? **Workflow (`.github/workflows/tpcds.yml`)** Replaces the single job with a four-leg matrix, following the shape `tpch.yml` already uses: | Leg | Planner | `--partitions` | |---|---|---| | `aqe-off-p16` | static | 16 | | `aqe-off-p1` | static | 1 | | `aqe-on-p16` | adaptive | 16 | | `aqe-on-p1` | adaptive | 1 | - Per-leg `label` / `planner_args` / `partitions` / `slug`, matching `tpch.yml`'s convention. - `fail-fast: false`, so one red leg does not mask the others. - 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. This mirrors `tpch.yml`'s `shared-key: tpch-sf10`. - The cluster-log artifact is now named per leg so a failure in one leg does not collide with another. The single-partition legs are new coverage in their own right: they exercise the planner where stages have one partition, which is where the partitioning assertions behind #2047 originally showed up. **Query loading (`benchmarks/src/bin/tpcds.rs`)** - Adds a `COLUMN_RENAMES` table and applies it to the query text at load time, which removes q64, q81, q84, q85 and q93 from `SKIP`. The gate goes from 92 to 97 of 99 queries; only the two non-deterministic ones (q31, q71) remain skipped. - The rewrite is applied to the query string, so the Ballista run and the single-process DataFusion oracle see identical SQL and the comparison stays apples-to-apples. - It is applied at load time rather than by editing the files under `benchmarks/queries-tpcds/`, because `dev/vendor-tpcds-queries.sh` re-downloads all 99 queries and would clobber a local edit. - The replacement is whole-word rather than a plain `str::replace`: `r_reason_desc` is a prefix of `r_reason_description`, so a substring replace would corrupt an already correct reference. - Drops the now-stale comment about #2047 from the skip list. Four unit tests cover the word-boundary behaviour, idempotency, full coverage of the rename table, and that the rewrite is actually reached from the query load path. # Are there any user-facing changes? No. This only affects CI coverage and the benchmark runner. ## Note on CI status Opened as a draft on purpose. The two AQE legs have never run against TPC-DS, and the five newly unskipped queries have never been executed under any configuration, so this PR's own CI run is the first real signal on both. If a leg comes back red it is surfacing a genuine pre-existing bug rather than a regression from this change, and I will triage from there before marking it ready. -- 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]
