Parquet Variant Extract Benchmark - #23616
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: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 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 The PR adds a localized benchmark and has only a minor source-header cleanup concern; no actionable merge-blocking risk remains beyond normal review and build checks. 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> 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. 😕
There was a problem hiding this comment.
I have changed a lot of the names & magic numbers, I hope this code is more understandable!
| 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?
There was a problem hiding this comment.
I did clang-format the code, but now have checked specifically for this phenomenon
|
/ok to test a4a49b7 |
vuule
left a comment
vuule
left a comment
There was a problem hiding this comment.
few nits, nothing blocking
| } | ||
|
|
||
| // Header byte for a primitive value of the given physical type. | ||
| constexpr uint8_t make_variant_primitive(variant_primitive_type type) |
There was a problem hiding this comment.
to be consistent with other names
| constexpr uint8_t make_variant_primitive(variant_primitive_type type) | |
| constexpr uint8_t make_variant_primitive_header(variant_primitive_type type) |
| // Header byte for a short string of the given length (must fit in 6 bits: 0..63). | ||
| constexpr uint8_t make_variant_short_string_header(std::size_t length) | ||
| { | ||
| return make_variant_header(variant_basic_type::SHORT_STRING, static_cast<uint8_t>(length)); |
There was a problem hiding this comment.
should we check the length here?
it would also help justify this helper function
| return make_variant_header(variant_basic_type::ARRAY, 0); | ||
| } | ||
|
|
||
| void append_le(std::vector<uint8_t>& out, uint64_t bits, int width) |
There was a problem hiding this comment.
this function could use a comment to help clarify
Co-authored-by: Vukasin Milovanovic <vmilovanovic@nvidia.com>
Co-authored-by: Vukasin Milovanovic <vmilovanovic@nvidia.com>
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