Fix uninitialized page info for column chunks without a dictionary page - #23579
Conversation
|
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR documents dictionary page span ordering, decodes dictionary page headers from supplied data, updates preprocessing allocation and page sorting, and adds filtering tests for mixed dictionary and plain encoding. ChangesDictionary page decoding and filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cpp/tests/io/experimental/hybrid_scan_filters_test.cpp (1)
1812-1813: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse a non-default CUDA stream.
Create a test-owned stream and pass its view to the dictionary filtering path. Default-stream ordering can hide missing stream dependencies.
As per coding guidelines, “Avoid unnecessary host-device synchronization, default-stream use, and
cudaDeviceSynchronize().”🤖 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 1812 - 1813, Update the test around the dictionary filtering path to create an owned non-default CUDA stream, then pass that stream’s view through the relevant filtering calls instead of using cudf::get_default_stream(). Keep the existing device resource selection unchanged and ensure all operations under test use the test-owned stream.Source: Coding guidelines
cpp/src/io/parquet/experimental/hybrid_scan_preprocess.cu (1)
83-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare the device-lambda return type.
Add
-> voidto the lambda at Line 87. The lambda is passed to a Thrust device algorithm and must have an explicit return type.Proposed update
- [chunks = chunks.device_begin(), pages = pages.device_begin()] __device__(auto chunk_idx) { + [chunks = chunks.device_begin(), pages = pages.device_begin()] __device__(auto chunk_idx) -> void {As per coding guidelines, “Declare explicit return types for extended device lambdas passed to device algorithms.”
🤖 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/experimental/hybrid_scan_preprocess.cu` around lines 83 - 92, Update the device lambda passed to thrust::for_each in the hybrid scan preprocessing code to explicitly declare a void return type. Preserve the existing page flag check and dictionary-page assignment.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.
Inline comments:
In `@cpp/include/cudf/io/experimental/hybrid_scan.hpp`:
- Around line 409-410: Update the documentation for secondary filter byte ranges
near secondary_filters_byte_ranges() to identify dictionary_page_data as
matching the second element of the returned vector, after bloom-filter ranges.
Clarify that spans must be aligned with dictionary-page ranges rather than the
bloom-filter ranges.
In `@cpp/tests/io/experimental/hybrid_scan_filters_test.cpp`:
- Around line 1776-1895: Expand DictionaryFilterGapTest to cover empty input,
nullable strings, sliced columns, and non-ASCII UTF-8 values while retaining the
existing multi-block dictionary/plain row-group scenario. Add these cases
through the test’s input construction and assertions so dictionary filtering
behavior remains validated for each required string edge case.
---
Nitpick comments:
In `@cpp/src/io/parquet/experimental/hybrid_scan_preprocess.cu`:
- Around line 83-92: Update the device lambda passed to thrust::for_each in the
hybrid scan preprocessing code to explicitly declare a void return type.
Preserve the existing page flag check and dictionary-page assignment.
In `@cpp/tests/io/experimental/hybrid_scan_filters_test.cpp`:
- Around line 1812-1813: Update the test around the dictionary filtering path to
create an owned non-default CUDA stream, then pass that stream’s view through
the relevant filtering calls instead of using cudf::get_default_stream(). Keep
the existing device resource selection unchanged and ensure all operations under
test use the test-owned stream.
🪄 Autofix
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: 0b166863-c2f4-4693-bc42-ea2e3d16ab6a
📒 Files selected for processing (5)
cpp/include/cudf/io/experimental/hybrid_scan.hppcpp/include/cudf/io/experimental/hybrid_scan_multifile.hppcpp/src/io/parquet/experimental/hybrid_scan_preprocess.cucpp/src/io/parquet/reader_impl_preprocess_utils.cucpp/tests/io/experimental/hybrid_scan_filters_test.cpp
|
/merge |
Description
This PR zero-initializes page info buffers in dictionary page row group filtering to ensure empty buffers (no pages) are reliably discarded by the decoder instead of reading uninitialized data.
Checklist