Add python bindings for hybrid scan metadata and release GIL in the reader APIs - #23546
Conversation
Matt711
added
feature request
|
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. |
github-actions
Bot
added
libcudf
|
/ok to test 44c9b4f |
|
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 (13)
🚧 Files skipped from review as they are similar to previous changes (13)
Included review availability: Your plan includes up to 12 reviews per rolling hour; 8 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds shared Parquet metadata for hybrid scan readers. It exposes metadata construction and reader reuse through libcudf and pylibcudf, adds all-true row-mask creation, releases the GIL during native operations, and adds shared-reader validation. ChangesHybrid Scan Metadata
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to The PR adds shared hybrid-scan metadata bindings and releases the GIL in reader APIs; no actionable merge-blocking risk remains beyond normal checks and review. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/pylibcudf/pylibcudf/io/experimental/hybrid_scan.pyi (1)
65-150: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the runtime accept the declared
Sequence[Span]inputs.The stub declares seven
Sequence[Span]parameters, but the matching Cython methods declare them aslist. Tuple inputs are rejected before_get_device_span()runs. Change the runtime parameters to accept sequence-compatible objects, or restorelist[Span]in the stub.🤖 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 `@python/pylibcudf/pylibcudf/io/experimental/hybrid_scan.pyi` around lines 65 - 150, Align the runtime signatures of the hybrid scan methods accepting span collections with the stub’s Sequence[Span] declarations, rather than list-only annotations. Update the corresponding dictionary-page, bloom-filter, materialization, and chunking methods so tuple and other sequence inputs reach _get_device_span() without rejection; keep the existing behavior for list inputs.
🧹 Nitpick comments (1)
cpp/tests/io/experimental/hybrid_scan_test.cpp (1)
1002-1031: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the
FileMetaDataconstructor.Line 1003 tests only the footer-byte constructor.
hybrid_scan_metadata(FileMetaData const&, ...)is a separate new public path. Add a case that constructs shared metadata from materializedFileMetaDataand validates both reader results.🤖 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/tests/io/experimental/hybrid_scan_test.cpp` around lines 1002 - 1031, Extend the test around hybrid_scan_metadata construction to materialize FileMetaData and add a shared-metadata case using hybrid_scan_metadata(FileMetaData const&, ...), rather than only the footer-buffer constructor. Reuse the existing reader flow and expected table, and validate both independent reader results produced from the FileMetaData-based metadata.
🤖 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 `@python/pylibcudf/pylibcudf/io/experimental/hybrid_scan.pyx`:
- Around line 831-844: Protect each HybridScanReader instance with its own lock,
and acquire it across setup_chunking_for_filter_columns plus
has_next_table_chunk() and materialize_*_chunk() stateful operations, including
related wrapper-state updates. Ensure the lock spans the nogil native calls and
accesses to _input_columns, _output_buffers, _row_mask_offset,
_output_chunk_produced, and selection flags, while keeping locks independent
between reader instances.
- Around line 121-126: Update both from_parquet_metadata() methods and the
HybridScanMetadata() constructor to validate wrapper arguments and their c_obj
fields before entering with nogil or invoking native code. Raise TypeError for
None or invalid wrapper types and ValueError for wrappers with empty c_obj,
preventing null native dereferences while preserving valid construction.
- Line 867: Update the chunk materialization flow around _filter_chunk_data so
both _filter_chunk_data and _payload_chunk_data remain available across all
chunks returned by the native reader. Only clear these buffers after the final
chunk has been materialized, not after the first chunk.
---
Outside diff comments:
In `@python/pylibcudf/pylibcudf/io/experimental/hybrid_scan.pyi`:
- Around line 65-150: Align the runtime signatures of the hybrid scan methods
accepting span collections with the stub’s Sequence[Span] declarations, rather
than list-only annotations. Update the corresponding dictionary-page,
bloom-filter, materialization, and chunking methods so tuple and other sequence
inputs reach _get_device_span() without rejection; keep the existing behavior
for list inputs.
---
Nitpick comments:
In `@cpp/tests/io/experimental/hybrid_scan_test.cpp`:
- Around line 1002-1031: Extend the test around hybrid_scan_metadata
construction to materialize FileMetaData and add a shared-metadata case using
hybrid_scan_metadata(FileMetaData const&, ...), rather than only the
footer-buffer constructor. Reuse the existing reader flow and expected table,
and validate both independent reader results produced from the
FileMetaData-based metadata.
🪄 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: ef7ee563-811e-4451-9f38-e82b3af7b83d
📒 Files selected for processing (13)
cpp/include/cudf/io/experimental/hybrid_scan.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/reader_impl.cppcpp/src/io/parquet/reader_impl.hppcpp/tests/io/experimental/hybrid_scan_common.cppcpp/tests/io/experimental/hybrid_scan_test.cpppython/pylibcudf/pylibcudf/io/experimental/__init__.pypython/pylibcudf/pylibcudf/io/experimental/hybrid_scan.pxdpython/pylibcudf/pylibcudf/io/experimental/hybrid_scan.pyipython/pylibcudf/pylibcudf/io/experimental/hybrid_scan.pyxpython/pylibcudf/pylibcudf/libcudf/io/hybrid_scan.pxd
a870eff to
06df8af
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/tests/io/experimental/hybrid_scan_test.cpp (1)
1007-1020: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep both readers alive during the shared-metadata test.
Each
read_all_columns()call destroys its reader before the next call. The tests validate sequential reuse only.Create both readers first. Interleave their operations before destroying either reader. This detects reader-specific state that is incorrectly stored in shared metadata.
Based on PR objectives, this change must validate multiple
HybridScanReaderinstances sharing metadata.Also applies to: 1057-1069
🤖 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/tests/io/experimental/hybrid_scan_test.cpp` around lines 1007 - 1020, Update the shared-metadata test around read_all_columns so both HybridScanReader instances are created before either is destroyed, then interleave their row-group discovery, byte-range fetching, and materialization operations. Preserve the existing table comparisons while ensuring both readers remain alive throughout the test; apply the same lifetime pattern to the corresponding block near the second referenced section.
🤖 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.
Nitpick comments:
In `@cpp/tests/io/experimental/hybrid_scan_test.cpp`:
- Around line 1007-1020: Update the shared-metadata test around read_all_columns
so both HybridScanReader instances are created before either is destroyed, then
interleave their row-group discovery, byte-range fetching, and materialization
operations. Preserve the existing table comparisons while ensuring both readers
remain alive throughout the test; apply the same lifetime pattern to the
corresponding block near the second referenced section.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 593ce8d1-b6de-439b-9708-1ccc214d0774
📒 Files selected for processing (13)
cpp/include/cudf/io/experimental/hybrid_scan.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/reader_impl.cppcpp/src/io/parquet/reader_impl.hppcpp/tests/io/experimental/hybrid_scan_common.cppcpp/tests/io/experimental/hybrid_scan_test.cpppython/pylibcudf/pylibcudf/io/experimental/__init__.pypython/pylibcudf/pylibcudf/io/experimental/hybrid_scan.pxdpython/pylibcudf/pylibcudf/io/experimental/hybrid_scan.pyipython/pylibcudf/pylibcudf/io/experimental/hybrid_scan.pyxpython/pylibcudf/pylibcudf/libcudf/io/hybrid_scan.pxd
🚧 Files skipped from review as they are similar to previous changes (11)
- cpp/src/io/parquet/reader_impl.cpp
- cpp/src/io/parquet/experimental/hybrid_scan_impl.hpp
- cpp/src/io/parquet/experimental/hybrid_scan_impl.cpp
- python/pylibcudf/pylibcudf/io/experimental/init.py
- cpp/tests/io/experimental/hybrid_scan_common.cpp
- cpp/include/cudf/io/experimental/hybrid_scan.hpp
- cpp/src/io/parquet/experimental/hybrid_scan.cpp
- python/pylibcudf/pylibcudf/io/experimental/hybrid_scan.pxd
- python/pylibcudf/pylibcudf/libcudf/io/hybrid_scan.pxd
- cpp/src/io/parquet/reader_impl.hpp
- python/pylibcudf/pylibcudf/io/experimental/hybrid_scan.pyi
|
/ok to test 9fafd51 |
| if len(footer_bytes) > 0: | ||
| footer_ptr = &footer_bytes[0] | ||
| with nogil: | ||
| self.c_obj = make_unique[cpp_hybrid_scan_metadata]( |
There was a problem hiding this comment.
Does this need to be a shared pointer?
There was a problem hiding this comment.
Good question. I think the answer is no, it doesn't "need" to be a shared pointer. We can come up with a scenario where we end up creating multiple python HybridScanMetadata objects from the same file. With the idea being they could all share ownership of the same C++ hybrid_scan_metadata. I think I would argue in favor of the sticking with the simpler ownership sematintics of unique_ptr and then just implementing a way of caching at the python level to handle the shared metadata scenario.
wence-
left a comment
wence-
left a comment
There was a problem hiding this comment.
I am concerned about lifetime of references in the python bindings
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@python/pylibcudf/pylibcudf/io/experimental/hybrid_scan.pyx`:
- Around line 95-104: Update the typed metadata parameters in
from_parquet_metadata and from_metadata to require not None, and validate
metadata.c_obj before entering any nogil block so unset metadata raises a
TypeError rather than dereferencing null. Add an __init__ to HybridScanMetadata
that rejects direct construction, preventing default-initialized instances with
an empty unique_ptr.
In `@python/pylibcudf/pylibcudf/libcudf/io/hybrid_scan.pxd`:
- Around line 104-108: Update build_all_true_row_mask in
python/pylibcudf/pylibcudf/libcudf/io/hybrid_scan.pxd at lines 104-108 to use
std_span[const_size_type] for row_group_indices, matching the native std::span
API. Update the build_all_true_row_mask call in
python/pylibcudf/pylibcudf/io/experimental/hybrid_scan.pyx at lines 490-497 to
construct std_span from indices_vec.data() and indices_vec.size() instead of
host_span.
🪄 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: ffb876b4-fce4-4412-a11b-0f353fbe23cc
📒 Files selected for processing (13)
cpp/include/cudf/io/experimental/hybrid_scan.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/reader_impl.cppcpp/src/io/parquet/reader_impl.hppcpp/tests/io/experimental/hybrid_scan_common.cppcpp/tests/io/experimental/hybrid_scan_test.cpppython/pylibcudf/pylibcudf/io/experimental/__init__.pypython/pylibcudf/pylibcudf/io/experimental/hybrid_scan.pxdpython/pylibcudf/pylibcudf/io/experimental/hybrid_scan.pyipython/pylibcudf/pylibcudf/io/experimental/hybrid_scan.pyxpython/pylibcudf/pylibcudf/libcudf/io/hybrid_scan.pxd
wence-
left a comment
wence-
left a comment
There was a problem hiding this comment.
Approving with one C++ docs nit
0bd51ef to
aeb06b9
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
mhaseeb123
left a comment
mhaseeb123
left a comment
There was a problem hiding this comment.
Blocking merge to review it.
mhaseeb123
left a comment
mhaseeb123
left a comment
There was a problem hiding this comment.
Approving for now. I will look open a subsequent PR with a broader refactor to remove confusion between ownerships and copies.
| /** | ||
| * @brief Own Parquet file metadata from a pre-populated `FileMetaData` | ||
| * | ||
| * @param parquet_metadata Pre-populated Parquet file metadata | ||
| * @param options Parquet reader options | ||
| */ | ||
| hybrid_scan_metadata(FileMetaData const& parquet_metadata, parquet_reader_options const& options); |
There was a problem hiding this comment.
non-blocking as it needs a broader fix in a follow up. This may be misleading as the FileMetaData object is copied internally by metadata(FileMetaData&) and any subsequent operations (such as setting page index) on hybrid_scan_metadata won't affect the original FileMetaData it was constructed from.
| * @note The metadata is immutable after `setup_page_index()` has been called (or immediately after | ||
| * construction if page index setup is skipped). Concurrent usage by multiple readers is thread | ||
| * safe. This handle does not support multi-source (multi-file) metadata. |
There was a problem hiding this comment.
Note: setup_page_index() may itself only be called from one reader using this. Otherwise, it is not thread-safe.
There was a problem hiding this comment.
This docstring feels off. The object itself is not immutable. Immutability is enforced at the level of the reader that takes in and leverages this metadata in a particular way. The shared_ptr management is all done in the readers, not here. The documentation and management of appropriate usage should be in the place where that control is actually happening.
vyasr
left a comment
vyasr
left a comment
There was a problem hiding this comment.
I'm a bit concerned about exactly how the ownership sharing is done here.
| * @note The metadata is immutable after `setup_page_index()` has been called (or immediately after | ||
| * construction if page index setup is skipped). Concurrent usage by multiple readers is thread | ||
| * safe. This handle does not support multi-source (multi-file) metadata. |
There was a problem hiding this comment.
This docstring feels off. The object itself is not immutable. Immutability is enforced at the level of the reader that takes in and leverages this metadata in a particular way. The shared_ptr management is all done in the readers, not here. The documentation and management of appropriate usage should be in the place where that control is actually happening.
There was a problem hiding this comment.
Based on my other feedback, can you add a test that actually creates multiple readers? That would suss out the kinds of issues I was worried about above.
|
My other two bits of feedback are still valid and I'd like to see them implemented, but the shared_ptr moves no longer look unsafe here so I'm approving to unblock. Thanks! |
|
/merge |
8 participants
Description
This PR allows multiple HybridScanReader instances to share pre-parsed Parquet file metadata rather than each re-parsing it. It also releases the GIL in all existing
HybridScanReadermethods. Used in #23317.Checklist