Experimental Parquet reader supports retention vectors - #23402
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesRetention Vector Support
Estimated code review effort: 3 (Moderate) | ~20 minutes 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.
Actionable comments posted: 2
🤖 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/deletion_vectors.hpp`:
- Around line 51-53: Update compute_num_deleted_rows to honor
are_retention_vectors: when retention mode is enabled, invert the bitmap count
so the API returns deleted-row count rather than retained-row count. Preserve
the existing count behavior for non-retention vectors.
In `@cpp/tests/io/parquet_deletion_vectors_test.cpp`:
- Around line 303-305: Update the concatenated bitmap generation in
build_roaring_bitmap_and_expected_row_mask to forward
final_deletion_vector_info.are_retention_vectors to each generated bitmap,
preserving retention semantics when num_concat is greater than one.
🪄 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: a99b0488-dd73-44eb-90cf-2834e00db6a2
📒 Files selected for processing (3)
cpp/include/cudf/io/experimental/deletion_vectors.hppcpp/src/io/parquet/experimental/deletion_vectors.cucpp/tests/io/parquet_deletion_vectors_test.cpp
jihoonson
left a comment
There was a problem hiding this comment.
LGTM. Thanks @mhaseeb123
|
/merge |
c16794f
into
NVIDIA:release/26.08
) First of all, I apologize for making a PR targeting the 26.08 branch. I understand the code freeze has taken effect since yesterday, but wish this change can still be shipped in the 26.08 release. ## Description This PR adds the JNI bridge for the recent change in #23402 to support the retention filter for the deletion vector. Previously, it was assumed that deletion vectors represent only the rows deleted. With #23402 and this change, now Java applications can pass the correct filter type for deletion vectors. ## Checklist - [x] I am familiar with the [Contributing Guidelines](https://github.com/rapidsai/cudf/blob/HEAD/CONTRIBUTING.md). - [x] New or existing tests cover these changes. - [x] The documentation is up to date with these changes.
…ors for OSS delta [databricks] (#15368) Fixes #15326. ### Description The CDF read with deletion vectors currently fails. Two things were missing to support this case: - The `IF_NOT_CONTAINED` row index filter type support. The Delta CDC reader can use this type of row index filter. - Inline deletion vector support. The CDC reader creates inline deletion vectors. This PR adds those supports based on NVIDIA/cudf#23402 for OSS Delta. The plugin now can load inline deletion vectors and process the `IF_NOT_CONTAINED` filter properly with all 3 Delta readers. Note that the issue exists only with the native readers (`GpuDeltaParquetFileFormatBase2`). The legacy reader (`GpuDeltaParquetFileFormatBase`) does not have this issue. Databricks readers have the same issue, and will be fixed in #15365. ### Checklists Documentation - [ ] Updated for new or modified user-facing features or behaviors - [x] No user-facing change Testing - [x] Added or modified tests to cover new code paths - [ ] Covered by existing tests (Please provide the names of the existing tests in the PR description.) - [ ] Not required Performance - [ ] Tests ran and results are added in the PR description - [ ] Issue filed with a link in the PR description - [x] Not required --------- Signed-off-by: Jihoon Son <ghoonson@gmail.com>
Description
This PR enables the experimental parquet reader to now also support retention vectors in addition to deletion vectors
Checklist