Supply stream and mr to column synthesizers - #23209
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughParquet output assembly now forwards explicit CUDA stream and memory resource parameters to row-index and source-index synthesis helpers. The helpers use those parameters for device allocations, asynchronous copies, labeling, transforms, synchronization, and returned columns. ChangesParquet index column context
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/io/parquet/reader_impl_preprocess.cu (1)
1226-1240: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStream mismatch:
host_row_offsetsstill allocated on_streaminstead of the newstreamparameter.
synthesize_source_index_columnnow takes an explicitstream/mr, and every other allocation/op in this block correctly usesstream(lines 1235-1239), buthost_row_offsetson line 1230 is still allocated against the member_stream. The subsequent async H2D copy (make_device_uvector_async(host_row_offsets, stream, ...)) andlabel_segmentsrun onstream, while the pinned staging buffer's lifetime/deallocation is tracked against_stream. If a future caller passes astreamdifferent from_stream, the pinned buffer could be reclaimed/reused once_streamcompletes, before the copy onstreamfinishes — a stream-ordering race on the staging buffer. Today it happens to be safe only because the sole caller (finalize_output) passes_streamfor both parameters, but that coincidence defeats the purpose of this refactor.🐛 Proposed fix
auto host_row_offsets = - cudf::detail::make_empty_pinned_vector<cudf::size_type>(num_sources + 1, _stream); + cudf::detail::make_empty_pinned_vector<cudf::size_type>(num_sources + 1, stream);As per coding guidelines, "Propagate stream and memory-resource parameters through internal APIs consistently" and "Ensure async memcpy staging buffers outlive the copy they are staging."
🤖 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/reader_impl_preprocess.cu` around lines 1226 - 1240, In synthesize_source_index_column, the pinned staging buffer allocation for host_row_offsets is still tied to _stream while the rest of the work uses the explicit stream parameter. Update that allocation to use the same stream passed into the function so the pinned buffer lifetime and async H2D copy in make_device_uvector_async remain ordered on the correct stream. Keep the existing label_segments and synchronization flow, but make the stream usage consistent throughout this block.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@cpp/src/io/parquet/reader_impl_preprocess.cu`:
- Around line 1226-1240: In synthesize_source_index_column, the pinned staging
buffer allocation for host_row_offsets is still tied to _stream while the rest
of the work uses the explicit stream parameter. Update that allocation to use
the same stream passed into the function so the pinned buffer lifetime and async
H2D copy in make_device_uvector_async remain ordered on the correct stream. Keep
the existing label_segments and synchronization flow, but make the stream usage
consistent throughout this block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 34aa6af4-35af-4b03-b9de-6fd7bde95b4a
📒 Files selected for processing (3)
cpp/src/io/parquet/reader_impl.cppcpp/src/io/parquet/reader_impl.hppcpp/src/io/parquet/reader_impl_preprocess.cu
|
/merge |
Description
Follow up #23077
Supply stream and mr to column synthesizers which are used to produce the output columns
Checklist