NVIDIA / NVIDIA/cudf

[PERF] Avoid deep-cloning FileMetaData on parquet_metadatas reuse

Open
#23,572 3 comments 0 reactions 1 assignee Claimed by @TomAugspurger View on GitHub
cudf-polars Performance
Dominant language
C++
Stars
9.8k
Forks
1.1k
Avg merge
3d 6m
Merged PRs (30d)
278

Description

Follow up to #23558. That PR fixed the GIL contention when reusing prefetched `FileMetaData` across concurrent reads. But there's still a cost left on the table, raised in this thread: https://github.com/NVIDIA/cudf/pull/23558#discussion_r3731627553

Every `pylibcudf.io.parquet.read_parquet` / `ChunkedParquetReader` call that passes `parquet_metadatas=` still deep clones the whole `FileMetaData` object before reading. You can see it in `parquet.pyx`:

```cython
with nogil:
c_metadatas.reserve(metadata_ptrs.size())
for i in range(metadata_ptrs.size()):
c_metadatas.push_back(dereference(metadata_ptrs[i])) # deep copy
c_result = move(cpp_read_parquet(move(sources), move(c_metadatas), ...))
```

The GIL isn't held for this anymore, so the copy itself is cheaper to do concurrently, but it's still a full copy on every read, and it's forced by the API: `read_parquet` and `chunked_parquet_reader` take `std::vector&&`, so they need to own what's passed in. For files with lots of row groups and column chunks (especially with page indexes), that's a lot of data being copied just to read from it.

I looked at what `aggregate_reader_metadata` actually mutates once it has the metadata (`reader_impl_helpers.cpp:860-908`), and it's small:

1. `schema[i].repetition_type` on the first file only, promoting `REQUIRED` to `OPTIONAL` when merging sources with mismatched nullability.
2. `apply_arrow_schema()`, also just schema.
3. Erasing `ARROW_SCHEMA_KEY` from each file's `keyval_maps`.

`row_groups`, and everything under it (column chunks, page indexes), is never touched. That's the bulk of the object for wide files, and we're cloning it for no reason.

So the fix is to stop requiring ownership. Change the metadata-taking constructors and readers to accept `host_span` instead of `std::vector&&`. `hybrid_scan` already does something like this, so there's precedent. Inside `aggregate_reader_metadata`, only copy the small mutable part (`schema` and `keyval_maps`) into a local working copy, and just reference `row_groups` from the caller's metadata instead of copying it. Once the C++ side takes a span, we can drop the `c_metadatas` copy loop in `parquet.pyx` entirely and pass pointers straight through.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.