Repository navigation
Add python bindings for hybrid scan metadata and release GIL in the reader APIs #23546
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
051e21d
06df8af
9fafd51
5627e40
be697fe
916cd4c
bb57d2f
2dfac40
aeb06b9
2b09a07
d424c6a
0350367
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,11 @@ namespace cudf::io::parquet::experimental::detail { | |
| * Hybrid Scan operation. | ||
| */ | ||
| class hybrid_scan_reader_impl; | ||
|
|
||
| /** | ||
| * @brief Internal parsed Parquet file metadata for the Hybrid Scan reader. | ||
| */ | ||
| class aggregate_reader_metadata; | ||
| } // namespace cudf::io::parquet::experimental::detail | ||
|
|
||
| //! Using `byte_range_info` from cudf::io::text | ||
|
|
@@ -52,6 +57,72 @@ enum class use_data_page_mask : bool { | |
| NO = false ///< Do not compute or use a data page mask | ||
| }; | ||
|
|
||
| /** | ||
| * @brief Shareable, pre-parsed Parquet file metadata for the Hybrid Scan reader. | ||
| * | ||
| * Parses the Parquet file metadata once so that multiple `hybrid_scan_reader` instances reading | ||
| * the same file can borrow it rather than each re-parsing and copying the row group metadata. | ||
| * The intended use is to read disjoint row-group ranges of a single file: construct one | ||
| * `hybrid_scan_metadata` per file and pass it to as many readers as there are ranges. | ||
| * | ||
| * @code{.cpp} | ||
| * // Parse the metadata once | ||
| * auto metadata = std::make_shared<parquet::experimental::hybrid_scan_metadata>(*footer_buffer, | ||
| * options); | ||
| * // Construct lightweight readers that share it | ||
| * auto reader_a = std::make_unique<parquet::experimental::hybrid_scan_reader>(*metadata); | ||
| * auto reader_b = std::make_unique<parquet::experimental::hybrid_scan_reader>(*metadata); | ||
| * @endcode | ||
| * | ||
| * @note The metadata is immutable after `setup_page_index()` has been called (or immediately after | ||
| * construction if page index setup is skipped). Readers sharing one instance may read different | ||
| * row-group ranges of the same single file concurrently; overlapping ranges produce duplicate rows. | ||
| * This handle does not support multi-source (multi-file) metadata. | ||
|
Matt711 marked this conversation as resolved.
Outdated
|
||
| */ | ||
| class hybrid_scan_metadata { | ||
| public: | ||
| /** | ||
| * @brief Parse and own Parquet file metadata from a span of footer bytes | ||
| * | ||
| * @param footer_bytes Host span of Parquet file footer bytes | ||
| * @param options Parquet reader options | ||
| */ | ||
| hybrid_scan_metadata(cudf::host_span<uint8_t const> footer_bytes, | ||
| parquet_reader_options const& options); | ||
|
|
||
| /** | ||
| * @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); | ||
|
Comment on lines
+92
to
+98
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. non-blocking as it needs a broader fix in a follow up. This may be misleading as the |
||
|
|
||
| /** | ||
| * @brief Destructor for the shared Parquet metadata | ||
| */ | ||
| ~hybrid_scan_metadata(); | ||
|
|
||
| hybrid_scan_metadata(hybrid_scan_metadata const&) = default; ///< Copy constructor | ||
| hybrid_scan_metadata(hybrid_scan_metadata&&) = default; ///< Move constructor | ||
|
|
||
| /** | ||
| * @brief Copy assignment operator | ||
| * @return Reference to this object | ||
| */ | ||
| hybrid_scan_metadata& operator=(hybrid_scan_metadata const&) = default; | ||
|
|
||
| /** | ||
| * @brief Move assignment operator | ||
| * @return Reference to this object | ||
| */ | ||
| hybrid_scan_metadata& operator=(hybrid_scan_metadata&&) = default; | ||
|
|
||
| private: | ||
| std::shared_ptr<detail::aggregate_reader_metadata> _metadata; | ||
| friend class hybrid_scan_reader; | ||
|
wence- marked this conversation as resolved.
|
||
| }; | ||
|
|
||
| /** | ||
| * @brief The experimental parquet reader class to optimally read parquet files subject to | ||
| * highly selective filters, called a Hybrid Scan operation | ||
|
|
@@ -300,6 +371,18 @@ class hybrid_scan_reader { | |
| explicit hybrid_scan_reader(FileMetaData const& parquet_metadata, | ||
| parquet_reader_options const& options); | ||
|
|
||
| /** | ||
| * @brief Constructor that borrows shared, pre-parsed Parquet file metadata | ||
| * | ||
| * Constructs a reader that shares `metadata` instead of parsing and copying the file metadata | ||
| * again. Use this to read disjoint row-group ranges of a single file without paying the metadata | ||
| * copy per reader. The reader options that govern reading (column selection, filter, ...) are | ||
| * supplied per call to the individual read methods. | ||
|
Matt711 marked this conversation as resolved.
Outdated
|
||
| * | ||
| * @param metadata Shared, pre-parsed Parquet file metadata | ||
| */ | ||
| explicit hybrid_scan_reader(hybrid_scan_metadata const& metadata); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: the type signature of this method (and I know it is similar to But here that is not the case, because a I wonder, and will summon @mhaseeb123, if it makes more sense to do a in some sense "larger" refactor:
Introduce a new ctor An alternative would be for the hybrid_scan_reader to take a raw pointer, but then lifetime management would be on the caller, which is fiddlier. WDYT?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about we take it by-value instead? 916cd4c Copying the "hybrid_scan_metadata" just means copying the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI Haseeb's out until next week, so would love to get this in sooner. I feel more comfortable doing that because hybrid scan stuff is in an experimental namespace.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think taking in a shared ptr of
This isn't really helpful as
This one makes more sense to me. |
||
|
|
||
| /** | ||
| * @brief Destructor for the experimental parquet reader class | ||
| */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,6 +59,17 @@ class hybrid_scan_reader_impl : public parquet::detail::reader_impl { | |
| explicit hybrid_scan_reader_impl(cudf::host_span<FileMetaData const> parquet_metadatas, | ||
| parquet_reader_options const& options); | ||
|
|
||
| /** | ||
| * @brief Constructor that shares pre-parsed Parquet metadata | ||
| * | ||
| * Borrows an already-constructed `aggregate_reader_metadata` instead of parsing and copying the | ||
| * file metadata again. Multiple single-file readers can share one metadata object, avoiding a | ||
| * per-reader copy of the (potentially large) row group metadata. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Usually if one passes a
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 916cd4c |
||
| * | ||
| * @param metadata Shared, pre-parsed Parquet file metadata. Must not be null. | ||
| */ | ||
| explicit hybrid_scan_reader_impl(std::shared_ptr<aggregate_reader_metadata> metadata); | ||
|
|
||
| /** | ||
| * @copydoc cudf::io::parquet::experimental::hybrid_scan_multifile::parquet_metadatas | ||
| */ | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,16 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION. | ||
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| from pylibcudf.io.experimental.hybrid_scan import ( | ||
| HybridScanMetadata, | ||
| HybridScanReader, | ||
| UseDataPageMask, | ||
| ) | ||
| from pylibcudf.io.parquet_metadata import FileMetaData | ||
|
|
||
| __all__ = [ | ||
| "FileMetaData", # backwards compatibility | ||
| "HybridScanMetadata", | ||
| "HybridScanReader", | ||
| "UseDataPageMask", | ||
| ] |
Uh oh!
There was an error while loading. Please reload this page.