Skip to content

Compute ScalarFunction properties including return_type and nullable on creation #13825

Open

Description

@jayzhan211

Is your feature request related to a problem or challenge?

Continue on the discussion from #13756 (comment)

We create scalar function with the defined function udf and inputs args and keep them in Expr. We then coerce the inputs in optimizer step with TypeCoercionRewriter and compute the nullability when we create physical expr create_physical_expr.

Both return_type and nullability are computed based on the inputs which are defined by ScalarUDFImpl in logical layer. As long as we have udf and inputs: Vec<Expr> we can decide whether it has a valid types and what implicit coercion should we do, and we can define the return_type and nullablity based on those information.

I think such properties can be decided as early as possible (when the function is created in logical layer).

Once we have return_type, I think we can avoid call of data_types_with_scalar_udf again in ExprSchemable::get_type, coerce_arguments_for_signature_with_scalar_udf and create_physical_expr

Describe the solution you'd like

Compute the return_type and nullable as early as possible.

#[derive(Clone, PartialEq, Eq, PartialOrd, Hash, Debug)]
pub struct ScalarFunction {
    /// The function
    pub func: Arc<crate::ScalarUDF>,
    /// List of expressions to feed to the functions as arguments
    pub args: Vec<Expr>,
    
    // New fields for ScalarFunction
    pub return_type: Vec<DataType>,
    pub nullable: bool,
}

ScalarFunction::new_udf is used to create function, need to double check if there is other way to create scalar function. We need to make sure they all go to the same computation.

Ideally we have something like

ScalarFunction::new(udf: Arc<crate::ScalarUDF>, args: Vec<Expr>) -> Result<()> {
  1. Check udf's signature against args including length check, type check, implicit coercion
  2. Call fun.return_type() to determine the return type (or even return Field that includes the nullability)
  
  return error if anything goes wrong
}

Describe alternatives you've considered

No response

Additional context

We create physical expr with logical input and schema which doesn't seem correct too.

/// Create a physical expression for the UDF.
pub fn create_physical_expr(
    fun: &ScalarUDF,
    input_phy_exprs: &[Arc<dyn PhysicalExpr>],
    input_schema: &Schema,
    args: &[Expr],
    input_dfschema: &DFSchema,
) -> Result<Arc<dyn PhysicalExpr>> {
    let input_expr_types = input_phy_exprs
        .iter()
        .map(|e| e.data_type(input_schema))
        .collect::<Result<Vec<_>>>()?;

    // verify that input data types is consistent with function's `TypeSignature`
    data_types_with_scalar_udf(&input_expr_types, fun)?;

    // Since we have arg_types, we dont need args and schema.
    let return_type =
        fun.return_type_from_exprs(args, input_dfschema, &input_expr_types)?;

    Ok(Arc::new(
        ScalarFunctionExpr::new(
            fun.name(),
            Arc::new(fun.clone()),
            input_phy_exprs.to_vec(),
            return_type,
        )
        .with_nullable(fun.is_nullable(args, input_dfschema)),
    ))
}

Activity

  1. findepi commented on Dec 18, 2024

    @findepi
    Member

    Compute the return_type and nullable as early as possible.

    Correct, but not only that.
    We should clearly separate function calls where coercions need to be applied from those where they shouldn't. (see also @alamb 's #13756 (comment) )

  2. jayzhan211 commented on Dec 19, 2024

    @jayzhan211
    ContributorAuthor

    from those where they shouldn't

    I think the example in the comment requires coercion. ExprSchemable::get_type is asking for the return type of the function. To compute the real return type, coerced types are required.

  3. jayzhan211 commented on Dec 19, 2024

    @jayzhan211
    ContributorAuthor

    ScalarFunction::new(udf: Arc<crate::ScalarUDF>, args: Vec<Expr>). We might also need schema, need to take a look whether we have valid schema at this point

  4. self-assigned this
    on Dec 19, 2024
  5. Trikooo commented on Feb 12, 2026

    @Trikooo

    take

  6. alamb commented on Feb 13, 2026

    @alamb
    Contributor

    FWIW I think the trick of this issue and others like it will be to minimize the API changes / impacts to downstream crates

    A good rule of thumb I have found when making API changes is that the benefit should be clear to users downstream who have to change their code. For example, it is really annoying to have to make API changes with the justification that "some APIs in DataFusion are cleaner"

    It is much less annoying to make API changes when the justification is "planning goes 10% faster" , or DataFusion now supports INSERRTs, etc

  7. Trikooo commented on Apr 1, 2026

    @Trikooo

    untake

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

enhancementNew feature or request

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions