Consolidate protobuf scan and decode interfaces - #4836
Conversation
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
|
NOTE: release/26.08 has been created from main. Please retarget your PR to release/26.08 if it should be included in the release. |
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Greptile SummaryThis PR consolidates the protobuf scan and decode interfaces by unifying six separate scanning kernels under a single
Confidence Score: 5/5Safe to merge — well-scoped refactoring with one intentional and tested behavioral change, 154 passing tests, and no Java API change. The changes consolidate previously duplicated scan/decode paths without altering observable outputs for valid inputs. The one intentional change (silently skipping a mismatched singular field inside a nested message instead of aborting) is explicitly documented, matches protobuf-java behavior, and is covered by two new test cases. Kernel argument ordering and stride indexing were verified to be internally consistent across all call sites. Files Needing Attention: No files require special attention. The Important Files Changed
Reviews (10): Last reviewed commit: "Merge branch 'main' into protobuf-refact..." | Re-trigger Greptile |
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
| field_location location) { | ||
| field_locations[f] = location; | ||
| auto record_singular = [&](int f, field_location location) { | ||
| field_locations[fields.lookup.data[f].output_index] = location; |
There was a problem hiding this comment.
That's not quite what I asked. Have we tried benchmarks to confirm that this is actually a performance issue either way? Your new code is simpler, so readability-wise it's an improvement regardless (though we might want to make the same change to scan_nested_message_fields_kernel and clean up the unused output_index assignments in make_field_descriptors when output_indices is empty).
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
| if (!field.enum_valid_values.empty()) { | ||
| validate_enum_and_propagate_rows( | ||
| out, valid, field.enum_valid_values, decode_ctx, {num_items, top_row_indices}, stream); | ||
| } |
There was a problem hiding this comment.
[Really optional] This is actually the only difference between the INT32 case and extract_and_build_scalar_field_column<int32_t>. I wish we could add this as an if constexpr (std::is_same_v<T, int32_t>) block into extract_and_build_scalar_field_column, and replace the INT32 case as well, but even if we inline extract_and_build_scalar_column, as suggested in #4836 (comment), we'd still need to propagate top_row_indices, so let's defer and think on how we could consolidate it in a future PR.
There was a problem hiding this comment.
Agreed and let's defer this.
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
igorpeshansky
left a comment
There was a problem hiding this comment.
LGTM
with some minor remaining cleanup items.
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
|
build |
|
build |
Summary
This PR consolidates the protobuf scan and decode interfaces without changing the Java API.
scan_message_field_locations<MismatchPolicy>the common message walker used byscan_all_fields_kernel,count_repeated_fields_kernel,scan_all_field_occurrences_in_message, andscan_nested_message_fields_kernel; callers provide descriptor lookup pluson_singularandon_repeatedcallbacks throughmessage_scan_contextdecode_varint_valueanddecode_fixed_value, which are now shared byextract_varint_kernel,extract_fixed_kernel,extract_varint_batched_kernel, andextract_fixed_batched_kernelextract_integer_into_buffersfrom bothextract_typed_columnandbuild_repeated_scalar_columnprotobuf_field_decode_request,required_field_input_view, andprotobuf_value_domain_viewenum_value_device_view,enum_domain_device_view, andenum_string_lookup_device_viewrepeated_field_workdirectly throughbuild_repeated_scalar_column,build_repeated_string_column,build_repeated_enum_string_column, andbuild_repeated_child_list_column; the object keepsschema_idx,total_count, LISToffsets, and theoccurrencesscratch buffer togethervalidate_nonempty_repeated_field_workscalar_grouplaunch_scan_all_fields,launch_count_repeated_fields, andlaunch_scan_all_field_occurrencesfromd_in.size()ProtobufTestcases to buildexpectedStructandexpectedValuescolumns and compare them withactualStructviaAssertUtils.assertStructColumnsAreEqual, replacing manualcopyToHost()child traversal and spot checks.The refactor removes 246 net lines from the six production files. It does not add protobuf field types or change the Java API.
Intentional behavior change
scan_nested_message_fields_kernelnow callsscan_message_field_locations<wire_type_mismatch_policy::skip>. When a singular field inside a nested message has a wire type different fromfield_descriptor::expected_wire_type, the field is skipped and remains absent, while later sibling fields continue decoding.This matches protobuf-java, which stores the mismatched field in the nested message's
UnknownFieldSet, and Spark CPU, whereProtobufDataToCatalystchecksDynamicMessage.getUnknownFieldsonly on the root message.Top-level behavior is unchanged:
scan_all_fields_kerneluseswire_type_mismatch_policy::report_errorcount_repeated_fields_kerneluseswire_type_mismatch_policy::report_error_and_skipfor fields fromnested.schema_lookupwire_type_mismatch_policy::skipPart of NVIDIA/cudf-spark#14069.
Testing
com.nvidia.spark.rapids.jni.ProtobufTest(154 tests)testNestedSingularWrongWireType_FailfastSkipsMismatchedFieldtestNestedSingularWrongWireType_PermissiveSkipsMismatchedField