Add parquet benchmark for output_dict_columns option - #23596
Conversation
github-actions
Bot
added
libcudf
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 7 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds an ChangesParquet dictionary-output benchmarks
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR only updates Parquet benchmark support in two benchmark files; no actionable merge-blocking risk remains, so it is merge-ready after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
vuule
left a comment
vuule
left a comment
There was a problem hiding this comment.
Could we add this to parquet_reader_options.cpp and follow the existing benchmark patterns there? Since this measures output_dict_columns, keeping it with the other reader-options benchmarks would make it easier to find and maintain.
Could we also make the PR title more specific, for example: “Add benchmark for Parquet output_dict_columns”?
vuule
added
tests
y2kiran
changed the title
6b75627 to
6edb425
Compare
|
@vuule Apologies for the title. Apparently I missed it and it just used the name of the commit. I've incorporated this into the existing parquet_reader_options benchmark. Thanks for the input! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cpp/benchmarks/io/parquet/parquet_reader_options.cpp`:
- Around line 206-222: The BM_parquet_read_options benchmark currently validates
only column count, so add a non-timed preflight that inspects an eligible flat
STRING column and verifies its type is DICTIONARY32 when output_dict is YES and
STRING when output_dict is NO. Keep the timing path unchanged and use the
existing benchmark setup and output_dict axis symbols.
- Around line 221-222: Update the row_group_size_rows axis in the benchmark
options to use 100'000 instead of 1'000'000, preserving 0 while ensuring the
configured values represent distinct row-group limits.
🪄 Autofix
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: Pro Plus
Run ID: 5e4f95eb-a035-46c1-a993-59aa00e24692
📒 Files selected for processing (2)
cpp/benchmarks/io/nvbench_helpers.hppcpp/benchmarks/io/parquet/parquet_reader_options.cpp
6edb425 to
a40db45
Compare
| // Non-timed preflight: confirm the reader honors `output_dict_columns` on a flat STRING column -- | ||
| // YES transcodes it to DICTIONARY32, NO leaves it as STRING. Skipped when | ||
| // `convert_strings_to_categories` is set. | ||
| if constexpr (not str_to_categories) { | ||
| auto const preflight_tbl = cudf::io::read_parquet(read_options).tbl; | ||
| auto const preflight = preflight_tbl->view(); | ||
| auto const has_type = [&](cudf::type_id id) { | ||
| return std::any_of(preflight.begin(), preflight.end(), [id](auto const& col) { | ||
| return col.type().id() == id; | ||
| }); | ||
| }; | ||
| if constexpr (output_dict_columns) { | ||
| CUDF_EXPECTS(has_type(cudf::type_id::DICTIONARY32), | ||
| "output_dict_columns=YES must produce a DICTIONARY32 column"); | ||
| } else { | ||
| CUDF_EXPECTS(has_type(cudf::type_id::STRING), | ||
| "output_dict_columns=NO must produce a STRING column"); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't think we need this check. Other options don't check if they are applied correctly.
There was a problem hiding this comment.
Fixed in the latest commit.
Apologies for the force push. As you can see from the coderabbit comments below - I messed up a rebase/pull on main. The state was really messed up, and I thought the best option was to reset to a clean state.
My bad. Won't happen again.
github-actions
Bot
added
Python
9f6186a to
3483447
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
3483447 to
c610a67
Compare
|
@y2kiran Can you please run this benchmark and post some base numbers as a comment here for posterity |
| // 0 == cuDF default (1,000,000 rows/RG → few, large row groups); 100,000 forces ~10x more, | ||
| // smaller row groups, exercising the multi-row-group concatenate path. |
There was a problem hiding this comment.
Can we explicitly enumerate the row group sizes we want to benchmark rather than relying on defaults that live a long way from here?
There was a problem hiding this comment.
Done. Added in explicit row group sizes of 1K , 10K , 100K, 1M.
Thanks!
347a997 to
e0b4fa8
Compare
|
Here are results of the benchmark on a RTX 6000 Blackwell. As expected, there is a regression when the row groups are small, and the number of row groups is very large. But it will be fixed in a follow up MR. @mhaseeb123
|
b879ed6 to
a81cb89
Compare
|
/ok to test a81cb89 |
a81cb89 to
97d60d0
Compare
97d60d0 to
721fac5
Compare
|
/merge |
4 participants
Description
This PR adds benchmarks to test the newly added
output_dict_columnsoptions for the Parquet reader, which was introduced in this PRChecklist