Skip to content
Open
33 changes: 26 additions & 7 deletions datafusion/catalog/src/information_schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ use datafusion_common::error::Result;
use datafusion_common::types::NativeType;
use datafusion_execution::TaskContext;
use datafusion_execution::runtime_env::RuntimeEnv;
use datafusion_expr::{AggregateUDF, ScalarUDF, Signature, TypeSignature, WindowUDF};
use datafusion_expr::{AggregateUDF, ScalarUDF, Signature, TypeSignature, WindowUDF, ReturnFieldArgs};
use datafusion_expr::{TableType, Volatility};
use datafusion_physical_plan::SendableRecordBatchStream;
use datafusion_physical_plan::stream::RecordBatchStreamAdapter;
Expand Down Expand Up @@ -421,10 +421,22 @@ fn get_udf_args_and_return_types(
Ok(arg_types
.into_iter()
.map(|arg_types| {
// only handle the function which implemented [`ScalarUDFImpl::return_type`] method
// prefer the newer `return_field_from_args` API which allows
// nullability and value-based return type decisions. Build
// the expected `ReturnFieldArgs` from the example arg types
// and pass `None` for scalar arguments since we don't have
// concrete scalar values here.
Comment on lines +424 to +428
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// prefer the newer `return_field_from_args` API which allows
// nullability and value-based return type decisions. Build
// the expected `ReturnFieldArgs` from the example arg types
// and pass `None` for scalar arguments since we don't have
// concrete scalar values here.

Personally I'd remove these comments; we don't need this much info, and some of the info here can be misleading. For example it mentions using the return_field_from_args API allows nullability & value-based return type decisions, which is true, but not here; here we always set nullability as true and set the scalar values as None, so they don't actually play a factor here.

This is a simple change which I don't think needs an explicit explanation, same for below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refrain from resolving a conversation if no action has been taken nor any comment left to explain why action was not taken.

let arg_fields_vec = arg_types
.iter()
.map(|t| Arc::new(Field::new(udf.name(), t.clone(), true)))
.collect::<Vec<_>>();
let scalar_args = vec![None; arg_fields_vec.len()];
let return_type = udf
.return_type(&arg_types)
.map(|t| remove_native_type_prefix(&NativeType::from(t)))
.return_field_from_args(ReturnFieldArgs {
arg_fields: &arg_fields_vec,
scalar_arguments: &scalar_args,
})
.map(|f| remove_native_type_prefix(&NativeType::from(f.data_type())))
.ok();
let arg_types = arg_types
.into_iter()
Expand All @@ -447,11 +459,18 @@ fn get_udaf_args_and_return_types(
Ok(arg_types
.into_iter()
.map(|arg_types| {
// only handle the function which implemented [`ScalarUDFImpl::return_type`] method
// prefer the `return_field` API which accepts argument fields
// and can provide nullability and other metadata. Build
// field references from the example arg types and call
// `return_field` on the UDAF.
let arg_fields_vec = arg_types
.iter()
.map(|t| Arc::new(Field::new(udaf.name(), t.clone(), true)))
.collect::<Vec<_>>();
let return_type = udaf
.return_type(&arg_types)
.return_field(&arg_fields_vec)
.ok()
.map(|t| remove_native_type_prefix(&NativeType::from(t)));
.map(|f| remove_native_type_prefix(&NativeType::from(f.data_type())));
let arg_types = arg_types
.into_iter()
.map(|t| remove_native_type_prefix(&NativeType::from(t)))
Expand Down
14 changes: 7 additions & 7 deletions datafusion/functions/src/datetime/date_trunc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.
use datafusion_common::internal_err;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this import down, it's causing the license CI check to fail


use std::any::Any;
use std::num::NonZeroI64;
Expand Down Expand Up @@ -223,13 +224,12 @@ impl ScalarUDFImpl for DateTruncFunc {
&self.signature
}

// keep return_type implementation for information schema generation
fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
if arg_types[1].is_null() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to move this logic into return_field_from_args, since currently it relies on calling return_type

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this comment was resolved without any action taken.

Ok(Timestamp(Nanosecond, None))
} else {
Ok(arg_types[1].clone())
}
fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
// This UDF implements `return_field_from_args` and should use that
// for return type (including nullability) decisions. Returning an
// internal error here makes misuse visible and matches other UDFs
// that implement `return_field_from_args` directly.
internal_err!("return_field_from_args should be used instead")
}

fn return_field_from_args(&self, args: ReturnFieldArgs) -> Result<FieldRef> {
Expand Down
Loading