tlfua opened a new pull request, #7450: URL: https://github.com/apache/datafusion-comet/pull/7450
## Which issue does this PR close? Closes #7245. Related to #5889: as discussed in [this comment](https://github.com/apache/datafusion-comet/pull/5889#issuecomment-6425145597), the padding fix is split out of that PR so it can land before the struct-typed scalar subquery support. ## Rationale for this change The string argument of `lpad`/`rpad` can reference columns and still evaluate to a scalar for a whole batch. For example, `IF(id > 0, (SELECT max(s) FROM t), s)` returns a scalar when every row in the batch takes the subquery branch. The serde only rejects a *literal* string, so this shape reaches the native `read_side_padding` kernel. Every arm there expects the string to be an array, so the query fails with: ``` Unsupported arguments [Scalar(Utf8("b")), ...] for function rpad/lpad/read_side_padding ``` @sunchao raised this in [the #5889 review](https://github.com/apache/datafusion-comet/pull/5889#discussion_r4321296723) and suggested handling a runtime scalar string in the kernel. ## What changes are included in this PR? - `spark_read_side_padding2` gets one new arm for a scalar string argument. It expands the scalar to an array sized to the other array arguments (or one row if every argument is a scalar), then reuses the existing arms. This keeps all of the existing semantics: truncation, `len <= 0`, empty pad, and dictionary/`LargeUtf8` handling. - When every argument is a scalar, the result is returned as a scalar. - A NULL scalar string yields NULL, matching Spark's null-intolerant `StringLPad`/`StringRPad`. No serde change. Literal strings and non-literal padding still use codegen dispatch, as documented in `expressions.md`. ## How are these changes tested? New unit tests in `read_side_padding.rs`: - scalar string with a scalar length, for `lpad`, `rpad`, `rpad` truncation, and `read_side_padding` - scalar string with an array length - NULL scalar string - `LargeUtf8` scalar string These failed on `main` with the `Unsupported arguments` error above, and pass with this change. SQL file tests in `expressions/string/string_lpad.sql` and `string_rpad.sql`, using `expect_native(lpad|rpad)`: - `IF` and `CASE WHEN` with the subquery branch, with 3 arguments - 2-argument form - subquery returning NULL - mixed batch, where only some rows take the subquery branch so the string stays an array Locally (macOS, Rust 1.99.1): - `cargo test -p datafusion-comet-spark-expr --lib read_side_padding`: 16 passed - `cargo clippy -p datafusion-comet-spark-expr --all-targets -- -D warnings`: clean - `cargo fmt --all -- --check`: clean I did not run `CometSqlFileTestSuite` locally. The SQL file tests run in the fork CI (Spark 4.1 default profile): https://github.com/tlfua/datafusion-comet/actions/runs/41571963649. I'll mark this ready for review once it is green. AI disclosure: the change, the tests and this description were written with an AI coding assistant, and reviewed by me. -- 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]
