Parquet Variant Extract Benchmark - #23616
Conversation
github-actions
Bot
added
libcudf
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change adds a CUDA NVBench target for Parquet VARIANT casting and field extraction. It generates configurable VARIANT inputs and adds tests for one-byte and two-byte metadata dictionary offsets, including the 255/256-byte boundary. ChangesVARIANT benchmark
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: ⚪ Minimal · up to This PR adds localized Parquet Variant microbenchmarks and benchmark registration; no actionable merge-blocking risk remains beyond normal checks and review. Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@cpp/benchmarks/io/parquet/experimental/variant/extract.cpp`:
- Around line 19-22: Add the direct declaration headers <cstdint>,
<cuda_runtime_api.h>, and <tuple> to the includes in extract.cpp for its
fixed-width integer types, cudaStreamSynchronize, and std::ignore usage.
🪄 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: Enterprise
Run ID: c1a8c46b-67f1-4a01-bbe5-91ef1b75588e
📒 Files selected for processing (2)
cpp/benchmarks/CMakeLists.txtcpp/benchmarks/io/parquet/experimental/variant/extract.cpp
vuule
left a comment
vuule
left a comment
There was a problem hiding this comment.
Primarily a request to expand the coverage, as , for example, this benchmark would not capture the performance improvement from the binary searches.
| for (cudf::size_type i = 0; i < num_rows; ++i) { | ||
| val_rows[i] = (static_cast<int>(i % 100) < hit_rate) ? hit_val : miss_val; | ||
| } |
There was a problem hiding this comment.
I would prefer this to be a random chance instead of contiguous ranges of rows with the same hit/miss value.
There was a problem hiding this comment.
Yes, I think the function fill_val_rows is now randomized.
| }); | ||
| } | ||
|
|
||
| NVBENCH_BENCH(bench_variant_extract) |
There was a problem hiding this comment.
Can we also vary the number of fields? With that, we can vary which field we extract (first/last).
With this knob, we have too many parameters to vary in a single benchmark, so we should split into multiple benchmarks.
See benchmarks in #22434, they were useful for the initial implementation.
There was a problem hiding this comment.
I have refactored this code to include a new benchmark that will showcase the benefits of the binary search optimizations.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/benchmarks/io/parquet/experimental/variant/extract.cpp (1)
36-49: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse a wider dictionary-offset encoding for the 100-field case.
build_metadatastores cumulative key offsets inuint8_t. Thenum_fields=100case creates 101 dictionary keys and 300 bytes off00throughf99key data.runningwraps after 255 bytes, so the metadata offsets are invalid.Encode offsets with a VARIANT-supported width that holds the full dictionary, or reduce the field-count axis. Add a regression test that extracts
f99from the 100-field input. As per coding guidelines,**/*requires: “Add unit tests and unit benchmarks.”🤖 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 `@cpp/benchmarks/io/parquet/experimental/variant/extract.cpp` around lines 36 - 49, Update build_metadata to encode cumulative dictionary offsets using a VARIANT-supported width that can represent the complete 100-field dictionary without wrapping, and adjust the metadata layout consistently. Add regression coverage for extracting f99 from the 100-field input, including the required unit test and benchmark coverage.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@cpp/benchmarks/io/parquet/experimental/variant/extract.cpp`:
- Around line 36-49: Update build_metadata to encode cumulative dictionary
offsets using a VARIANT-supported width that can represent the complete
100-field dictionary without wrapping, and adjust the metadata layout
consistently. Add regression coverage for extracting f99 from the 100-field
input, including the required unit test and benchmark coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 883d4e53-3dd1-4711-800c-a5b568e203c5
📒 Files selected for processing (1)
cpp/benchmarks/io/parquet/experimental/variant/extract.cpp
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 `@cpp/tests/io/experimental/variant_extract_test.cpp`:
- Around line 783-820: Add boundary-size tests alongside
LargeDictionary100FieldsExtractLast that construct dictionary metadata totaling
exactly 255 bytes and exactly 256 bytes, then extract a representative field and
verify the expected value. Ensure the cases exercise both sides of the
build_metadata total > 255u threshold and preserve the existing coverage for
wider offsets.
🪄 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: Enterprise
Run ID: 1d01bcbf-dd66-4c57-a4df-180fd3204679
📒 Files selected for processing (2)
cpp/benchmarks/io/parquet/experimental/variant/extract.cppcpp/tests/io/experimental/variant_extract_test.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/benchmarks/io/parquet/experimental/variant/extract.cpp
| std::vector<uint8_t> build_leaf_value(std::string const& type_str) | ||
| { | ||
| if (type_str == "int32_t") { | ||
| std::vector<uint8_t> out{0x14}; |
There was a problem hiding this comment.
I thought we defined an enum somewhere in the tests to reduce the magic numbers. Does that need to be redefined here? There are a lot of magic values (some with comments) floating around. I would rather have named values like we settled on in the tests, iirc.
| std::vector<std::vector<uint8_t>> const& meta_rows, | ||
| std::vector<std::vector<uint8_t>> const& val_rows, |
There was a problem hiding this comment.
Can we take these as a span-of-spans? Like
| std::vector<std::vector<uint8_t>> const& meta_rows, | |
| std::vector<std::vector<uint8_t>> const& val_rows, | |
| std::span<std::span<uint8_t const>> meta_rows, | |
| std::span<std::span<uint8_t const>> val_rows, |
There are a few other places where we can do this, too.
|
|
||
| cudf::data_type get_target_type(std::string const& type_str) | ||
| { | ||
| if (type_str == "float") return cudf::data_type{cudf::type_id::FLOAT32}; |
There was a problem hiding this comment.
Let's use a switch with a default case that throws.
| std::vector<uint8_t> offs{0x00}; | ||
| uint8_t running = 0; | ||
| int const offset_size = (total > 255u) ? 2 : 1; | ||
| std::vector<uint8_t> out{static_cast<uint8_t>(0x01 | ((offset_size - 1) << 6))}; |
There was a problem hiding this comment.
I have no idea what this line means. None of the names or numbers are descriptive. 😕
| constexpr uint8_t bool_true_byte = 0x04; | ||
|
|
||
| std::vector<uint8_t> val{make_variant_object_header(), static_cast<uint8_t>(n_fields)}; | ||
| for (int i = 0; i < n_fields; ++i) |
There was a problem hiding this comment.
We always use braces { ... } even for one-line bodies, for readability and to ensure correctness if the body changes in the future. I'm surprised clang-format let you do this. Maybe we're missing a setting, or excluding test code?
3 participants
Description
This PR adds a microbenchmark (VARIANT_NVBENCH) exercising
cudf::io::parquet::experimental::extract_variant_fieldandcast_variant. It contains 3 different benchmarks, which are described below:bench_variant_cast: runscast_variant(only leaf primitives, no path traversal), with axesnum_rows(32k, 256k, 2m rows), type (int32_t / float / bool / string), hit rate (20,80)bench_variant_extract_nesting: evaluatesextract_variant_fieldwith varying path depth, with axesnum_rows(32k, 256k, 2m rows), type (int32_t / float / bool / string / array), nesting (1 / 5), hit rate (20,80)bench_variant_extract_fields: evaluatesextract_variant_fieldon a flat object with varying field count and target position, with axesnum_rows(32k, 256k, 2m rows),num_fields(1, 10, 100), field position (first or last), hit rate (20,80)This solves one of the issues in story issue #22312.
Checklist