Jefffrey commented on code in PR #26209:
URL: https://github.com/apache/datafusion/pull/26209#discussion_r4267855187
##########
datafusion/spark/src/function/string/space.rs:
##########
@@ -77,6 +78,25 @@ impl ScalarUDFImpl for SparkSpace {
Ok(return_type)
}
+ fn return_field_from_args(&self, args: ReturnFieldArgs<'_>) ->
Result<FieldRef> {
+ let arg_types = args
+ .arg_fields
+ .iter()
+ .map(|field| field.data_type().clone())
+ .collect::<Vec<_>>();
+ // Spark's UnaryExpression follows input nullability for plain
integers:
+ //
https://github.com/apache/spark/blob/32f7299601108917fb01920a54e084595b7b3bf8/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala#L596-L599
+ let input = &args.arg_fields[0];
+ // Retained dictionaries can have null values even with non-null keys.
+ let nullable = input.is_nullable()
+ || matches!(input.data_type(), DataType::Dictionary(_, _));
Review Comment:
why does space have this extra dictionary logic compared to soundex
##########
datafusion/spark/src/function/string/soundex.rs:
##########
@@ -62,6 +62,22 @@ impl ScalarUDFImpl for SparkSoundex {
}
}
+ fn return_field_from_args(&self, args: ReturnFieldArgs<'_>) ->
Result<FieldRef> {
+ let arg_types = args
+ .arg_fields
+ .iter()
+ .map(|field| field.data_type().clone())
+ .collect::<Vec<_>>();
+ // Like Spark's UnaryExpression, non-null inputs never produce null:
+ //
https://github.com/apache/spark/blob/32f7299601108917fb01920a54e084595b7b3bf8/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala#L596-L599
+ let input = &args.arg_fields[0];
+ Ok(Arc::new(Field::new(
+ self.name(),
+ self.return_type(&arg_types)?,
Review Comment:
same here
##########
datafusion/functions/src/string/concat_ws.rs:
##########
@@ -109,6 +109,22 @@ impl ScalarUDFImpl for ConcatWsFunc {
Ok(deduce_return_type(arg_types))
}
+ fn return_field_from_args(&self, args: ReturnFieldArgs<'_>) ->
Result<FieldRef> {
+ // Null values are skipped, so only a null separator produces null.
+ let separator = &args.arg_fields[0];
+ let arg_types = args
+ .arg_fields
+ .iter()
+ .map(|field| field.data_type().clone())
+ .collect::<Vec<_>>();
+ Ok(Field::new(
+ self.name(),
+ self.return_type(&arg_types)?,
Review Comment:
```suggestion
deduce_return_type(&arg_types)?,
```
then can leave `return_type` to return a not implemented error, same pattern
we follow for other UDFs that implement `return_field_from_args` and not
`return_type`
--
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]
