Skip to content

fix: return NULL from rpad/lpad when the length column is NULL instead of panicking - #5680

Open
peterxcli wants to merge 1 commit into
apache:mainfrom
peterxcli:fix/rpad-lpad-null-length
Open

fix: return NULL from rpad/lpad when the length column is NULL instead of panicking#5680
peterxcli wants to merge 1 commit into
apache:mainfrom
peterxcli:fix/rpad-lpad-null-length

Conversation

@peterxcli

Copy link
Copy Markdown
Member

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, and spark_read_side_padding_internal unwrapped every value of the length array. Any NULL in the length column panicked with called Option::unwrap() on a None value and failed the task with a CometNativeException. Spark's StringRPad / StringLPad are 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 receive Int32(Some(_)) (Spark folds NULL-literal arguments to NULL before Comet sees them) and are untouched.
  • Rust unit tests: 2-arg and 3-arg rpad/lpad (and read_side_padding) with a length array containing None → 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 a column + column + literal query (rpad(s, len, 'x')) so the 3-arg array-length arm is covered natively.
  • CometStringExpressionSuite: lpad/rpad with NULL length builds the rows explicitly (FuzzDataGenerator cannot generate NULL integers, FuzzDataGenerator silently drops nulls for Boolean/Byte/Short/Integer columns #5389) and checks Spark vs Comet with checkSparkAnswerAndOperator for 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 on main with the panic from the issue.
  • cargo fmt --all -- --check and cargo 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, including lpad/rpad with NULL length, sql-file: expressions/string/string_lpad.sql and sql-file: expressions/string/string_rpad.sql.

…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 sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rpad/lpad panic (Option::unwrap on None) when the length column contains NULL

2 participants