adriangb opened a new pull request, #25809: URL: https://github.com/apache/datafusion/pull/25809
## Which issue does this PR close? - No issue. Found while benchmarking https://github.com/apache/datafusion/pull/24086. ## Rationale for this change With `SIMULATE_LATENCY=true`, a benchmark result can change when a PR changes the number of object store requests in *other* queries, or off the critical path of the same query. `LatencyObjectStore` gives each request the next entry of a 20-entry latency table, with one counter for the whole process: ```rust let idx = self.get_counter.fetch_add(1, Ordering::Relaxed) % GET_LATENCIES_MS.len(); ``` So the latency of a request depends on how many requests came before it, in all earlier queries of the run. The simulator is deterministic, so the same wrong result comes back on every rerun. Example from https://github.com/apache/datafusion/pull/24086 (TPC-DS SF1, `SIMULATE_LATENCY=true`): the bot reported Q27 1.55x slower (141 → 218 ms) and Q44 1.38x slower. Both queries have only two serial round trips on the critical path. Q44 makes identical requests with and without the change. Local runs, median of 9, ms: | latency model | Q27 base | Q27 PR | Q44 base | Q44 PR | |---|---|---|---|---| | fixed 50 ms per request | 111 | 111 | 114 | 121 | | fixed 100 ms per request | 216 | 211 | – | – | | current round-robin table | 238 | 288 | 212 | 211 | A sweep over the 20 possible start positions of the counter (100 iterations each) gives Q27 medians of 239 ms (base) and 241 ms (PR). The min-of-5 that the bot reports depends on the start position: the base build gets a lucky minimum at 11 of 20 positions, the PR build at 5. ## What changes are included in this PR? `LatencyObjectStore` draws each GET and LIST latency independently at random from the same 20-entry distributions. The distributions do not change. | approach | latency of a request depends on | result | |---|---|---| | shared counter (before) | the number of earlier requests in the process | a change in one query moves latencies in other queries | | hash of the request (not chosen) | the request's path and byte range | a request shape gets the same latency in every iteration, so builds with different request shapes do not average out | | independent random draw (this PR) | nothing | each request samples the same distribution, and iterations average out | ## What is the testing strategy for this PR? This change affects only the benchmark harness. - With this change, Q27 over 99 iterations gives base median 217 ms and PR median 224 ms (mean 223.5 ± 7.5 vs 217.6 ± 6.9), so there is no difference, as with a fixed latency. - Queries that do read data keep their differences: TPC-DS Q3 703 → 353 ms, Q7 1078 → 526 ms, TPC-H Q1 1006 → 209 ms, Q6 1862 → 218 ms (median of 5, same two builds). - `cargo test -p datafusion-benchmarks --lib` and clippy pass. There is no new unit test: the property ("no dependency on earlier requests") is structural, since the store has no state left. ## Are there any user-facing changes? No. Benchmark runs with `SIMULATE_LATENCY=true` are no longer bit-for-bit reproducible between runs. Queries with few serial round trips have more spread per iteration (Q27 SD about 70 ms), so compare them with more iterations or with means. 🤖 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]
