Add column projection resolution benchmarks for the Parquet readers - #24193
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. |
94ee62d to
eb6a7b2
Compare
Benchmark resultsMachine: NVIDIA GH200 480GB (132-SM Hopper, Neoverse-V2 CPU), driver 580.95.05, CUDA 13.3. All runs with
Raw nvbench output — PR branch, |
eb6a7b2 to
f0d6376
Compare
… produced (#24191) Split out of #24162. This PR is the shared groundwork that #24192 and #24193 build on. It also sets `max_page_fragment_size` and raises `max_page_size_bytes` to ensure benchmark `parquet_read_file_shape` can actually generate requested Parquet shape. Authors: - Qi Chen (https://github.com/qbacpey) Approvers: - Vukasin Milovanovic (https://github.com/vuule) - Kyle Edwards (https://github.com/KyleFromNVIDIA) URL: #24191
f0d6376 to
a23494a
Compare
Moves the writer inlined in BM_parquet_filter_name_resolution into write_named_resolution_parquet_file so the projection benchmarks can resolve against the same deterministically named schema.
hybrid_scan_projection times the filter and payload column chunk byte range calls across a side axis; parquet_read_column_projection is the naive reader baseline, timing chunked reader construction with a full explicit projection.
Introduces the `named_resolution_column_names` function to generate deterministic column names for Parquet files. This function is utilized in `write_named_resolution_parquet_file` and various benchmarks to ensure consistent naming across different Parquet operations, enhancing code clarity and maintainability.
a23494a to
bbe33c6
Compare
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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: Repository: NVIDIA/cudf/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughAdds Parquet reader-selection and hybrid-scan projection benchmarks. The reader-selection benchmark covers positional and name-based selection. The filter-name-resolution benchmark now reports schema column count instead of throughput. ChangesParquet Projection Benchmarks
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change adds a registered projection benchmark and changes the filter benchmark’s reported metric. No concrete repository-internal breakage or other actionable merge risk is established. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| using projection_sides = nvbench::enum_type_list<projection_side::FILTER, | ||
| projection_side::PAYLOAD, | ||
| projection_side::PAYLOAD_EXPLICIT>; | ||
|
|
||
| NVBENCH_BENCH_TYPES(BM_hybrid_scan_projection, NVBENCH_TYPE_AXES(projection_sides)) | ||
| .set_name("hybrid_scan_projection") | ||
| .set_type_axes_names({"side"}) | ||
| .set_min_samples(4) | ||
| .add_int64_axis("num_cols", {64, 512, 2048, 4096}); |
There was a problem hiding this comment.
Do we really need enums here. We could just use a string axis.
| { | ||
| auto const num_cols = static_cast<cudf::size_type>(state.get_int64("num_cols")); | ||
|
|
||
| auto source_sink = write_named_resolution_parquet_file(num_cols, io_type::FILEPATH); |
There was a problem hiding this comment.
By default, the parquet writer writes column names as: _col0, _col1, .., ... Can we just use that instead of explicitly writing col0, col1... names? We can also select columns by index as well if needed. We don't need the two new helpers in parquet_common.xpp that way
| // Caller-supplied payload list | ||
| if constexpr (Side == projection_side::PAYLOAD_EXPLICIT) { | ||
| read_opts_builder.column_names(named_resolution_column_names(num_cols)); | ||
| } | ||
| auto const read_opts = read_opts_builder.build(); |
There was a problem hiding this comment.
Is PAYLOAD_EXPLICIT even needed? Isn't the column selection cost just proportional to the number of columns being selected? Also please check if any of the existing parquet benchmarks cover column selection. If so, then PAYLOAD_EXPLICIT as well as PAYLOAD modes are just duplicates and need to be removed
There was a problem hiding this comment.
Removed PAYLOAD_EXPLICIT.
None of the existing benchmarks cover the payload case (a filter and no projection):
- No other benchmark calls
payload_column_chunks_byte_ranges; the hybrid scan benchmarks time the filters or read in a single step throughall_column_chunks_byte_ranges parquet_column_selectionpasses no names, so it times the linear pathparquet_read_column_selectiontimes a full read of a few dozen columns, where selection is negligible
Selection cost is only proportional to the number of columns when no names are passed. With names, select_columns looks up each one with a linear find_if over every schema path, so the cost is roughly selected columns × schema width. With a filter and no projection, select_payload_columns turns every non-filter column into a name and passes all n−1 of them to that lookup, while the regular reader in the same situation passes no names and selects all columns in one pass.
Same build of this branch, GH200:
| num_cols | hybrid payload selection | regular reader, filter, no projection | regular reader, all names projected |
|---|---|---|---|
| 64 | 0.130 ms | 0.079 ms | 0.099 ms |
| 512 | 1.45 ms | 0.72 ms | 1.21 ms |
| 2048 | 10.58 ms | 2.46 ms | 9.08 ms |
| 4096 | 38.52 ms | 5.20 ms | 32.64 ms |
The columns come from hybrid_scan_projection, parquet_filter_name_resolution (case_sensitive=1, heavy_filter=0) and parquet_read_column_projection.
Should we further drop the the payload case (a filter and no projection)?
| cudf::ast::tree filter_tree; | ||
| auto const& col_ref = filter_tree.push(cudf::ast::column_name_reference("col0")); | ||
| auto const& lit = filter_tree.push(cudf::ast::literal(filter_literal)); | ||
| auto const& filter_expr = | ||
| filter_tree.push(cudf::ast::operation(cudf::ast::ast_operator::GREATER_EQUAL, col_ref, lit)); |
There was a problem hiding this comment.
The filter AST tree seems trivial (single predicate) so the filter column selection cost would be trivial. Can we use something like here 4a59ca5#diff-c9f9907616a2121d2c0cd6dd2d50126a42964461acf05132bfe59db0b67b2a21 to build a varying depth/columns AST tree or skip this altogether as filter/payload columns ratio is usually small in real world.
There was a problem hiding this comment.
Agreed, dropped it since parquet_filter_name_resolution sweeps heavy filters for the regular reader.
…ing metadata handling - Removed the `named_resolution_column_names` and `write_named_resolution_parquet_file` functions from `parquet_common.cpp` and `parquet_common.hpp`. - Updated benchmarks in `parquet_reader_metadata.cpp` and `hybrid_scan_projection.cpp` to directly handle column names and metadata without the removed functions. - Adjusted CMake configuration to reflect the changes in benchmark source files. This cleanup enhances code maintainability and reduces unnecessary complexity in the benchmark implementations.
| // Select the filter columns first, as a real read does, so the timed call covers | ||
| // only the payload selection | ||
| std::ignore = reader->filter_column_chunks_byte_ranges(row_groups, read_opts); |
There was a problem hiding this comment.
I think this might not be needed anymore. The hybrid_scan_impl::select_columns() now computes the filter column names (if not already done and filter is present) before deducing payload columns. Please check and update 🙂
|
|
||
| state.exec(nvbench::exec_tag::sync | nvbench::exec_tag::timer, | ||
| [&](nvbench::launch& launch, auto& timer) { | ||
| drop_page_cache_if_enabled(source_info.filepaths()); |
There was a problem hiding this comment.
Don't think this is needed either
| mem_stats_logger.peak_memory_usage(), "peak_memory_usage", "peak_memory_usage"); | ||
| } | ||
|
|
||
| // Benchmark full-projection column-name resolution during naive reader construction. |
There was a problem hiding this comment.
Can we just add an axis (selection method?) to the other benchmark instead where we either provide column selection or not instead of this separate benchmark?
There was a problem hiding this comment.
Done, parquet_column_selection now has a selection_method axis
…mn name selection. Removed unused benchmark for full-projection column-name resolution. Updated benchmark parameters to include selection method options.
…akeLists.txt. This cleanup eliminates unused code and simplifies the benchmark setup.
|
/ok to test 5f4e0f0 |
|
/merge |
Description
Split out of #24162
Adds a new axis for an existed benchmark, both of them are about column-name resolution
Checklist