Add floating point type support to Parquet variant field extraction - #23075
Conversation
abigalekim
added
feature request
|
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:
📝 WalkthroughWalkthroughThis PR extends experimental VARIANT casting to support FLOAT32/FLOAT64 primitive types, updates public documentation, broadens ChangesVARIANT float casting support
Estimated code review effort: 3 (Moderate) | ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/src/io/parquet/experimental/variant_extract.cu (1)
578-615: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKernel/functor duplication between int and float cast paths.
cast_variant_float_kerneland the floatoperator()incast_variant_fnare near-identical copies ofcast_variant_int_kernel(Line 549) and the intoperator()(Line 678), differing only in the decode function invoked (decode_int<T>vsdecode_float<T>). Consider a single templated kernel/functor parameterized on the decode function to avoid maintaining two copies of the same launch/null-handling logic going forward.♻️ Sketch of a unified kernel
-template <typename T> -CUDF_KERNEL __launch_bounds__(block_size) void cast_variant_int_kernel( - cudf::lists_column_device_view values, device_span<T> d_output, bitmask_type* d_null_mask) -{ ... decode_int<T> ... } - -template <typename T> -CUDF_KERNEL __launch_bounds__(block_size) void cast_variant_float_kernel( - cudf::lists_column_device_view values, device_span<T> d_output, bitmask_type* d_null_mask) -{ ... decode_float<T> ... } +template <typename T, typename DecodeFn> +CUDF_KERNEL __launch_bounds__(block_size) void cast_variant_fixed_width_kernel( + cudf::lists_column_device_view values, device_span<T> d_output, bitmask_type* d_null_mask, + DecodeFn decode) +{ + // shared body; calls decode(val) instead of decode_int<T>/decode_float<T> +}Also applies to: 698-717
🤖 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/variant_extract.cu` around lines 578 - 615, The float casting path duplicates the same launch and null-handling logic used by the int path, so update `cast_variant_float_kernel` and the float `operator()` in `cast_variant_fn` to share the same implementation as `cast_variant_int_kernel` and its int functor. Refactor the kernel/functor pair into a single templated path parameterized by the decode routine (for example, `decode_int<T>` vs `decode_float<T>`) while preserving the existing row iteration, null-mask updates, and output assignment behavior.
🤖 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 `@cpp/src/io/parquet/experimental/variant_extract.cu`:
- Around line 578-615: The float casting path duplicates the same launch and
null-handling logic used by the int path, so update `cast_variant_float_kernel`
and the float `operator()` in `cast_variant_fn` to share the same implementation
as `cast_variant_int_kernel` and its int functor. Refactor the kernel/functor
pair into a single templated path parameterized by the decode routine (for
example, `decode_int<T>` vs `decode_float<T>`) while preserving the existing row
iteration, null-mask updates, and output assignment behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 654845a8-137a-4729-9b5a-b05e8d420e8c
📒 Files selected for processing (3)
cpp/include/cudf/io/experimental/variant.hppcpp/src/io/parquet/experimental/variant_extract.cucpp/tests/io/experimental/variant_extract_test.cpp
| cuda::std::is_same_v<T, int8_t> || cuda::std::is_same_v<T, int16_t> || | ||
| cuda::std::is_same_v<T, int32_t> || cuda::std::is_same_v<T, int64_t>; |
There was a problem hiding this comment.
Even this could be cudf::is_integral_not_bool<T>() and cudf::is_signed<T>() (pending checking if __int128 is excluded from this)
There was a problem hiding this comment.
I don't think int128 is included in support, does this make the check refactorable?
There was a problem hiding this comment.
Should cudf::is_signed() include __int128? If so, this check is fragile.
There was a problem hiding this comment.
Is is_variant_int being use elsewhere besides computing is_variant_primitive. If not, we can directly compute is_variant_primitive.
Also, yes we can write is_variant_int` as:
template <typename T>
constexpr bool is_variant_int = cudf::is_integral_not_bool<T>() and cudf::is_signed<T>() and not cuda::std::is_same_v<T, __int128_t>();Not sure if it's any better than the current
| cuda::std::is_same_v<T, int8_t> || cuda::std::is_same_v<T, int16_t> || | ||
| cuda::std::is_same_v<T, int32_t> || cuda::std::is_same_v<T, int64_t>; |
There was a problem hiding this comment.
Should cudf::is_signed() include __int128? If so, this check is fragile.
Co-authored-by: Vukasin Milovanovic <vmilovanovic@nvidia.com>
|
/ok to test 9030bda |
|
/ok to test 07a767d |
|
/ok to test f2efc36 |
|
/ok to test 6ab6fd9 |
|
/merge |
7 participants
Description
Adds floating point type support to Apache Parquet variant field extraction.
Checklist