NVIDIA / NVIDIA/cudf

[FEA] Refactor hybrid-scan APIs to avoid caller-side regrouping

Open
#22,971 0 comments 0 reactions 0 assignees View on GitHub
0 - Backlog feature request
Dominant language
C++
Stars
9.8k
Forks
1.1k
Avg merge
3d 6m
Merged PRs (30d)
278

Description

**Is your feature request related to a problem? Please describe.**

The multifile hybrid-scan byte-range APIs (`all_column_chunks_byte_ranges`, `dictionary_pages_byte_ranges`, and any others with the same shape) return `std::pair, std::vector>` — a flat list of byte ranges plus a parallel per-range source map.

Every caller must regroup the flat ranges by source before calling the vectorized `fetch_byte_ranges_to_device_async`. This regrouping is duplicated.

**Describe the solution you'd like**

Since per-source ranges are almost always laid out contiguously, return source offsets instead of a per-range source map, e.g.

```
struct byte_range_source_map {
std::vector src_indices; // source per run
std::vector src_offsets; // run start offsets; + ranges.size() at the end
};
```

Apply uniformly to all APIs with this return type so callers can slice per-source spans directly and the duplicated test helpers can be deleted.

**Additional context**

> This nested loop to detect runs of the same source index isn't particularly intuitive. Is the return type of `dictionary_pages_byte_ranges` set in stone already (e.g., for compatibility with `all_column_chunks_byte_ranges`)? If not, we could return a data structure instead of a vector, e.g., a struct with 2 vectors — the source indices and the offsets of the starts of the runs in the source map (which would be easy to compute in the `dictionary_pages_byte_ranges` implementation, e.g., in the outer `for_each` right after the inner `for_each`).
>
> For example, suppose we return:
>
> ```c++
> class hybrid_scan_multifile {
> public:
> …
> struct dict_page_source_map {
> std::vector src_indices; // per byte range
> std::vector src_offsets; // per source with pages; plus src_indices.size() for convenience
> };
>
> [[nodiscard]] std::pair, dict_page_source_map>
> dictionary_pages_byte_ranges(…);
> …
> };
> ```
>
> Then this loop becomes, e.g.:
>
> ```c++
> auto const [dict_page_byte_ranges, dict_page_source_map] =
> reader.dictionary_pages_byte_ranges(input_row_group_indices, options);
> …
> auto byte_ranges_it = dict_page_byte_ranges.begin();
> for (std::size_t src_idx = 1; src_idx < dict_page_source_map.src_offsets.size(); ++src_idx) {
> auto const start = dict_page_source_map.src_offsets[src_idx - 1];
> auto const end = dict_page_source_map.src_offsets[src_idx];
> std::vector src_ranges(byte_ranges_it + start, byte_ranges_it + end);
> auto [buffers, data, task] = cudf::io::parquet::fetch_byte_ranges_to_device_async(
> *inputs.datasources[dict_page_source_map.src_indices[start]], src_ranges, stream, mr);
> …
> }
> ```
>
> We could make it even nicer by returning a custom iterator, but that's a longer discussion…
>
> _Originally posted by @igorpeshansky in https://github.com/rapidsai/cudf/pull/22866#discussion_r3456880134_
>
> ---
>
> I like the suggestion from @igorpeshansky - return an offsets vector instead of per-byte-range source map as byte ranges per source are almost always contiguously laid out.
>
> However a couple things:
>
> 1. We should not be fetching byte ranges datasource by datasource and instead be using the vectorized `fetch_byte_ranges_to_device` API which does all at once.
> 2. The suggested cleanup should be applied all at once in a future PR. We should improve all APIs with the same return type if needed
>
> _Originally posted by @mhaseeb123 in https://github.com/rapidsai/cudf/pull/22866#discussion_r3464088310_

Contributor guide

Open the contributing guide

Research direction

Start at the multifile hybrid-scan APIs all_column_chunks_byte_ranges and dictionary_pages_byte_ranges, then trace callers using the parallel source map and vectorized fetch_byte_ranges_to_device_async. Done means the APIs consistently return source-run offsets, callers no longer regroup ranges, and duplicated test helpers are removed; resolve the compatibility and vectorized-fetch questions before implementation.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend-api-design, data-engineering
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
39/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.