arminazimi opened a new pull request, #7004: URL: https://github.com/apache/datafusion-comet/pull/7004
## Which issue does this PR close? Closes #6983 ## Rationale for this change The four generic split implementations in `native/spark-expr/src/string_funcs/split.rs` each ended with the same block: finish the values builder, select `Utf8`/`LargeUtf8` from `O::IS_LARGE`, build the `item` field, and construct a `GenericListArray`. They differ only in the per-row push call and in how the null buffer is produced (cloned input nulls vs. a `NullBufferBuilder`). Keeping a single shared helper avoids future drift between these paths while preserving their existing behavior. A change to the list construction, such as the `item` field's nullability, previously had to be made in four places. ## What changes are included in this PR? - Add a private `finish_split_list` helper that builds the list output from the collected offsets, the values builder, and a caller-supplied null buffer. - Update `split_generic`, `split_sql_generic_scalar`, `split_sql_generic_scalar_array`, and `split_sql_generic_array` to call it, removing the four duplicated blocks. - Preserve the existing per-row loops, offset bookkeeping, `Utf8`/`LargeUtf8` item-type selection, and null handling: the first two paths pass `string_array.nulls().cloned()`, the last two keep their `NullBufferBuilder` and pass `nulls.finish()`. - Add focused tests for the output paths that previously constructed the array inline: array input with a scalar delimiter, array input with an array delimiter, and the `LargeUtf8` scalar with a delimiter array. The last one asserts the resulting `LargeList` has `i64` offsets, a non-nullable `item` field of type `LargeUtf8`, and a NULL row for the NULL delimiter. No public APIs, output types, or splitting behavior change. Each variant still chooses its own null predicate; the helper only constructs the output array. ## How are these changes tested? Run from `native/`, with Rust 1.99.0 (stable) and JDK 17 on macOS arm64: - `cargo fmt --all -- --check` — passes with no changes required. - `cargo check -p datafusion-comet-spark-expr --tests` — passes. - `cargo clippy -p datafusion-comet-spark-expr --lib --tests --bench split -- -D warnings` — passes with no warnings. - `cargo test -p datafusion-comet-spark-expr --lib string_funcs::split` — `test result: ok. 19 passed; 0 failed`. - `cargo test -p datafusion-comet-spark-expr --lib` — `test result: ok. 1215 passed; 0 failed`. The new tests are `test_split_sql_array_scalar_delimiter_with_nulls`, `test_split_sql_array_string_and_array_delimiter`, and `test_split_sql_large_utf8_scalar_string_array_delimiter`. Existing split tests (`test_split_basic`, `test_split_with_limit`, `test_split_with_nulls`, `test_split_limit_positive`, `test_split_limit_zero`, `test_split_limit_negative`, and the rest) continue to pass and cover the regex path, so no additional regex test was added. The JVM-backed Spark suites were not run locally; this change only touches Rust list construction in the `spark-expr` crate, which is covered by the `spark-expr` unit suite above. -- 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]
