JNI bridge for the retention filter support for deletion vectors - #23423
Conversation
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughDeletion-vector metadata now records retention mode, validates that all vectors share one mode, and propagates it through Java, JNI, and native Parquet readers. Tests cover both modes and reject mixed deletion and retention vectors. ChangesRetention vector support
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
java/src/main/native/src/DeletionVectorJni.cpp (1)
182-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRun clang-format on the new JNI call sites.
The added
make_deletion_vector_info(...)calls are manually wrapped into overlong lines. Please format the file with clang-format before merging.As per coding guidelines, C++ and CUDA code must be formatted with clang-format.
Also applies to: 273-273
🤖 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 `@java/src/main/native/src/DeletionVectorJni.cpp` at line 182, Run clang-format on the new make_deletion_vector_info JNI call sites in DeletionVectorJni.cpp, including both referenced call sites, and preserve the existing arguments and behavior while applying the project’s standard C++ formatting.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.
Nitpick comments:
In `@java/src/main/native/src/DeletionVectorJni.cpp`:
- Line 182: Run clang-format on the new make_deletion_vector_info JNI call sites
in DeletionVectorJni.cpp, including both referenced call sites, and preserve the
existing arguments and behavior while applying the project’s standard C++
formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1789a313-44bf-4d14-9352-d0a2d248170c
📒 Files selected for processing (3)
java/src/main/java/ai/rapids/cudf/DeletionVector.javajava/src/main/native/src/DeletionVectorJni.cppjava/src/test/java/ai/rapids/cudf/DeletionVectorTableTest.java
mhaseeb123
left a comment
There was a problem hiding this comment.
Approving C++ changes
revans2
left a comment
There was a problem hiding this comment.
I assume it is okay that we are breaking backwards compatibility on one one API. I just wanted to check.
|
@GregoryKimball approved the (small) breaking change on slack -- merging this in |
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