Parquet utilities to fetch footer and page index buffers from multiple sources

[mhaseeb123]

Description

Contributes to #22583. Follow up #22586

This PR adds new overloads of remaining Parquet IO utility to fetch parquet footers and page index bytes from multiple sources.

Checklist

• I am familiar with the Contributing Guidelines.
• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[copy-pr-bot]

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.

[mhaseeb123]

Helper (fetches one footer) dispatched for all sources in a loop or via thread pool if more than parallel_threshold

[mhaseeb123]

Helper (fetches one page index bytes) dispatched for all sources in a loop or via thread pool if more than parallel_threshold

[coderabbitai]

Actionable comments posted: 3

🤖 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/io_utils/parquet_io_utils.cpp`:
- Around line 46-54: The code computes len - ender_len and calls
datasource.host_read to fetch the footer before validating the data source size,
which can underflow for short inputs; move the CUDF_EXPECTS(len > header_len +
ender_len, "Incorrect data source") check to before any computation or call that
uses len - ender_len (i.e., before the ender_buffer = datasource.host_read(...)
and reinterpret_cast to file_ender_s), so validate length first and only then
call datasource.host_read for the footer and interpret buffers for file_header_s
and file_ender_s.
- Around line 98-101: The lambda fetch_page_index may pass negative
byte_range_info values to datasource.host_read because byte_range_info::offset()
and ::size() are signed; guard against negative offset() or size() before
calling host_read in fetch_page_index (or validate page_index_bytes up front in
the calling code). Specifically, in the fetch_page_index lambda (and any callers
using cudf::io::text::byte_range_info), check that page_index_bytes.offset() >=
0 and page_index_bytes.size() >= 0 and handle invalid ranges by returning an
appropriate error/result (e.g., throw a descriptive exception or return an empty
buffer/error) instead of calling datasource.host_read with converted huge
unsigned values.

In `@cpp/tests/io/experimental/hybrid_scan_filters_test.cpp`:
- Around line 164-209: The test MultiSourceFooterAndPageIndex only exercises the
nominal multi-source path; extend it to validate edge cases and the parallel
split by adding sub-cases that call cudf::io::parquet::fetch_footers_to_host and
cudf::io::parquet::fetch_page_indexes_to_host with: (1) empty inputs (empty
datasources vector and empty page_index_byte_ranges) and assert they return
empty buffers or throw as appropriate, (2) a size-mismatch between datasources
and page_index_byte_ranges to assert the expected failure from
fetch_page_indexes_to_host, and (3) boundary sizes around the parallel_threshold
by running the same logic with num_sources set to the threshold (e.g., 16) and
threshold+1 (e.g., 17) to ensure both serial and parallel branches in
hybrid_scan_reader::page_index_byte_range and fetch_page_indexes_to_host are
exercised; use the same helpers as the existing test (hybrid_scan_reader,
page_index_byte_range, fetch_footer_to_host, fetch_page_index_to_host) and add
ASSERT/EXPECT checks comparing sizes and contents or expecting errors for
mismatch/empty cases.

🪄 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: b9f4e719-3945-4894-9beb-e494a637eb86

📥 Commits

Reviewing files that changed from the base of the PR and between 5967cc2 and 94df470.

📒 Files selected for processing (3)

• cpp/include/cudf/io/parquet_io_utils.hpp
• cpp/src/io/parquet/io_utils/parquet_io_utils.cpp
• cpp/tests/io/experimental/hybrid_scan_filters_test.cpp

[mhaseeb123]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (3)

cpp/src/io/parquet/io_utils/parquet_io_utils.cpp (2)

94-98: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Validate datasource length before tail read.

Line 96 computes len - ender_len and reads before the size precondition is checked on Line 98. For short inputs this can underflow and trigger invalid IO.

Suggested patch

   auto const fetch_footer = [](cudf::io::datasource& datasource) {
     constexpr auto header_len = sizeof(file_header_s);
     constexpr auto ender_len  = sizeof(file_ender_s);
     size_t const len          = datasource.size();
+    CUDF_EXPECTS(len > header_len + ender_len, "Incorrect data source");
 
     auto header_buffer = datasource.host_read(0, header_len);
     auto const header  = reinterpret_cast<file_header_s const*>(header_buffer->data());
     auto ender_buffer  = datasource.host_read(len - ender_len, ender_len);
     auto const ender   = reinterpret_cast<file_ender_s const*>(ender_buffer->data());
-    CUDF_EXPECTS(len > header_len + ender_len, "Incorrect data source");

🤖 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/io_utils/parquet_io_utils.cpp` around lines 94 - 98, The
code reads the tail (ender_buffer = datasource.host_read(len - ender_len,
ender_len)) before validating the data source length, which can underflow for
small inputs; move or add the size check using CUDF_EXPECTS(len > header_len +
ender_len, "Incorrect data source") before any host_read that computes len -
ender_len (so both header_buffer and ender_buffer reads occur only after
validation), and ensure the length arithmetic uses an unsigned/size_t-safe
comparison to avoid underflow when computing the read offset for
datasource.host_read.

---

123-126: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard page-index ranges before calling host_read.

Line 125 forwards unchecked offset/size directly to IO. Add explicit range validation to prevent invalid reads and arithmetic wraparound.

Suggested patch

   auto const fetch_page_index = [](cudf::io::datasource& datasource,
                                    cudf::io::text::byte_range_info const& page_index_bytes) {
-    return datasource.host_read(page_index_bytes.offset(), page_index_bytes.size());
+    auto const offset = static_cast<std::size_t>(page_index_bytes.offset());
+    auto const size   = static_cast<std::size_t>(page_index_bytes.size());
+    CUDF_EXPECTS(offset <= datasource.size(), "Invalid page index offset");
+    CUDF_EXPECTS(size <= (datasource.size() - offset), "Invalid page index size");
+    return datasource.host_read(offset, size);
   };

🤖 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/io_utils/parquet_io_utils.cpp` around lines 123 - 126, The
lambda fetch_page_index forwards page_index_bytes.offset() and .size() to
datasource.host_read without validation; add explicit range checks in
fetch_page_index to ensure size > 0 and that offset and size do not overflow and
that offset + size is within the datasource's available length before calling
datasource.host_read (use a safe check like offset <= max_length - size to avoid
wraparound). If the range is invalid, return an error/empty buffer or propagate
a clear exception instead of calling host_read; reference fetch_page_index,
cudf::io::datasource::host_read, and
cudf::io::text::byte_range_info::offset/size when implementing the checks.

cpp/tests/io/experimental/hybrid_scan_filters_test.cpp (1)

164-209: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Expand this new test to cover edge and boundary paths.

This currently validates only the nominal path (num_sources = 3). Please add sub-cases for empty inputs, datasource/page-range size mismatch, and boundary sizes around the dispatch split (threshold and threshold+1) so both serial and parallel paths are exercised.

As per coding guidelines, cpp/**/*test*.{cu,cpp}: “Test functions must cover edge cases: empty input, null values, sliced columns, boundary sizes, multi-block sizes”.

🤖 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_filters_test.cpp` around lines 164 -
209, Expand TEST_F(HybridScanFiltersTest, MultiSourceFooterAndPageIndex) to add
sub-cases exercising empty inputs, size mismatches, and boundary dispatch
behavior: add runs with num_sources = 0 (empty datasources) to verify
fetch_footers_to_host/fetch_page_indexes_to_host handle empty input; add a case
where the datasources array length differs from page_index_byte_ranges length to
assert the functions return an error/throw (or validate documented behavior)
when calling fetch_page_indexes_to_host; and add runs with num_sources equal to
the reader dispatch split threshold and threshold+1 (use the library's dispatch
threshold constant or the value that controls serial vs parallel dispatch) to
exercise both serial and parallel code paths when calling
fetch_footers_to_host/fetch_page_indexes_to_host and comparing to single-source
results (use hybrid_scan_reader::page_index_byte_range, fetch_footer_to_host,
fetch_page_index_to_host as in the test). Ensure each sub-case performs the same
assertions (size and memcmp) or appropriate error assertions so
empty/mismatched/boundary behavior is validated.

🤖 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/include/cudf/io/parquet_io_utils.hpp`:
- Around line 56-57: The header declares public, side-effect-free functions that
return owned buffers (notably fetch_footers_to_host and the other
buffer-returning APIs declared nearby); mark these function declarations with
the [[nodiscard]] attribute so callers cannot silently drop the returned buffers
— update the declarations in cpp/include/cudf/io/parquet_io_utils.hpp to
prepend/annotate [[nodiscard]] on each non-void, buffer-returning function
(e.g., fetch_footers_to_host and the other functions around lines 80-82).

---

Duplicate comments:
In `@cpp/src/io/parquet/io_utils/parquet_io_utils.cpp`:
- Around line 94-98: The code reads the tail (ender_buffer =
datasource.host_read(len - ender_len, ender_len)) before validating the data
source length, which can underflow for small inputs; move or add the size check
using CUDF_EXPECTS(len > header_len + ender_len, "Incorrect data source") before
any host_read that computes len - ender_len (so both header_buffer and
ender_buffer reads occur only after validation), and ensure the length
arithmetic uses an unsigned/size_t-safe comparison to avoid underflow when
computing the read offset for datasource.host_read.
- Around line 123-126: The lambda fetch_page_index forwards
page_index_bytes.offset() and .size() to datasource.host_read without
validation; add explicit range checks in fetch_page_index to ensure size > 0 and
that offset and size do not overflow and that offset + size is within the
datasource's available length before calling datasource.host_read (use a safe
check like offset <= max_length - size to avoid wraparound). If the range is
invalid, return an error/empty buffer or propagate a clear exception instead of
calling host_read; reference fetch_page_index, cudf::io::datasource::host_read,
and cudf::io::text::byte_range_info::offset/size when implementing the checks.

In `@cpp/tests/io/experimental/hybrid_scan_filters_test.cpp`:
- Around line 164-209: Expand TEST_F(HybridScanFiltersTest,
MultiSourceFooterAndPageIndex) to add sub-cases exercising empty inputs, size
mismatches, and boundary dispatch behavior: add runs with num_sources = 0 (empty
datasources) to verify fetch_footers_to_host/fetch_page_indexes_to_host handle
empty input; add a case where the datasources array length differs from
page_index_byte_ranges length to assert the functions return an error/throw (or
validate documented behavior) when calling fetch_page_indexes_to_host; and add
runs with num_sources equal to the reader dispatch split threshold and
threshold+1 (use the library's dispatch threshold constant or the value that
controls serial vs parallel dispatch) to exercise both serial and parallel code
paths when calling fetch_footers_to_host/fetch_page_indexes_to_host and
comparing to single-source results (use
hybrid_scan_reader::page_index_byte_range, fetch_footer_to_host,
fetch_page_index_to_host as in the test). Ensure each sub-case performs the same
assertions (size and memcmp) or appropriate error assertions so
empty/mismatched/boundary behavior is validated.

🪄 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: 1a73a544-a58e-44dd-8c20-9fa4b88cbc81

📥 Commits

Reviewing files that changed from the base of the PR and between 5967cc2 and 27e01c4.

📒 Files selected for processing (3)

• cpp/include/cudf/io/parquet_io_utils.hpp
• cpp/src/io/parquet/io_utils/parquet_io_utils.cpp
• cpp/tests/io/experimental/hybrid_scan_filters_test.cpp

[bdice]

Do we want to start complying with the recommendations in #22588?

[mhaseeb123]

Not yet. Waiting for this reader to be feature complete (few PRs related to this) before I pull the trigger on the entire thing.

[qbacpey]

One comment, otherwise LGTM.

[mhaseeb123]

/merge