adriangb opened a new pull request, #25634: URL: https://github.com/apache/datafusion/pull/25634
## Which issue does this PR close? - Follow-up to https://github.com/apache/datafusion/pull/25625, which added the `spill_views` suite. It closes no issue. ## Rationale for this change Each `spill_views` benchmark holds its DataFusion memory limit as a literal: `40M` for the `repeated` subgroup and `96M` for the `distinct` subgroup. A benchmark bot trigger comment accepts only `env`, `baseline` and `changed`, so today you cannot change that limit from a trigger comment. Two things follow from that. **A query that runs out of memory returns no numbers at all.** When one query hits the limit, the suite run stops, and the queries that did pass report nothing. The comparison gives back a failure instead of numbers. This is not hypothetical: on the benchmark bot, https://github.com/apache/datafusion/pull/23565 runs out of memory on `q05` (`GROUP BY` on all-distinct strings) at `96M` on the branch side in 4 of 4 runs, while 3 of 3 main-against-main runs pass. With a higher limit both sides finish, and you compare time and memory. **You cannot sweep the limit.** The limit is the knob that says how much a change matters, and a sweep shows where the two sides start to differ. ## What changes are included in this PR? Each benchmark takes its memory limit from an environment variable, and today's value stays the default: | Subgroup | Variable | Default | | ---------- | ---------------------------- | ------- | | `repeated` | `SPILL_VIEWS_LIMIT_REPEATED` | `40M` | | `distinct` | `SPILL_VIEWS_LIMIT_DISTINCT` | `96M` | A trigger comment can now raise a limit: ``` run benchmark spill_views env: SPILL_VIEWS_LIMIT_DISTINCT: "128M" ``` `spill_views.suite` declares both variables as suite options, the same mechanism `parquet_row_filter_skip` and `predicate_eval` use. `benchmark_runner` therefore also shows them in `--help` and `--dry-run` and accepts `--limit-repeated` and `--limit-distinct` on the command line, while `cargo bench --bench sql` and `bench.sh` read the environment variables. Two notes for whoever raises a limit, both now in the suite help and in `sql_benchmarks/README.md`: - The query must still spill. On this dataset `q05` stops spilling above about `160M`, so it measures nothing at `256M`. A comparison that must keep spilling should raise the limit to about `128M`. - Give the limit as a whole number of `K`, `M` or `G` units. The suite asserts the limit that the memory pool reports back, and the pool reports `1024M` as `1G`. The defaults do not change. The failure is not monotonic in the limit, so a larger default is not obviously safer. ## Peak pool memory stays unavailable for this suite The bot's "Memory Pool Peaks" table needs the benchmark process to hold a `PeakRecordingPool`. `CommonOpt::runtime_env_builder` in `benchmarks/src/util/options.rs` installs one only when `--memory-limit` or `DATAFUSION_RUNTIME_MEMORY_LIMIT` gives the harness a limit. This suite sets its limit with SQL `SET datafusion.runtime.memory_limit` instead. `SessionContext::set_runtime_variable` builds a new `RuntimeEnv` through `RuntimeEnvBuilder::with_memory_limit`, which installs a fresh `TrackConsumersPool<GreedyMemoryPool>` and replaces the runtime env on the session. The harness pool, and its `PeakRecordingPool` wrapper, are gone after that, so `print_memory_stats` finds no recorder and prints nothing. For the same reason `--mem-pool-type` has no effect on this suite. Moving the suite to the harness-level pool is not a small change, so this PR leaves it: - `DATAFUSION_RUNTIME_MEMORY_LIMIT` holds one value for the whole process, while the two subgroups need different limits. A per-benchmark limit needs a new benchmark-file directive and harness plumbing into `make_ctx`. - The harness pool covers the whole session. This suite loads its data without a limit on purpose: `init/settings.sql` runs after `load`, so the `COPY` that writes the 1M-row Parquet file stays outside the limit. A harness-level limit would cover the load too. ## What is the testing strategy for this PR? The change touches only benchmark definition files, so it has no unit tests of its own. The suite metadata is parsed by the existing `datafusion-benchmarks` tests, and each run checks its own limit: the template asserts the value of `datafusion.runtime.memory_limit` that `information_schema.df_settings` reports, so a limit that does not reach the session fails the run. Checks run locally, on macOS, with a release `benchmark_runner`: **Defaults behave as before.** `benchmark_runner spill_views -i 3` runs all five queries and passes every assert. **The assert has teeth.** `SPILL_VIEWS_LIMIT_DISTINCT=1024M` fails with `expected value "1024M" but got value "1G"`, which is the pool reporting back the limit the override installed. **An override reaches the session and the query still spills.** With `SPILL_VIEWS_LIMIT_DISTINCT=256M` the suite passes and `q05` drops from about 1.7 s to about 0.6 s. `EXPLAIN ANALYZE` on the suite's own `distinct.parquet` with `target_partitions = 4` gives, for `q05`: | Limit | `spill_count` per partition | | ------ | --------------------------- | | `96M` | 9, 4, 0 | | `128M` | 8, 0, 0 | | `160M` | 0, 4, 0 | | `192M` | 0, 0, 0 | | `256M` | 0, 0, 0 | `q04` still spills at every one of those limits (4 at `96M`, 3 at `128M`, 2 at `256M`). This is why the docs say to raise the limit to about `128M`, not `256M`. **Both entry points read the variable.** `benchmark_runner --dry-run` reports the value with source `default`, `environment` or `command_line`. The `cargo bench --bench sql` path that `bench.sh` uses has no suite-option plumbing and reads the variable through the `${NAME:-default}` fallback in the benchmark file; it was checked with the same `1024M` negative test and with a `256M` run. **The repeated subgroup works the same way**, checked with `SPILL_VIEWS_LIMIT_REPEATED=64M` and the same `1024M` negative test. Also run: `cargo fmt --all` (no Rust changes), `cargo clippy -p datafusion-benchmarks --all-targets --features datafusion/parquet_encryption -- -D warnings` (clean), `cargo test -p datafusion-benchmarks --lib --bins` (204 tests pass) and `./ci/scripts/doc_prettier_check.sh`. ## Are there any user-facing changes? No. The change is limited to the benchmark suite, and the defaults keep today's behaviour. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
