[FEA] Support input of dictionary-encoded types in Transforms - #21552
Conversation
|
This is awesome. |
Co-authored-by: David Wendt <45795991+davidwendt@users.noreply.github.com>
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
Co-authored-by: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com>
Co-authored-by: Nghia Truong <7416935+ttnghia@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds shared child-column index constants, dictionary-encoded device column access, and dictionary-aware JIT transform reflection and validation. Public docs and integration tests now cover dictionary inputs for CUDA and PTX transform paths. ChangesDictionary support in JIT transform
Estimated code review effort: 4 (Complex) | ~50 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/column/column_device_view_base.cuh`:
- Around line 518-521: The dictionary decode path in
column_device_view_core::element is using the raw element_index from the indices
child and ignores the parent slice offset set by slice(), so sliced dictionary
views can return the wrong key. Update the dictionary branch to read the indices
child at element_index + offset() (mirroring the string-path handling) while
keeping the existing child() and keys lookup logic intact.
In `@cpp/include/cudf/transform.hpp`:
- Around line 99-101: The PTX capability docs in transform.hpp overstate support
by saying PTX accepts all fixed-width inputs and outputs, but perform_checks()
in transform.cu still only allows integral/floating-point value types and
dictionary inputs whose values are integral/floating-point. Update the
documentation around the transform API (including the referenced input/output
type comments) to match the actual runtime constraints, explicitly limiting PTX
to the value types that perform_checks() permits and avoiding mention of
unsupported fixed-width types like timestamps, durations, and fixed-point.
In `@cpp/tests/transform/integration/unary_transform_test.cpp`:
- Around line 417-459: The TransformDictionaryString coverage only tests full
dictionary columns, so add a sliced dictionary input case to exercise the
offset-sensitive path through dictionary children/device views. Extend
UnaryOperationIntegrationTest::TransformDictionaryString (and the related
dictionary transform tests in the same area) by creating a sliced strings
column, encoding it, and verifying transform_extended on the slice matches the
expected sliced input values for both nullable and non-nullable cases.
🪄 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: 23ca6781-4c6b-4d56-a1a7-6d12d7346480
📒 Files selected for processing (8)
cpp/include/cudf/column/column_child_offsets.hppcpp/include/cudf/column/column_device_view_base.cuhcpp/include/cudf/dictionary/dictionary_column_view.hppcpp/include/cudf/transform.hppcpp/src/jit/column_accessor.cuhcpp/src/jit/helpers.cppcpp/src/transform/transform.cucpp/tests/transform/integration/unary_transform_test.cpp
@ttnghia, that will be testing a behaviour independent of dictionary transforms. Is there a particular reason why we have to test specifically for zero-length dictionaries?
PTX transforms with dictionary inputs are now supported |
Just to make sure the code cover edge cases. |
|
/merge |
Description
This pull-request adds support for dictionary-encoded types in the transform and filter APIs.
Checklist