Add opt-in per-row status column to VARIANT extraction and cast APIs - #23560
Add opt-in per-row status column to VARIANT extraction and cast APIs#23560abigalekim wants to merge 35 commits into
Conversation
abigalekim
added
non-breaking
|
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
WalkthroughThe PR adds a public VARIANT operation-status enum, optional status inputs and outputs, status-aware extraction and casting, JNI call updates, and comprehensive status tests. ChangesVARIANT status-aware extraction and casting
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to This opt-in API enhancement has no actionable merge-blocking risk identified at the current head and is merge-ready after normal checks and review. Possibly related PRs
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.
Actionable comments posted: 5
🧹 Nitpick comments (4)
cpp/src/io/parquet/experimental/variant_extract.cu (3)
1502-1503: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove the unused
placeholder_dev_view.
placeholder_dev_viewis never read. Lines 1504-1506 create a second device view from the sameplaceholder_col. The unused view performs an extra device allocation and host-to-device copy on everycast_variantcall.♻️ Proposed fix
auto placeholder_col = make_empty_column(data_type{type_id::UINT8}); - auto placeholder_dev_view = column_device_view::create(*placeholder_col, stream); auto incoming_dev_view = (incoming_status != nullptr)🤖 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 1502 - 1503, Remove the unused placeholder_dev_view declaration from the cast_variant setup, while retaining placeholder_col and the later device view created from it.
424-463: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefine the legacy resolvers in terms of the status-aware resolvers.
find_key_in_metadata_with_status(Lines 424-463) duplicatesfind_key_in_metadata(Lines 232-272) statement for statement. The same duplication exists forlocate_object_field_with_status(Lines 468-519) versuslocate_object_field(Lines 300-351), and forlocate_array_element_with_status(Lines 522-569) versuslocate_array_element(Lines 367-409). Each pair contains the same bounds checks and the same offset arithmetic.Keep one implementation per resolver and make the value-only version a thin wrapper. A later fix to the parsing logic then applies once instead of twice.
♻️ Proposed wrapper form
-__device__ cuda::std::optional<size_type> find_key_in_metadata(device_span<uint8_t const> meta, - cudf::string_view key) -{ - // ... duplicated body ... -} +__device__ cuda::std::optional<size_type> find_key_in_metadata(device_span<uint8_t const> meta, + cudf::string_view key) +{ + return find_key_in_metadata_with_status(meta, key).first; +}Apply the same pattern to
locate_object_fieldandlocate_array_element, returning only the span.🤖 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 424 - 463, Refactor the legacy resolvers find_key_in_metadata, locate_object_field, and locate_array_element to call their corresponding status-aware implementations instead of duplicating parsing logic. Have each value-only wrapper return only the span/value from the status-aware result while preserving existing behavior and status handling in the *_with_status functions.
1128-1143: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueRemove the unused
mrparameter frommake_status_columnand its call sites.variant_operation_statusalready usesuint8_t, so the status buffer andUINT8column are consistent.🤖 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 1128 - 1143, Remove the unused mr parameter from make_status_column and update every call site to stop passing it, while preserving the existing status buffer construction and UINT8 column type.cpp/tests/io/experimental/variant_extract_test.cpp (1)
1769-1797: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd sliced, multi-block, and non-ASCII UTF-8 status coverage.
The new status tests use 1 to 4 rows and ASCII strings only. Three required cases are missing for the changed status paths:
- Non-ASCII UTF-8 in
StringStatusTracking, socast_status_for_stringand the sizing pass are checked with multi-byte characters.- A sliced VARIANT column, so the status null mask is checked against a non-zero column offset.
- A row count above one block (
block_sizeis 256), so the grid-stride loops inlocate_variant_fields_with_status_kernelandcast_variant_primitive_status_kernelare checked with more than one block.Do you want me to generate these three test cases?
As per coding guidelines: "Tests must cover empty inputs, nulls, sliced columns, boundary and multi-block sizes, and non-ASCII UTF-8 for string tests".
🤖 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/variant_extract_test.cpp` around lines 1769 - 1797, Add coverage in CastVariantStatusTest for non-ASCII UTF-8 strings, a sliced VARIANT input with a non-zero offset, and a row count greater than the 256-row block size. Verify status values and null masks for each case, preserving existing null, mismatch, malformed, and success expectations.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/variant.hpp`:
- Around line 69-74: Reorder the parameters in get_variant_field and the two
other affected APIs in cpp/include/cudf/io/experimental/variant.hpp at lines
69-74, 100-106, and 127-133: place status_out, or incoming_status followed by
status_out, before stream and mr. Keep stream immediately before mr as the final
two parameters at every site, and update corresponding declarations or
definitions as needed.
In `@cpp/src/io/parquet/experimental/variant_extract.cu`:
- Around line 710-731: Update resolve_path_with_status after each array or
object lookup, before the intermediate-null check, to detect an empty resolved
span and return an empty value with missing_path status. This must cover
zero-length spans reported with success by locate_array_element_with_status so
status and non-status resolution produce the same null-mask behavior.
- Around line 1504-1507: Validate incoming_status in cast_variant before
creating incoming_dev_view: when non-null, require at least num_rows entries and
the expected uint8_t type, rejecting invalid inputs before apply_incoming_status
can index the column or its null mask. Preserve the existing placeholder-column
path when incoming_status is null.
- Around line 917-953: Update cast_status_for_primitive and cast_status_for_bool
to use one shared predicate listing every recognized primitive_type accepted by
variant_value_length, including DATE, decimal widths, UUID, timestamp/time
variants, and BINARY. Return type_mismatch for recognized primitive IDs that are
valid but not the requested type, while returning malformed_variant when the
requested type ID is recognized but its payload cannot be decoded, indicating
truncation. Keep unrecognized IDs mapped to malformed_variant and ensure both
classifiers use the same predicate.
- Around line 1165-1178: Initialize status buffers to zero before kernel
execution at all four allocation sites: the numerical and boolean paths near
status_data, the string path, and get_variant_field. Add a shared helper that
allocates the status data and null mask, zeroes the status buffer, and returns
both buffers; replace each direct uninitialized allocation with this helper
while preserving the existing make_status_column flow.
---
Nitpick comments:
In `@cpp/src/io/parquet/experimental/variant_extract.cu`:
- Around line 1502-1503: Remove the unused placeholder_dev_view declaration from
the cast_variant setup, while retaining placeholder_col and the later device
view created from it.
- Around line 424-463: Refactor the legacy resolvers find_key_in_metadata,
locate_object_field, and locate_array_element to call their corresponding
status-aware implementations instead of duplicating parsing logic. Have each
value-only wrapper return only the span/value from the status-aware result while
preserving existing behavior and status handling in the *_with_status functions.
- Around line 1128-1143: Remove the unused mr parameter from make_status_column
and update every call site to stop passing it, while preserving the existing
status buffer construction and UINT8 column type.
In `@cpp/tests/io/experimental/variant_extract_test.cpp`:
- Around line 1769-1797: Add coverage in CastVariantStatusTest for non-ASCII
UTF-8 strings, a sliced VARIANT input with a non-zero offset, and a row count
greater than the 256-row block size. Verify status values and null masks for
each case, preserving existing null, mismatch, malformed, and success
expectations.
🪄 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: 65f72fc2-c35e-4565-9370-175f019ef09a
📒 Files selected for processing (4)
cpp/include/cudf/io/experimental/variant.hppcpp/include/cudf/io/experimental/variant_spec.hppcpp/src/io/parquet/experimental/variant_extract.cucpp/tests/io/experimental/variant_extract_test.cpp
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/io/experimental/variant_extract_test.cpp`:
- Line 1495: Update both terminal VARIANT-null tests around get_variant_field to
assert the returned bytes, not only that the output row is non-null. Build the
expected one-row list<uint8> column from enc_null() and compare it with *got in
both the status_out and no-status_out cases, preserving coverage of the public
raw-byte contract.
🪄 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: bae4b030-156e-46b5-a69d-3c3340a72534
📒 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
🚧 Files skipped from review as they are similar to previous changes (2)
- cpp/include/cudf/io/experimental/variant.hpp
- cpp/src/io/parquet/experimental/variant_extract.cu
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cpp/tests/io/experimental/variant_extract_test.cpp (1)
1973-2020: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd end-to-end malformed-status coverage.
extract_variant_fieldhas no test forST_MALFORMED. The wrapper must chain the malformed status fromget_variant_fieldthroughcast_variant. Add a case with malformed metadata and assertST_MALFORMEDplus a null output row.🤖 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/variant_extract_test.cpp` around lines 1973 - 2020, Add an end-to-end malformed-metadata test alongside VariantNullStatus, TypeMismatchStatus, and MissingNestedPathStatus using the existing variant builders and extract_variant_field flow. Construct a malformed variant metadata case, request a compatible output type, then assert the returned status contains ST_MALFORMED and the extracted column has one null row.
🤖 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/tests/io/experimental/variant_extract_test.cpp`:
- Around line 1470-1475: Update the test cases around cast_variant calls at the
status-column setups near lines 1449, 1472, 1483, and 1494 to retain each
nullable status column’s released unique_ptr in a local variable, then obtain
the status view from that variable. Keep the owning unique_ptr alive until
cast_variant completes instead of releasing it in the same expression as view
creation.
---
Nitpick comments:
In `@cpp/tests/io/experimental/variant_extract_test.cpp`:
- Around line 1973-2020: Add an end-to-end malformed-metadata test alongside
VariantNullStatus, TypeMismatchStatus, and MissingNestedPathStatus using the
existing variant builders and extract_variant_field flow. Construct a malformed
variant metadata case, request a compatible output type, then assert the
returned status contains ST_MALFORMED and the extracted column has one null row.
🪄 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: 8af860dc-3e79-4689-bf31-3e0ed6a569dd
📒 Files selected for processing (2)
cpp/src/io/parquet/experimental/variant_extract.cucpp/tests/io/experimental/variant_extract_test.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/src/io/parquet/experimental/variant_extract.cu
|
/ok to test 3cb0e7f |
|
/ok to test f743b73 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cpp/include/cudf/io/experimental/variant_spec.hpp (2)
52-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse one enumerator naming style in this header.
variant_basic_typeandvariant_primitive_typeuse upper-case enumerators.variant_operation_statususes lower-case enumerators. All three are public enums in the same header. Rename the new enumerators to upper case for consistency, or state the deviation explicitly. The enum is part of the public API, so a later rename breaks callers.♻️ Proposed rename
enum class variant_operation_status : uint8_t { - success, - row_null, ///< the SQL row itself was null (no VARIANT data to decode) - missing_path, - variant_null, - type_mismatch, - malformed_variant, - overflow, - invalid_conversion, + SUCCESS, ///< the operation resolved and decoded the value + ROW_NULL, ///< the SQL row itself was null (no VARIANT data to decode) + MISSING_PATH, ///< the path did not resolve in this row + VARIANT_NULL, ///< the resolved value is an encoded VARIANT null + TYPE_MISMATCH, ///< the encoded type is not compatible with the requested type + MALFORMED_VARIANT, ///< the encoding is truncated or structurally invalid + OVERFLOW, ///< the decoded value does not fit the requested type + INVALID_CONVERSION, ///< the value cannot be converted to the requested type };Note that
OVERFLOWis a common macro name on some toolchains. If you adopt upper case, prefer a distinct spelling such asVALUE_OVERFLOW.🤖 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/include/cudf/io/experimental/variant_spec.hpp` around lines 52 - 61, Rename the public enumerators in variant_operation_status to match the uppercase style used by variant_basic_type and variant_primitive_type, using a distinct spelling such as VALUE_OVERFLOW for overflow to avoid macro collisions. Update all references to the renamed status values throughout the codebase, preserving their semantics.
59-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove
overflowandinvalid_conversionfromvariant_operation_status.No current producer emits these values. They add unreachable states to the public enum.
🤖 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/include/cudf/io/experimental/variant_spec.hpp` around lines 59 - 60, Remove the overflow and invalid_conversion enumerators from the variant_operation_status enum, leaving the remaining status values unchanged.
🤖 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/variant_spec.hpp`:
- Around line 53-61: Document every enumerator in variant_operation_status by
adding a concise ///< comment to success, missing_path, variant_null,
type_mismatch, malformed_variant, overflow, and invalid_conversion, while
preserving the existing row_null documentation.
In `@cpp/src/io/parquet/experimental/variant_extract.cu`:
- Around line 774-791: In cpp/src/io/parquet/experimental/variant_extract.cu
lines 774-791 within cast_variant_primitive_kernel, after incoming success,
check the value null mask; for a clear bit, write T{}, set op_status::row_null,
and continue. Apply the same check in cast_variant_string_fn::operator() at
lines 879-895, setting d_sizes[row] = 0 on the sizing pass, and in the boolean
thrust::for_each lambda at lines 1018-1032, writing false; record row_null in
both sibling paths.
---
Nitpick comments:
In `@cpp/include/cudf/io/experimental/variant_spec.hpp`:
- Around line 52-61: Rename the public enumerators in variant_operation_status
to match the uppercase style used by variant_basic_type and
variant_primitive_type, using a distinct spelling such as VALUE_OVERFLOW for
overflow to avoid macro collisions. Update all references to the renamed status
values throughout the codebase, preserving their semantics.
- Around line 59-60: Remove the overflow and invalid_conversion enumerators from
the variant_operation_status enum, leaving the remaining status values
unchanged.
🪄 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: Pro Plus
Run ID: 537d864b-1ed2-429a-92b5-dc7b21f9a756
📒 Files selected for processing (5)
cpp/include/cudf/io/experimental/variant.hppcpp/include/cudf/io/experimental/variant_spec.hppcpp/src/io/parquet/experimental/variant_extract.cucpp/tests/io/experimental/variant_extract_test.cppjava/src/main/native/src/VariantUtilsJni.cpp
|
/ok to test d00a0c3 |
nartal1
left a comment
nartal1
left a comment
There was a problem hiding this comment.
Thanks @abigalekim for addressing the review comments. Just have one nit.
|
/ok to test ada04d9 |
| __device__ cuda::std::pair<cuda::std::optional<size_type>, op_status> find_key_in_metadata( | ||
| device_span<uint8_t const> meta, cudf::string_view key) |
There was a problem hiding this comment.
nit/microoptimisation: cuda::std::optional<size_type> uses 8 bytes to store 4 bytes of payload and 1 bit of validity. the uint8_t op_status is then padded by pair to 4 bytes.
Perhaps this is worrying about nothing, but would it make sense to use our own encoding in a single 64bit int? We could store the value in the low 32 bits, steal 1 bit for the validity and then have the remaining 31 bits for the op_status.
So encode would be:
val | (op_status << 33) | (valid << 32)
And decode would be:
val = packed & 0xFFFFFFFF;
valid = (packed >> 32) & 0x1;
op_status = (packed >> 33) & 0xFF;
(untested).
Probably this doesn't matter because these kernels are big, but maybe it would help with register pressure.
wence-
left a comment
wence-
left a comment
There was a problem hiding this comment.
Some small comments, for fixup, but looks good, I think.
| // Returns true for every primitive_type ID that variant_value_length recognises (i.e. every ID | ||
| // that does not fall through to its `default: return nullopt` branch). Used by both | ||
| // cast_status_for_primitive and cast_status_for_bool so the two classifiers stay in sync. |
There was a problem hiding this comment.
Please adapt this comment to fix the incorrect "return nullopt" statement. Also, best not to say where it is used because that kind of documentation is best handled by just using clangd/searching the current codebase.
| * | ||
| * Three cases after `decode_primitive<T>` returns nullopt and the basic type is PRIMITIVE: | ||
| * - Header type == primitive_type_for<T>() but payload too short → truncated → malformed_variant | ||
| * - Header type is a different recognized type → type_mismatch | ||
| * - Header type is unrecognized → malformed_variant |
There was a problem hiding this comment.
This is also liable to going out of date. Does it instead suffice to document the semantics of the op_status enum members?
|
|
||
| // Status columns are always non-nullable: SQL-null rows are represented by the row_null enum | ||
| // value rather than by a null bit, so no null mask needs to be allocated or counted. | ||
| std::unique_ptr<column> make_status_column(rmm::device_buffer status_data, size_type num_rows) |
There was a problem hiding this comment.
nit:
| std::unique_ptr<column> make_status_column(rmm::device_buffer status_data, size_type num_rows) | |
| std::unique_ptr<column> make_status_column(rmm::device_buffer&& status_data, size_type num_rows) |
So that the caller doesn't accidentally induce a copy?
4 participants
Description
Adds an opt-in variant_operation_status output to
get_variant_field,cast_variant, andextract_variant_field. All of these APIs now have an optional input column that will contain the statuses of the call per row. Callers that do not pass a status_out pointer retain the existing value-only behavior with no additional allocation. Solves the issue mentioned in #23467.Checklist