mbutrovich opened a new issue, #26949: URL: https://github.com/apache/datafusion/issues/26949
### Describe the bug When the length argument is an array, `lpad` and `rpad` estimate the output size before they validate any length. [`pad_data_capacity`](https://github.com/apache/datafusion/blob/926901f9001977e85df617b96328801b42f6af4a/datafusion/functions/src/unicode/common.rs#L43-L48) adds up `len as usize` over every row, so a negative length wraps to a value near `usize::MAX`, and the sum saturates. The builder's [`with_capacity`](https://github.com/apache/datafusion/blob/926901f9001977e85df617b96328801b42f6af4a/datafusion/functions/src/unicode/pad.rs#L393-L395) then panics with `capacity overflow`. With a scalar length, [`parse_target_len`](https://github.com/apache/datafusion/blob/926901f9001977e85df617b96328801b42f6af4a/datafusion/functions/src/unicode/pad.rs#L129-L137) treats a negative length as 0, as its comment says, so the same input returns `""`. The sqllogictests expect that result for [`lpad('hi', -1)`](https://github.com/apache/datafusion/blob/926901f9001977e85df617b96328801b42f6af4a/datafusion/sqllogictest/test_files/string/string_literal.slt#L238-L251) and [`rpad('hi', -1)`](https://github.com/apache/datafusion/blob/926901f9001977e85df617b96328801b42f6af4a/datafusion/sqllogictest/test_files/string/string_literal.slt#L636-L649), but they pass the length as a literal, so they take the scalar path. The tests that read the length from a column use no negative lengths. `pad_data_capacity` came in with #23586, and #26367 moved it into the shared pad kernel. #3829 fixed an earlier panic on a negative length. ### To Reproduce On `main` at 926901f90: ```rust use std::sync::Arc; use arrow::array::{ArrayRef, Int64Array, StringArray}; use arrow::datatypes::Field; use datafusion_common::config::ConfigOptions; use datafusion_expr::{ColumnarValue, ReturnFieldArgs, ScalarFunctionArgs}; use datafusion_functions::unicode::lpad; let udf = lpad(); let args = vec![ ColumnarValue::Array(Arc::new(StringArray::from(vec!["a"])) as ArrayRef), // As ColumnarValue::Scalar(ScalarValue::Int64(Some(-1))), the result is [""] ColumnarValue::Array(Arc::new(Int64Array::from(vec![-1])) as ArrayRef), ]; let arg_fields: Vec<_> = args .iter() .map(|a| Arc::new(Field::new("a", a.data_type(), true))) .collect(); let return_field = udf .return_field_from_args(ReturnFieldArgs { arg_fields: &arg_fields, scalar_arguments: &[None, None], }) .unwrap(); let result = udf.invoke_with_args(ScalarFunctionArgs { args, arg_fields, number_rows: 1, return_field, config_options: Arc::new(ConfigOptions::default()), }); // panics: capacity overflow ``` `rpad` panics the same way. ### Expected behavior `[""]`, as with a scalar length of `-1`. PostgreSQL also treats a negative length as 0 ([`oracle_compat.c`](https://github.com/postgres/postgres/blob/884aca9b96989b9bc1b65d42e731e90f098ee2ee/src/backend/utils/adt/oracle_compat.c#L179-L181)), and its regression test expects `''` for [`lpad('hi', -5, 'xy')`](https://github.com/postgres/postgres/blob/884aca9b96989b9bc1b65d42e731e90f098ee2ee/src/test/regress/expected/strings.out#L3396-L3400). Spark's [`lpad`](https://github.com/apache/spark/blob/cecbd5d846a2cdd891a4d9f0bda302b58b02092a/common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java#L1572) returns an empty string for any length of 0 or less. ### Additional context Found by property-based tests that call each function with its arguments as arrays and as scalars, and compare the results. ### Willingness to contribute I cannot contribute a fix for this bug at this time -- 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]
