adriangb opened a new pull request, #24093: URL: https://github.com/apache/datafusion/pull/24093
## Which issue does this PR close? - Closes #. ## Rationale for this change `--simulate-latency` wraps the local filesystem in `LatencyObjectStore` so benchmarks can be run against S3-like IO without a network. It is not a faithful model of one, in three ways that matter. **It overrides `get_ranges`.** Neither `AmazonS3` nor `GoogleCloudStorage` implements that method; they inherit the `ObjectStore` default, which merges ranges less than `OBJECT_STORE_COALESCE_DEFAULT` (1MiB) apart and issues the merged chunks as up to 10 *concurrent* GETs. `LatencyObjectStore` overrides it, sleeps once, and delegates to `LocalFileSystem::get_ranges` — so it charges one round trip where S3 pays one per coalesced chunk, and it inherits local positional reads with no coalescing at all. How many chunks a vectored read produces is data dependent (column layout, projection, gap distances), which is exactly what an IO optimisation changes, so the simulation is blind to the thing being measured. **It has no per-byte cost.** A 4 byte footer read and a 100MB column chunk read cost the same. Coalescing across a gap trades bytes for round trips; without a bytes term the simulation says that trade is always free. **It charges one latency for an entire LIST stream.** Both S3 and GCS cap a listing response at 1000 keys, and each page needs the continuation token from the one before it, so pages are serial round trips. Listing 5000 files is five round trips, not one. ## What changes are included in this PR? Replaces `LatencyObjectStore` with `SimulatedObjectStore`, which presents a local filesystem the way a remote store presents itself. The organising principle is that it implements **only** the HTTP-shaped primitives a real remote store implements, and leaves `get_ranges`, `list_with_offset` and `rename_opts` to the `ObjectStore` defaults — exactly as `AmazonS3` and `GoogleCloudStorage` leave them. Everything arrow-rs does above the wire (coalescing, fan-out, pagination) then runs against the simulator unmodified. This is deliberately the opposite of the advice in `ObjectStore`'s "Wrappers" doc section, which tells wrappers to implement every method so they do not lose the wrapped store's overrides. That advice is for observability wrappers; here the wrapped store's overrides are the problem. The module documents this, and there is a comment against adding `#[deny(clippy::missing_trait_methods)]`. Each simulated request pays for: 1. a connection from a bounded pool (`max_concurrent_requests`, default 128), 2. time to first byte, drawn from the same S3-shaped distribution as before, 3. transfer time at `connection_bytes_per_second` (default 100MB/s), charged as the body is consumed. A `HEAD` is charged (1) only — its `GetResult` reports a range spanning the whole object even though no bytes move, and charging that as a transfer made every metadata probe cost as much as downloading the file. Bounded reads take a single blocking read; reads above `max_eager_read_bytes` stream, so a whole-object GET of a multi-GB CSV is not buffered in memory. The latency sampler now hashes its draw counter rather than round-robining. A 20-entry table cycled against `coalesce_ranges`' fixed waves of 10 meant every vectored read saw the same two fixed sets of latencies, and a read issuing exactly 10 or 20 requests always landed on the table mean. ### What is deliberately not modelled arrow-rs never splits one large range into several smaller concurrent requests — `merge_ranges` has no upper bound on merged size and no store chunks a large GET — so neither does this. A 200MB coalesced range is one GET on one connection, and is bandwidth bound. Retries, multipart part uploads and per-prefix throttling are also not modelled. ## Are these changes tested? Yes — 10 new tests. The ones carrying the argument above: - `distant_ranges_become_separate_requests` / `nearby_ranges_are_coalesced_into_one_request` — ranges more than 1MiB apart reach the wire as separate requests; nearer ones are merged into one. The old store reported one in both cases. - `projecting_more_columns_costs_more_requests` — end to end through a real DataFusion Parquet scan: two distant column chunks requested in a single `get_byte_ranges` call cost more requests than one. The old store could not express this difference. - `head_requests_do_not_pay_for_a_body`, `transfer_time_scales_with_bytes`, `list_is_charged_per_page`. `cargo test -p datafusion-benchmarks` passes (133 tests). `cargo clippy -p datafusion-benchmarks --all-targets --no-deps -- -D warnings` is clean. ### Benchmark results TPC-H SF1 parquet, 3 iterations, all 22 queries, run back to back on the same machine. "old sim" is `LatencyObjectStore` at the parent commit; "new sim" is this PR. Both with `--simulate-latency`. | Metric | Value | |---|---| | Total, no simulation | 5.33 s | | Total, old sim | 22.55 s | | Total, new sim | 26.75 s | | new / old | **1.19x** | The total is not the interesting number. The spread is: per query, new/old ranges from **0.90x to 2.04x**, because the new model differentiates queries that the old one charged identically. | Query | no sim | old sim | new sim | new/old | |---|---|---|---|---| | q22 | 23 ms | 529 ms | 1076 ms | 2.04x | | q21 | 153 ms | 2344 ms | 3850 ms | 1.64x | | q12 | 94 ms | 831 ms | 1160 ms | 1.40x | | q20 | 133 ms | 1016 ms | 1392 ms | 1.37x | | q4 | 46 ms | 812 ms | 1101 ms | 1.36x | | … | | | | | | q15 | 99 ms | 1267 ms | 1146 ms | 0.90x | Queries whose vectored reads fan out into many distant chunks (q22, q21) get materially more expensive; the small decreases at the bottom of the table are within run-to-run variance and I would not read anything into them. Relative query ranking changes between the two models, which is the point: an IO change evaluated under the old model was being scored against a cost function that could not see request count. Numbers from previous `--simulate-latency` runs are **not** comparable across this change. ## Are there any user-facing changes? No public API changes — this is benchmark-only. `--simulate-latency` and `SIMULATE_LATENCY` keep their names and meaning; their help text and `bench.sh` documentation are updated to describe what is now modelled. 🤖 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]
