You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "neilconway (via GitHub)" <gi...@apache.org> on 2026/02/15 02:40:54 UTC
[PR] fix: handle Utf8View and LargeUtf8 separators in concat_ws [datafusion]
neilconway opened a new pull request, #20361:
URL: https://github.com/apache/datafusion/pull/20361
## Which issue does this PR close?
- Closes #20360
## Rationale for this change
concat_ws only handled Utf8 separators (despite its signature claiming otherwise). Attempting to pass a Utf8View or LargeUtf8 separator would result in a panic or internal error.
## What changes are included in this PR?
* Add SLT test case for array Utf8View separator
* Add unit test for scalar Utf8View separator
* Fix behavior: add support for LargeUtf8 and Utf8View separators, both array and scalar
* Other minor code cleanups and improvements
## Are these changes tested?
Yes. Added new test cases.
Note that we can't easily the scalar separator case via SQL, because `simplify_concat_ws` happens to cast constant/scalar separators to Utf8. That behavior is dubious and should be changed, but I'll tackle that in a subsequent PR.
## Are there any user-facing changes?
No, aside from a previously failing query now succeeding.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Handle Utf8View and LargeUtf8 separators in concat_ws [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey merged PR #20361:
URL: https://github.com/apache/datafusion/pull/20361
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: handle Utf8View and LargeUtf8 separators in concat_ws [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #20361:
URL: https://github.com/apache/datafusion/pull/20361#discussion_r2822331972
##########
datafusion/functions/src/string/concat_ws.rs:
##########
@@ -162,23 +156,55 @@ impl ScalarUDFImpl for ConcatWsFunc {
// parse sep
let sep = match &args[0] {
- ColumnarValue::Scalar(ScalarValue::Utf8(Some(s))) => {
- data_size += s.len() * len * (args.len() - 2); // estimate
- ColumnarValueRef::Scalar(s.as_bytes())
- }
- ColumnarValue::Scalar(ScalarValue::Utf8(None)) => {
- return Ok(ColumnarValue::Array(Arc::new(StringArray::new_null(len))));
- }
- ColumnarValue::Array(array) => {
- let string_array = as_string_array(array)?;
- data_size += string_array.values().len() * (args.len() - 2); // estimate
- if array.is_nullable() {
- ColumnarValueRef::NullableArray(string_array)
- } else {
- ColumnarValueRef::NonNullableArray(string_array)
+ ColumnarValue::Scalar(scalar) => match scalar.try_as_str() {
+ Some(Some(s)) => {
+ data_size += s.len() * len * (args.len() - 2); // estimate
+ ColumnarValueRef::Scalar(s.as_bytes())
}
- }
- _ => unreachable!("concat ws"),
+ Some(None) => {
+ return Ok(ColumnarValue::Array(Arc::new(StringArray::new_null(
Review Comment:
We can probably return scalar here
##########
datafusion/functions/src/string/concat_ws.rs:
##########
@@ -162,23 +156,55 @@ impl ScalarUDFImpl for ConcatWsFunc {
// parse sep
let sep = match &args[0] {
- ColumnarValue::Scalar(ScalarValue::Utf8(Some(s))) => {
- data_size += s.len() * len * (args.len() - 2); // estimate
- ColumnarValueRef::Scalar(s.as_bytes())
- }
- ColumnarValue::Scalar(ScalarValue::Utf8(None)) => {
- return Ok(ColumnarValue::Array(Arc::new(StringArray::new_null(len))));
- }
- ColumnarValue::Array(array) => {
- let string_array = as_string_array(array)?;
- data_size += string_array.values().len() * (args.len() - 2); // estimate
- if array.is_nullable() {
- ColumnarValueRef::NullableArray(string_array)
- } else {
- ColumnarValueRef::NonNullableArray(string_array)
+ ColumnarValue::Scalar(scalar) => match scalar.try_as_str() {
+ Some(Some(s)) => {
+ data_size += s.len() * len * (args.len() - 2); // estimate
+ ColumnarValueRef::Scalar(s.as_bytes())
}
- }
- _ => unreachable!("concat ws"),
+ Some(None) => {
+ return Ok(ColumnarValue::Array(Arc::new(StringArray::new_null(
+ len,
+ ))));
+ }
+ None => {
+ return internal_err!("Expected string separator, got {scalar:?}");
+ }
+ },
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Utf8 => {
+ let string_array = as_string_array(array)?;
+ data_size += string_array.values().len() * (args.len() - 2);
+ if array.is_nullable() {
+ ColumnarValueRef::NullableArray(string_array)
+ } else {
+ ColumnarValueRef::NonNullableArray(string_array)
+ }
+ }
+ DataType::LargeUtf8 => {
+ let string_array = as_largestring_array(array);
Review Comment:
nit: we seem to be mixing cast methods here; this comes from arrow but we're using datafusion sourced ones for utf8 & utf8view
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: handle Utf8View and LargeUtf8 separators in concat_ws [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #20361:
URL: https://github.com/apache/datafusion/pull/20361#discussion_r2823138113
##########
datafusion/functions/src/string/concat_ws.rs:
##########
@@ -162,23 +156,55 @@ impl ScalarUDFImpl for ConcatWsFunc {
// parse sep
let sep = match &args[0] {
- ColumnarValue::Scalar(ScalarValue::Utf8(Some(s))) => {
- data_size += s.len() * len * (args.len() - 2); // estimate
- ColumnarValueRef::Scalar(s.as_bytes())
- }
- ColumnarValue::Scalar(ScalarValue::Utf8(None)) => {
- return Ok(ColumnarValue::Array(Arc::new(StringArray::new_null(len))));
- }
- ColumnarValue::Array(array) => {
- let string_array = as_string_array(array)?;
- data_size += string_array.values().len() * (args.len() - 2); // estimate
- if array.is_nullable() {
- ColumnarValueRef::NullableArray(string_array)
- } else {
- ColumnarValueRef::NonNullableArray(string_array)
+ ColumnarValue::Scalar(scalar) => match scalar.try_as_str() {
+ Some(Some(s)) => {
+ data_size += s.len() * len * (args.len() - 2); // estimate
+ ColumnarValueRef::Scalar(s.as_bytes())
}
- }
- _ => unreachable!("concat ws"),
+ Some(None) => {
+ return Ok(ColumnarValue::Array(Arc::new(StringArray::new_null(
+ len,
+ ))));
+ }
+ None => {
+ return internal_err!("Expected string separator, got {scalar:?}");
+ }
+ },
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Utf8 => {
+ let string_array = as_string_array(array)?;
+ data_size += string_array.values().len() * (args.len() - 2);
+ if array.is_nullable() {
+ ColumnarValueRef::NullableArray(string_array)
+ } else {
+ ColumnarValueRef::NonNullableArray(string_array)
+ }
+ }
+ DataType::LargeUtf8 => {
+ let string_array = as_largestring_array(array);
Review Comment:
Good catch! Fixed.
##########
datafusion/functions/src/string/concat_ws.rs:
##########
@@ -162,23 +156,55 @@ impl ScalarUDFImpl for ConcatWsFunc {
// parse sep
let sep = match &args[0] {
- ColumnarValue::Scalar(ScalarValue::Utf8(Some(s))) => {
- data_size += s.len() * len * (args.len() - 2); // estimate
- ColumnarValueRef::Scalar(s.as_bytes())
- }
- ColumnarValue::Scalar(ScalarValue::Utf8(None)) => {
- return Ok(ColumnarValue::Array(Arc::new(StringArray::new_null(len))));
- }
- ColumnarValue::Array(array) => {
- let string_array = as_string_array(array)?;
- data_size += string_array.values().len() * (args.len() - 2); // estimate
- if array.is_nullable() {
- ColumnarValueRef::NullableArray(string_array)
- } else {
- ColumnarValueRef::NonNullableArray(string_array)
+ ColumnarValue::Scalar(scalar) => match scalar.try_as_str() {
+ Some(Some(s)) => {
+ data_size += s.len() * len * (args.len() - 2); // estimate
+ ColumnarValueRef::Scalar(s.as_bytes())
}
- }
- _ => unreachable!("concat ws"),
+ Some(None) => {
+ return Ok(ColumnarValue::Array(Arc::new(StringArray::new_null(
Review Comment:
Yep! Fixed.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Handle Utf8View and LargeUtf8 separators in concat_ws [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #20361:
URL: https://github.com/apache/datafusion/pull/20361#issuecomment-3924209763
Thanks @neilconway & @xanderbailey
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: handle Utf8View and LargeUtf8 separators in concat_ws [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #20361:
URL: https://github.com/apache/datafusion/pull/20361#discussion_r2809204758
##########
datafusion/functions/src/string/concat_ws.rs:
##########
@@ -546,4 +564,41 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn concat_ws_utf8view_scalar_separator() -> Result<()> {
Review Comment:
Thank you! Added.
##########
datafusion/sqllogictest/test_files/expr.slt:
##########
@@ -504,6 +504,19 @@ abc
statement ok
drop table foo
+# concat_ws with a Utf8View column as separator
+statement ok
+create table test_concat_ws_sep (sep varchar, val1 varchar, val2 varchar) as values (',', 'foo', 'bar'), ('|', 'a', 'b');
+
+query T
+SELECT concat_ws(arrow_cast(sep, 'Utf8View'), val1, val2) FROM test_concat_ws_sep ORDER BY val1
Review Comment:
Thank you! Added.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: handle Utf8View and LargeUtf8 separators in concat_ws [datafusion]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #20361:
URL: https://github.com/apache/datafusion/pull/20361#discussion_r2809008987
##########
datafusion/sqllogictest/test_files/expr.slt:
##########
@@ -504,6 +504,19 @@ abc
statement ok
drop table foo
+# concat_ws with a Utf8View column as separator
+statement ok
+create table test_concat_ws_sep (sep varchar, val1 varchar, val2 varchar) as values (',', 'foo', 'bar'), ('|', 'a', 'b');
+
+query T
+SELECT concat_ws(arrow_cast(sep, 'Utf8View'), val1, val2) FROM test_concat_ws_sep ORDER BY val1
Review Comment:
Same here with a large string test?
##########
datafusion/functions/src/string/concat_ws.rs:
##########
@@ -546,4 +564,41 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn concat_ws_utf8view_scalar_separator() -> Result<()> {
Review Comment:
Is it worth adding a large utf8 string test also?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org