Repository navigation
Move Hybrid scan internals out of the experimental namespace - #24940
mhaseeb123 wants to merge 7 commits into
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. |
|
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.
|
| Layer / File(s) | Summary |
|---|---|
Update hybrid-scan declarations cpp/include/cudf/io/experimental/hybrid_scan*.hpp, cpp/src/io/parquet/hybrid_scan/*_helpers.hpp, cpp/src/io/parquet/hybrid_scan/hybrid_scan_impl.hpp, cpp/src/io/parquet/hybrid_scan/page_index_filter_utils.hpp |
Implementation pointers, forward declarations, and helper declarations now use cudf::io::parquet::hybrid_scan::detail. |
Relocate implementation and build entries cpp/CMakeLists.txt, cpp/src/io/parquet/experimental/hybrid_scan*.cpp, cpp/src/io/parquet/hybrid_scan/* |
Implementation definitions and constructor references use the new detail namespace. The CMake source list uses the hybrid-scan directory for the relocated sources. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Refactor
Possibly related issues
- NVIDIA/cudf#24852: This PR relocates hybrid-scan internals but does not change reader behavior or implement the default-reader change and its follow-ups.
Suggested reviewers: cryos
Merge Risk: 
Minimal · up to 68c09
-
No merge-blocking issue is identified; merge after normal build and test checks.
Pre-merge checks: 
5
-
Full details: Title check
-
Explanation
The title names the main change: moving Parquet hybrid scan internals out of the experimental namespace. The raw summary and the description confirm the move from src/io/parquet/experimental to src/io/parquet/hybrid_scan and the new cudf::io::parquet::hybrid_scan::detail namespace. The title is specific and matches the changeset.
Full details: Description check
-
Explanation
The description states that the PR moves Parquet hybrid scan internals from src/io/parquet/experimental to src/io/parquet/hybrid_scan and names the new namespace cudf::io::parquet::hybrid_scan::detail. This directly describes the changeset, so the check passes.
Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 14 files.
-
Finishing Touches
-
Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
CodeRabbit Verify
- Run CodeRabbit Verify to test the changes in this pull request.
Description
This trivial PR moves the Parquet hybrid scan internals from
src/io/parquet/experimentaltosrc/io/parquet/hybrid_scan. New namespace:cudf::io::parquet::hybrid_scan::detail(internals).Checklist