sam-1112 commented on code in PR #7450:
URL: https://github.com/apache/datafusion-comet/pull/7450#discussion_r4451363064
##########
native/spark-expr/src/static_invoke/char_varchar_utils/read_side_padding.rs:
##########
@@ -181,6 +181,29 @@ 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.
+ // Expand it to an array so the arms above handle it.
+ [ColumnarValue::Scalar(string), rest @ ..] => {
+ let num_rows = rest.iter().find_map(|arg| match arg {
+ ColumnarValue::Array(array) => Some(array.len()),
+ ColumnarValue::Scalar(_) => None,
+ });
+ let mut expanded = Vec::with_capacity(args.len());
+ expanded.push(ColumnarValue::Array(
+ string.to_array_of_size(num_rows.unwrap_or(1))?,
Review Comment:
Would it be possible to read the scalar string directly when building each
output row? I tested a 1 MiB scalar with 64 array lengths of 1: it expands to
64 MiB of input for only 64 bytes of output. At 4,096 rows, both `lpad` and
`rpad` panic with `offset overflow`.
Avoiding the intermediate expansion could help with both memory usage and
the overflow tracked in #7247.
##########
native/spark-expr/src/static_invoke/char_varchar_utils/read_side_padding.rs:
##########
@@ -181,6 +181,29 @@ 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.
+ // Expand it to an array so the arms above handle it.
+ [ColumnarValue::Scalar(string), rest @ ..] => {
Review Comment:
Could we also handle a NULL scalar length here? A length subquery such as
`(SELECT max(id) FROM t WHERE id > 100)` can return `Scalar(Int32(NULL))`,
causing both `lpad` and `rpad` to fail with `Unsupported arguments` instead of
returning NULL.
This is an existing kernel gap that the new scalar-string path still
reaches. Please add NULL propagation and a regression test for this case.
--
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]
