Improve scan partitioning planning to prefer sizes closer to target - #23011
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test 2f8bff8 |
📝 WalkthroughWalkthrough
ChangesNearest-blocksize partition plan heuristic
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| @@ -89,21 +89,35 @@ def scan_partition_plan( | |||
| ] | |||
| if (file_size := sum(column_sizes)) > 0: | |||
| if file_size > blocksize: | |||
There was a problem hiding this comment.
| if file_size > blocksize: | |
| # Split large files into parts or fuse small files together, picking | |
| # whichever factor keeps each chunk closest to the target size. | |
| if file_size > blocksize: |
There was a problem hiding this comment.
Will tack this on elsewhere
|
Blocking until we can determine whether this improvement is effectively equivalent to just raising the target size. |
Agree. To be concrete: Can you maybe collect results with the |
vyasr
left a comment
There was a problem hiding this comment.
We had a call and basically concluded that the benchmarks we wanted are basically orthogonal to this PR. This PR is primarily changing how the target_partition_size parameter works, but we still have a tunable parameter, so the change just shifts the parameter space. If this is a better representation of what the parameter should mean, it makes sense to merge the PR regardless, then follow up with more measurements of how the partition size at different points in the query pipeline is potentially having a differential impact in case we should be tuning them differently.
vyasr
left a comment
There was a problem hiding this comment.
We had a call and basically concluded that the benchmarks we wanted are basically orthogonal to this PR. This PR is primarily changing how the target_partition_size parameter works, but we still have a tunable parameter, so the change just shifts the parameter space. If this is a better representation of what the parameter should mean, it makes sense to merge the PR regardless, then follow up with more measurements of how the partition size at different points in the query pipeline is potentially having a differential impact in case we should be tuning them differently.
|
/merge |
Description
For parquet scans, we use the
target_partition_size(sayT) and the total uncompressed size in bytes (sayS) to decide whether to split a large file into smaller reads (ie.SplitScans) or fuse together smaller files into larger reads (ie.FusedScans). IfS < T, thenk, the number of splits isk = 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 toT. 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 intokork+1parts by how close the projected number of bytes is toT. And choose which ever projection is closer. If they tie, choosek.For
FusedScans, we should compare the projected number of bytes read for a fused task usingk, the number of files fused, andk+1. And similarly choose whichever projection is closer toT. For ties, preferk+1.Overall, the idea is to get each "task" as close to reading a
target_partition_sizeworth of bytes as possible.Benchmarks
On 1xB200,
Around a 10% improvement for H/DS benchmarks at SF1K.
Follow up from #22945
Checklist