Add multifile hybrid scan pass construction API - #22794
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:
📝 WalkthroughWalkthroughAdds a multifile public API to compute per-pass, per-source row-group partitions constrained by a memory hint; refactors internal pass construction to accept per-source inputs and return a source map; adds a metadata accessor, updates single-file wrapper/docs, and extends tests. ChangesMultifile row group pass partitioning API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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 `@cpp/src/io/parquet/experimental/hybrid_scan_multifile.cpp`:
- Around line 74-77: Summary: Early return when pass_read_limit == 0 skips
per-source count validation, allowing invalid input in unlimited mode. Fix: move
or replicate the per-source validation logic into the branch handling
pass_read_limit == 0 so that before returning the vector built from
row_group_indices you run the same source-count checks the bounded path
performs; reference the variables/functionality in the file such as
pass_read_limit, row_group_indices and the per-source validation routine (or
inline validation code used in the bounded path) and ensure the branch either
calls that validation helper or performs identical checks before producing the
return value.
🪄 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: 600cc6b3-0b91-42fc-9e84-2ddd86719091
📒 Files selected for processing (8)
cpp/include/cudf/io/experimental/hybrid_scan.hppcpp/include/cudf/io/experimental/hybrid_scan_multifile.hppcpp/src/io/parquet/experimental/hybrid_scan.cppcpp/src/io/parquet/experimental/hybrid_scan_impl.cppcpp/src/io/parquet/experimental/hybrid_scan_impl.hppcpp/src/io/parquet/experimental/hybrid_scan_multifile.cppcpp/src/io/parquet/reader_impl_helpers.hppcpp/tests/io/experimental/hybrid_scan_multifile_filters_test.cpp
| if (pass_read_limit == 0) { | ||
| return { | ||
| std::vector<std::vector<size_type>>{row_group_indices.begin(), row_group_indices.end()}}; | ||
| } |
There was a problem hiding this comment.
Validate source-count even when pass_read_limit == 0.
Line 74 returns early before source-count validation, so invalid per-source input can be accepted in unlimited mode while the bounded path rejects it.
Suggested fix
std::vector<std::vector<std::vector<size_type>>> hybrid_scan_multifile::construct_row_group_passes(
cudf::host_span<std::vector<size_type> const> row_group_indices,
std::size_t pass_read_limit) const
{
+ auto const num_sources = _impl->parquet_metadatas().size();
+ CUDF_EXPECTS(
+ row_group_indices.size() == num_sources,
+ "Encountered a mismatch in the number of row group indices vectors and the number of input "
+ "datasources",
+ std::invalid_argument);
+
auto const total_row_groups =
std::accumulate(row_group_indices.begin(),
row_group_indices.end(),
std::size_t{0},
[](auto sum, auto const& rgs) { return sum + rgs.size(); });
CUDF_EXPECTS(total_row_groups > 0, "Empty input row group indices encountered");
if (pass_read_limit == 0) {
return {
std::vector<std::vector<size_type>>{row_group_indices.begin(), row_group_indices.end()}};
}🤖 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/src/io/parquet/experimental/hybrid_scan_multifile.cpp` around lines 74 -
77, Summary: Early return when pass_read_limit == 0 skips per-source count
validation, allowing invalid input in unlimited mode. Fix: move or replicate the
per-source validation logic into the branch handling pass_read_limit == 0 so
that before returning the vector built from row_group_indices you run the same
source-count checks the bounded path performs; reference the
variables/functionality in the file such as pass_read_limit, row_group_indices
and the per-source validation routine (or inline validation code used in the
bounded path) and ensure the branch either calls that validation helper or
performs identical checks before producing the return value.
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/include/cudf/io/experimental/hybrid_scan_multifile.hpp (1)
179-179:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Incorrect exception type in documentation
Issue: Documentation claims
cudf::logic_erroris thrown, but implementation throwsstd::invalid_argument
Why: Public API contract mismatch will cause user exception handling to failEvidence from implementation (cpp/src/io/parquet/experimental/hybrid_scan_multifile.cpp:98-100):
CUDF_EXPECTS( total_row_groups > 0, "Empty input row group indices encountered", std::invalid_argument);Tests confirm
std::invalid_argumentis expected (cpp/tests/io/experimental/hybrid_scan_multifile_filters_test.cpp:274-279).Suggested fix:
- * `@throws` cudf::logic_error if no row group indices in the input + * `@throws` std::invalid_argument if no row group indices in the input🤖 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/include/cudf/io/experimental/hybrid_scan_multifile.hpp` at line 179, The docstring in hybrid_scan_multifile.hpp incorrectly states that cudf::logic_error is thrown; the implementation (CUDF_EXPECTS check on total_row_groups producing std::invalid_argument) and tests expect std::invalid_argument—update the `@throws` tag to std::invalid_argument to match CUDF_EXPECTS (the total_row_groups check) and existing tests, ensuring the public API documentation aligns with the actual exception type thrown.
🤖 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/include/cudf/io/experimental/hybrid_scan_multifile.hpp`:
- Line 179: The docstring in hybrid_scan_multifile.hpp incorrectly states that
cudf::logic_error is thrown; the implementation (CUDF_EXPECTS check on
total_row_groups producing std::invalid_argument) and tests expect
std::invalid_argument—update the `@throws` tag to std::invalid_argument to match
CUDF_EXPECTS (the total_row_groups check) and existing tests, ensuring the
public API documentation aligns with the actual exception type thrown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7b7a0508-e92d-4543-b73a-4e77ab0d87a3
📒 Files selected for processing (5)
cpp/include/cudf/io/experimental/hybrid_scan_multifile.hppcpp/src/io/parquet/experimental/hybrid_scan_impl.cppcpp/src/io/parquet/experimental/hybrid_scan_impl.hppcpp/src/io/parquet/experimental/hybrid_scan_multifile.cppcpp/tests/io/experimental/hybrid_scan_multifile_filters_test.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- cpp/src/io/parquet/experimental/hybrid_scan_multifile.cpp
- cpp/src/io/parquet/experimental/hybrid_scan_impl.cpp
- cpp/src/io/parquet/experimental/hybrid_scan_impl.hpp
- cpp/tests/io/experimental/hybrid_scan_multifile_filters_test.cpp
|
/merge |
|
/merge |
Description
Contributes to #22583
This PR adds an API to construct row group passes for multifile hybrid scan reader.
Checklist