martin-g commented on code in PR #19240:
URL: https://github.com/apache/datafusion/pull/19240#discussion_r2609504670
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -67,13 +68,42 @@ impl ConcatFunc {
pub fn new() -> Self {
use DataType::*;
Self {
- signature: Signature::variadic(
- vec![Utf8View, Utf8, LargeUtf8],
- Volatility::Immutable,
+ signature: Signature::variadic_any(Volatility::Immutable)
),
}
}
-}
+
+ // Check if any argument is an array type
+ fn has_array_args(args: &[ColumnarValue]) -> bool {
+ use arrow::datatypes::DataType::*;
+ args.iter().any(|arg| match arg {
+ ColumnarValue::Array(arr) => matches!(arr.data_type(), List(_)),
Review Comment:
```suggestion
ColumnarValue::Array(arr) => matches!(arr.data_type(),
DataType::List(_)),
```
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -67,13 +68,42 @@ impl ConcatFunc {
pub fn new() -> Self {
use DataType::*;
Self {
- signature: Signature::variadic(
- vec![Utf8View, Utf8, LargeUtf8],
- Volatility::Immutable,
+ signature: Signature::variadic_any(Volatility::Immutable)
),
}
}
-}
+
+ // Check if any argument is an array type
+ fn has_array_args(args: &[ColumnarValue]) -> bool {
+ use arrow::datatypes::DataType::*;
Review Comment:
```suggestion
```
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -67,13 +68,42 @@ impl ConcatFunc {
pub fn new() -> Self {
use DataType::*;
Self {
- signature: Signature::variadic(
- vec![Utf8View, Utf8, LargeUtf8],
- Volatility::Immutable,
+ signature: Signature::variadic_any(Volatility::Immutable)
),
}
}
-}
+
+ // Check if any argument is an array type
+ fn has_array_args(args: &[ColumnarValue]) -> bool {
+ use arrow::datatypes::DataType::*;
+ args.iter().any(|arg| match arg {
+ ColumnarValue::Array(arr) => matches!(arr.data_type(), List(_)),
+ ColumnarValue::Scalar(scalar) => matches!(scalar,
ScalarValue::List(_, _)),
+ })
+ }
+
+ // Convert arguments to array_concat function
+ fn to_array_concat(args: Vec<Expr>) -> Result<Expr> {
+ let array_concat = BuiltinScalarFunction::ArrayConcat;
+ let args = args.into_iter()
+ .map(|arg| {
+ // If the argument is not already an array, wrap it in an array
+ if
!matches!(arg.get_type(&DataType::Null).unwrap_or_default(), DataType::List(_))
{
+ Ok(Expr::ScalarFunction(ScalarFunction::new(
+ BuiltinScalarFunction::MakeArray,
+ vec![arg],
+ )))
+ } else {
+ Ok(arg)
+ }
+ })
+ .collect::<Result<Vec<_>>>()?;
+
+ Ok(Expr::ScalarFunction(ScalarFunction::new(
+ array_concat,
+ args,
+ )))
+ }
Review Comment:
```suggestion
}
}
```
to close the `impl` block
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -67,13 +68,42 @@ impl ConcatFunc {
pub fn new() -> Self {
use DataType::*;
Self {
- signature: Signature::variadic(
- vec![Utf8View, Utf8, LargeUtf8],
- Volatility::Immutable,
+ signature: Signature::variadic_any(Volatility::Immutable)
),
Review Comment:
```suggestion
,
```
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -26,10 +27,10 @@ use crate::strings::{
ColumnarValueRef, LargeStringArrayBuilder, StringArrayBuilder,
StringViewArrayBuilder,
};
use datafusion_common::cast::{as_string_array, as_string_view_array};
-use datafusion_common::{Result, ScalarValue, internal_err, plan_err};
-use datafusion_expr::expr::ScalarFunction;
+use datafusion_common::{Result, ScalarValue, internal_err, plan_err, exec_err};
+use datafusion_expr::expr::{ScalarFunction, ScalarFunctionExpr};
use datafusion_expr::simplify::{ExprSimplifyResult, SimplifyInfo};
-use datafusion_expr::{ColumnarValue, Documentation, Expr, Volatility, lit};
+use datafusion_expr::{ColumnarValue, Documentation, Expr, Volatility, lit,
BuiltinScalarFunction};
Review Comment:
BuiltinScalarFunction has been removed from DataFusion long time ago...
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -67,13 +68,42 @@ impl ConcatFunc {
pub fn new() -> Self {
use DataType::*;
Self {
- signature: Signature::variadic(
- vec![Utf8View, Utf8, LargeUtf8],
- Volatility::Immutable,
+ signature: Signature::variadic_any(Volatility::Immutable)
),
}
}
-}
+
+ // Check if any argument is an array type
+ fn has_array_args(args: &[ColumnarValue]) -> bool {
+ use arrow::datatypes::DataType::*;
+ args.iter().any(|arg| match arg {
+ ColumnarValue::Array(arr) => matches!(arr.data_type(), List(_)),
+ ColumnarValue::Scalar(scalar) => matches!(scalar,
ScalarValue::List(_, _)),
Review Comment:
https://github.com/apache/datafusion/blob/c1aa1b530ab2fa73efcdeb8896dbb50c30c492f0/datafusion/common/src/scalar/mod.rs#L380
- ScalarValue::List(_) accepts just one parameter
##########
datafusion/functions/src/string/concat.rs:
##########
@@ -90,22 +120,40 @@ impl ScalarUDFImpl for ConcatFunc {
fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
use DataType::*;
+
+ // If any argument is an array, return the array type
+ if arg_types.iter().any(|t| matches!(t, List(_))) {
Review Comment:
This `if` is not really needed. The inner one does the same.
--
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]
