fix: return NULL from rpad/lpad when the length column is NULL instead of panicking - #5680
fix: return NULL from rpad/lpad when the length column is NULL instead of panicking#5680peterxcli wants to merge 1 commit into
Conversation
…d of panicking `spark_read_side_padding_internal` unwrapped every value of the length array, so `rpad(s, len)` / `lpad(s, len)` (and the 3-arg forms with a literal pad) panicked with `called Option::unwrap() on a None value` and failed the task as soon as the length column contained a NULL. Spark's `StringRPad` / `StringLPad` are null-intolerant: a NULL length yields a NULL row. Treat a NULL length like a NULL string and emit a null row, and size the output buffer from the non-null lengths only, since the values under null slots are unspecified. Non-null rows are unchanged. Closes apache#5672 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
sunchao
left a comment
There was a problem hiding this comment.
PR summary
Reviewed 6fc59544 against PR base 81d637b9. No actionable P1/P2 findings. The change fixes NULL length propagation in the shared padding routine and adds column-backed regressions for lpad and rpad.
The sizing pass now ignores physical values beneath invalid length slots. The output loop emits NULL when either the string or length is NULL, matching the maintained Spark 3.5 and 4.0 expression sources. Existing non-null padding and truncation logic is unchanged.
Validation and CI
Local focused validation passed 28 component checks: 12 head production tests, 8 base tests and 8 reviewer checks, including a deliberately expected original null-length panic. These compiled exact production modules against pinned Arrow/DataFusion types. They cover mixed and all-null lengths, invalid-slot storage, empty arrays, Unicode/LargeUtf8 input and unchanged argument-shape boundaries. This was not a full native workspace build or a local Spark/JNI run.
The existing Spark 3.5/Linux expression job and Spark 4.0/macOS expression job explicitly passed the new NULL-length regression and both padding SQL files. The Rust test job also succeeded. They ran on synthetic merge 2ced5b41, with the reviewed base/head as parents. The relevant implementation, serde, tests and lockfile match the head, but full-tree equivalence is not claimed.
At 13:54 UTC, 56 checks had succeeded, 9 were still running and 7 were skipped, with no failures.
Performance
The fix keeps the existing two passes over input rows and introduces no additional array materialization. Skipping invalid lengths avoids reserving space from unspecified values. The component buffer check is not a throughput or process-memory benchmark, so no measured speedup is claimed.
Design
Null propagation stays in the existing shared append loop. The supported array-length paths receive the same behavior without separate implementations for each function.
Abstraction and complexity
No new production abstraction, dependency or public interface is introduced. The nullable-length helper is small and confined to tests, and the SQL regressions extend the existing test files.
Which issue does this PR close?
Closes #5672.
Rationale for this change
rpad(s, len)/lpad(s, len)(and the 3-arg forms with a literal pad) run natively by default, andspark_read_side_padding_internalunwrapped every value of the length array. Any NULL in the length column panicked withcalled Option::unwrap() on a None valueand failed the task with aCometNativeException. Spark'sStringRPad/StringLPadare null-intolerant and return NULL for that row.What changes are included in this PR?
read_side_padding.rs: in the array-length path (used by both the[Array, Array]and[Array, Array, Scalar]arms) a NULL length now yields a NULL row, like a NULL string. The output-buffer sizing pass only looks at non-null lengths, since values under null slots are unspecified. Non-null rows are unchanged. The scalar-length and dictionary paths only ever receiveInt32(Some(_))(Spark folds NULL-literal arguments to NULL before Comet sees them) and are untouched.rpad/lpad(andread_side_padding) with a length array containingNone→ NULL for that row, other rows unchanged; null string + null length; all-null lengths.string_rpad.sql/string_lpad.sql: rows('hi', NULL, 'x')and(NULL, NULL, 'x')in the Parquet-backed table, plus acolumn + column + literalquery (rpad(s, len, 'x')) so the 3-arg array-length arm is covered natively.CometStringExpressionSuite:lpad/rpad with NULL lengthbuilds the rows explicitly (FuzzDataGeneratorcannot generate NULL integers, FuzzDataGenerator silently drops nulls for Boolean/Byte/Short/Integer columns #5389) and checks Spark vs Comet withcheckSparkAnswerAndOperatorfor the 2-arg and 3-arg forms.How are these changes tested?
cd native && cargo test -p datafusion-comet-spark-expr read_side_padding: 12 passed (4 new). The 4 new tests fail onmainwith the panic from the issue.cargo fmt --all -- --checkandcargo clippy --all-targets --workspace -- -D warnings: clean../mvnw spotless:check test -Dtest=none -Dsuites="org.apache.comet.CometStringExpressionSuite,org.apache.comet.CometSqlFileTestSuite pad.sql"(Spark 4.1.3): 2 suites, 36 tests, 0 failures, includinglpad/rpad with NULL length,sql-file: expressions/string/string_lpad.sqlandsql-file: expressions/string/string_rpad.sql.