andygrove opened a new issue, #6977: URL: https://github.com/apache/datafusion-comet/issues/6977
### What is the problem the feature request solves? In `native/shuffle/src/spark_unsafe/row.rs`, `append_struct_fields_field_major` and `append_nested_struct_fields_field_major` are near copies: - [`append_nested_struct_fields_field_major` (L374-L592)](https://github.com/apache/datafusion-comet/blob/7d8d9b1544b734c85a93cca2cc9d5380ad70accd/native/shuffle/src/spark_unsafe/row.rs#L374-L592), cognitive complexity 67 - [`append_struct_fields_field_major` (L817-L1052)](https://github.com/apache/datafusion-comet/blob/7d8d9b1544b734c85a93cca2cc9d5380ad70accd/native/shuffle/src/spark_unsafe/row.rs#L817-L1052), cognitive complexity 71 Each declares an identical local `process_field!` macro and repeats the same per-type match arms (Boolean through Time64, then hand-expanded Binary/Utf8/Decimal128/Struct/List/Map). They differ only in how the row is located: `read_row_at!` plus `parent_row.get_struct(...)` in one, address/size slices in the other. Within each function, the Binary, Utf8 and Decimal128 arms also hand-expand the same null-check/append loop that `process_field!` already implements. Adding support for a type, or fixing null handling, has to be done in both functions and in several arms of each. The field-major layout is deliberate for performance, but the duplication is not needed for that. ### Describe the potential solution 1. Let `process_field!` (or a generic helper taking a getter closure) handle Binary (`get_binary`), Utf8 (`get_string`) and Decimal128 (a closure capturing the precision), so those arms become one line each. 2. Unify the two functions over one row source: for the top-level case, collect `(addr, size, is_null)` slices in one pass over `read_row_at!` plus `get_struct`, then call the nested implementation. `append_struct_fields_field_major` then shrinks to building those slices and the validity. 3. Optionally bundle `row_addresses_ptr/row_sizes_ptr/row_start/row_end` into a small struct to cut the eight-parameter signature. A shuffle benchmark before and after would confirm there is no regression. ### Additional context Line references are pinned to commit 7d8d9b1544b7. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
