Add a I/O partition planning information to benchmark runner - #22945
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds partition-plan collection and formatting for streaming queries, exposes the data through benchmark execution, appends IR annotations for streaming scans, and caps Parquet fusion factor by available paths. ChangesPartition Plan Summary Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/cudf_polars/tests/streaming/test_explain.py (1)
703-743: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider adding edge case tests for partition plan collection.
The current tests cover the main FUSED_FILES and SPLIT_FILES scenarios well, but miss important edge cases:
- Empty DataFrame input
- All-null columns (affects
projected_bytescalculation)- Zero files case
These edge cases could reveal bugs in byte calculations, partition logic, or division-by-zero scenarios in the factor arithmetic.
Consider adding tests like:
def test_collect_partition_plan_empty_dataframe(tmp_path): """Empty DataFrame should handle gracefully.""" empty_df = pl.DataFrame({"x": []}) empty_df.write_parquet(tmp_path / "empty.parquet") q = pl.scan_parquet(tmp_path / "empty.parquet") engine = pl.GPUEngine(executor="streaming", raise_on_fail=True) rows = collect_partition_plan(q, engine, q_id=1) # Assert expected behavior for empty input🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cudf_polars/tests/streaming/test_explain.py` around lines 703 - 743, Add three new test functions to cover edge cases in partition plan collection: test_collect_partition_plan_empty_dataframe to handle empty DataFrames, test_collect_partition_plan_all_null_columns to test all-null column scenarios that may affect projected_bytes calculations, and test_collect_partition_plan_zero_files for the zero files case. Each test should follow the same structure as the existing test_collect_partition_plan_fused and test_collect_partition_plan_split functions, using make_partitioned_source to set up the data and collect_partition_plan to verify the expected behavior without errors or division-by-zero issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@python/cudf_polars/tests/streaming/test_explain.py`:
- Around line 703-743: Add three new test functions to cover edge cases in
partition plan collection: test_collect_partition_plan_empty_dataframe to handle
empty DataFrames, test_collect_partition_plan_all_null_columns to test all-null
column scenarios that may affect projected_bytes calculations, and
test_collect_partition_plan_zero_files for the zero files case. Each test should
follow the same structure as the existing test_collect_partition_plan_fused and
test_collect_partition_plan_split functions, using make_partitioned_source to
set up the data and collect_partition_plan to verify the expected behavior
without errors or division-by-zero issues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4e77b53a-8220-48cc-9cc3-47dc9986b967
📒 Files selected for processing (4)
python/cudf_polars/cudf_polars/streaming/benchmarks/utils.pypython/cudf_polars/cudf_polars/streaming/explain.pypython/cudf_polars/cudf_polars/streaming/io.pypython/cudf_polars/tests/streaming/test_explain.py
vyasr
left a comment
There was a problem hiding this comment.
Was there a particular request that this PR was created for, or just something you thought would be useful?
|
/merge |
Yes I'm currently studying how well our split / fused task scans are aligned with the target partition size. I think we're currently being too conservative. I'll have a PR up soon. |
Table: `--explain-partition-plan`
```
Partition Plan Summary
+----+----------+--------+--------------+-------+------------------------+-----------+------------+
| Q | Table | Flavor | Factor | Files | Projected (bytes/file) | Size/task | Partitions |
+----+----------+--------+--------------+-------+------------------------+-----------+------------+
| 1 | lineitem | SPLIT | 3 tasks/file | 60 | 4.35 GB | 1.45 GB | 180 |
| 5 | region | FUSED | 1 file/task | 1 | 114 B | 114 B | 1 |
| | nation | FUSED | 1 file/task | 1 | 329 B | 329 B | 1 |
| | customer | FUSED | 1 file/task | 2 | 900 MB | 900 MB | 2 |
| | orders | FUSED | 1 file/task | 15 | 1.38 GB | 1.38 GB | 15 |
| | lineitem | SPLIT | 2 tasks/file | 60 | 2.71 GB | 1.36 GB | 120 |
| | supplier | FUSED | 1 file/task | 1 | 120 MB | 120 MB | 1 |
| 18 | orders | SPLIT | 2 tasks/file | 15 | 2.18 GB | 1.09 GB | 30 |
| | lineitem | SPLIT | 2 tasks/file | 60 | 1.6 GB | 800.1 MB | 120 |
| | customer | SPLIT | 2 tasks/file | 2 | 1.95 GB | 976.54 MB | 4 |
| 21 | lineitem | FUSED | 1 file/task | 60 | 1.11 GB | 1.11 GB | 60 |
| | lineitem | FUSED | 1 file/task | 60 | 1.42 GB | 1.42 GB | 60 |
| | supplier | FUSED | 1 file/task | 1 | 300.41 MB | 300.41 MB | 1 |
| | nation | FUSED | 1 file/task | 1 | 259 B | 259 B | 1 |
| | orders | FUSED | 1 file/task | 15 | 1.3 GB | 1.3 GB | 15 |
+----+----------+--------+--------------+-------+------------------------+-----------+------------+
```
Explain output: `CUDF_POLARS__EXPLAIN__PARTITION_PLAN=1 and --explain`
```
SORT ('l_returnflag', 'l_linestatus') ('l_returnflag', 'l_linestatus', 'sum_qty', '...', 'avg_disc', 'count_order') [180]
SELECT ('l_returnflag', 'l_linestatus', 'sum_qty', '...', 'avg_disc', 'count_order') [180]
GROUPBY ('l_returnflag', 'l_linestatus') ('l_returnflag', 'l_linestatus', 'sum_qty', '...', 'avg_disc', '______________11') [180]
HSTACK ('l_returnflag', 'l_linestatus', 'l_quantity', '...', 'l_tax', '__POLARS_CSER_0x6a4988e5b78df9ee') [180]
PROJECTION ('l_returnflag', 'l_linestatus', 'l_quantity', 'l_extendedprice', 'l_discount', 'l_tax') [180]
STREAMINGSCAN ('l_returnflag', 'l_linestatus', 'l_quantity', '...', 'l_tax', 'l_shipdate') [flavor=SPLIT_FILES factor=3 projected=4.35 GB] [180]
```
Authors:
- Matthew Murray (https://github.com/Matt711)
Approvers:
- Vyas Ramasubramani (https://github.com/vyasr)
- Mads R. B. Kristensen (https://github.com/madsbk)
URL: #22945
…23011) For parquet scans, we use the `target_partition_size`(say `T`) and the total uncompressed size in bytes (say `S`) to decide whether to split a large file into smaller reads (ie. `SplitScan`s) or fuse together smaller files into larger reads (ie. `FusedScans`). If `S < T`, then `k`, the number of splits is `k = ceil(S / T)`. This means we always round up the number of splits. And so we run into the situation where we split a file into two parts when the entire file size was closer to `T`. In that situation, one task should read the entire file rather than splitting. Similarly for two and three splits, four and five splits, and so on. What we should do instead is decide whether to split into `k` or `k+1` parts by how close the projected number of bytes is to `T`. And choose which ever projection is closer. If they tie, choose `k`. For `FusedScan`s, we should compare the projected number of bytes read for a fused task using `k`, the number of files fused, and `k+1`. And similarly choose whichever projection is closer to `T`. For ties, prefer `k+1`. Overall, the idea is to get each "task" as close to reading a `target_partition_size` worth of bytes as possible. ## Benchmarks On 1xB200, ```text Benchmark Branch Avg lukewarm (s) Avg hot (s) TPC-DS main 448.385 437.692 this PR 395.437 386.210 TPC-H main 131.884 128.187 this PR 119.677 118.426 ``` Around a 10% improvement for H/DS benchmarks at SF1K. Follow up from #22945 Authors: - Matthew Murray (https://github.com/Matt711) Approvers: - Mads R. B. Kristensen (https://github.com/madsbk) - Vyas Ramasubramani (https://github.com/vyasr) URL: #23011
Description
Table:
--explain-partition-planExplain output:
CUDF_POLARS__EXPLAIN__PARTITION_PLAN=1 and --explainChecklist