rich7420 commented on code in PR #7450:
URL: https://github.com/apache/datafusion-comet/pull/7450#discussion_r4511335582
##########
native/spark-expr/src/static_invoke/char_varchar_utils/read_side_padding.rs:
##########
@@ -181,6 +192,70 @@ fn spark_read_side_padding2(
))),
}
}
+ // The string argument can be a scalar even when it references
columns, e.g.
+ // `IF(p, (SELECT max(s) FROM t), s)` when every row takes the
subquery branch.
+ // Every row reads the one value rather than an expanded copy, which
for a wide string
+ // could exceed the 32-bit offsets of the input array.
+ [ColumnarValue::Scalar(string), length, rest @ ..] => {
+ let pad_string = match rest {
+ [] => SPACE,
+ [ColumnarValue::Scalar(ScalarValue::Utf8(Some(pad)))] =>
pad.as_str(),
+ _ => {
+ return Err(DataFusionError::Internal(format!(
+ "Unsupported arguments {args:?} for function
rpad/lpad/read_side_padding",
+ )))
+ }
+ };
+ let num_rows = match length {
+ ColumnarValue::Array(array) => Some(array.len()),
+ ColumnarValue::Scalar(_) => None,
+ };
+ let len = num_rows.unwrap_or(1);
+ let result = match string {
+ ScalarValue::Utf8(value) | ScalarValue::LargeUtf8(value) => {
+ let value = value.as_deref();
+ // A scalar contributes no capacity floor: reserving its
bytes for every
+ // row could exceed the 32-bit offsets of the output.
+ let strings = std::iter::repeat_n(value, len);
+ let ascii = value.is_none_or(str::is_ascii);
+ if matches!(string, ScalarValue::Utf8(_)) {
+ pad_strings::<i32, _>(
+ strings,
+ ascii,
+ 0,
+ truncate,
+ length.clone(),
+ pad_string,
+ is_left_pad,
+ )?
+ } else {
+ pad_strings::<i64, _>(
+ strings,
+ ascii,
+ 0,
+ truncate,
+ length.clone(),
+ pad_string,
+ is_left_pad,
+ )?
+ }
+ }
+ other => {
+ return Err(DataFusionError::Internal(format!(
+ "Unsupported data type {:?} for function
rpad/lpad/read_side_padding",
+ other.data_type()
+ )))
+ }
+ };
+ match num_rows {
Review Comment:
DataFusion's `ScalarFunctionExpr::evaluate` already turns a one-row array
into a scalar when every argument is a scalar. A local probe with this block
removed still got a scalar for multi-row batches. Could this just return
`result` and drop `num_rows`?
--
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]
