Use conservative column-size estimates in cudf-polars - #22796
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:
📝 WalkthroughWalkthroughThis PR adds a dtype-aware decoded-size floor helper, updates ParquetSourceInfo sizing to use per-column dtype floors and row-group samples, threads Parquet schema through cached source-info construction, accumulates schema in statistics grouping, and adds a unit test validating the floor application. ChangesDtype-aware Parquet sizing and schema threading
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@python/cudf_polars/cudf_polars/streaming/io.py`:
- Around line 820-847: The _decoded_size_floor function omits several
fixed-width TypeIds so TIMESTAMP_SECONDS, DURATION_DAYS, DURATION_SECONDS,
DECIMAL32, and DECIMAL64 fall through to max(1, nrows) and under-estimate sizes;
update _decoded_size_floor to add explicit branches for these plc.TypeId values
and return nrows * <bytes> + nullmask using the same byte widths convention as
the surrounding cases (TIMESTAMP_SECONDS -> 8, DURATION_DAYS -> 8,
DURATION_SECONDS -> 8, DECIMAL32 -> 4, DECIMAL64 -> 8) so ParquetSourceInfo
per_file_means/file_size calculations used by scan_partition_plan are correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: de2531a5-7463-42f4-944f-9fac6e54d133
📒 Files selected for processing (3)
python/cudf_polars/cudf_polars/streaming/io.pypython/cudf_polars/cudf_polars/streaming/statistics.pypython/cudf_polars/tests/streaming/test_stats.py
Matt711
left a comment
There was a problem hiding this comment.
LGTM. This doesn't seem like it would negatively impact out nightly TPC-H/DS benchmarks. Probably worth keeping an eye on them though once this merges
This is actually an important consideration. I just started collecting TPC-H numbers on H100s, and this PR does affect benchmark performance in a way that is probably not surprising: Since some queries now use more partitions (because we were previously under-estimating the in-memory size of some columns), performance with the default |
Simply use the estimated chunk size (already computed) for the memory reservation. I ran TPC-H locally (single-gpu, sf1k) and didn't notice any notable regressions. xref #22796 Authors: - Matthew Murray (https://github.com/Matt711) Approvers: - Mads R. B. Kristensen (https://github.com/madsbk) URL: #22846
Simply use the estimated chunk size (already computed) for the memory reservation. I ran TPC-H locally (single-gpu, sf1k) and didn't notice any notable regressions. xref NVIDIA#22796 Authors: - Matthew Murray (https://github.com/Matt711) Approvers: - Mads R. B. Kristensen (https://github.com/madsbk) URL: NVIDIA#22846
|
/merge |
Description
While experimenting with some real user data, I discovered that our existing logic for detecting "suspicious" column-size estimates for Parquet data is sometimes insufficient. More specifically, I was running into OOM errors as a result of Scan chunks that were multiple times larger than
target_partition_sizewould suggest.This PR sets a more-conservative floor on the column-size estimates we use for Scan partition planning.
Checklist