The framework about expression type coercion

[liukun4515]

Describe the bug
In the Implement DECIMAL type, I want to implement the decimal data type in datafusion.

In the survey period, I take a look the type coercion in the current datafusion, and find some confused points.

For example:

• AggFunction

logical expr：
agg(input_exprs)

In the expression evaluation, we need to coercion the expr and calc the result type of AggFunction expr.

In the current code, the logical result type use this API return_type, but get the physical expr result type use this API create_agg_expr.

These two API have a gap, which is coerce. In the coerce, the input expr may be added try_cast and change the data type. The result data type of inferred physical expr may be diff with the logical result data type.

To Reproduce
Steps to reproduce the behavior:

Expected behavior
A clear and concise description of what you expected to happen.

Additional context
Add any other context about the problem here.

plan

• rule for AggregateFunction Add coercion rules for AggregateFunctions #1387
• rule for binary operator: comparison operation
• rule for ScalarFunction
• rule for ScalarUDF
• rule for WindowFunction
• rule for AggregateUDF
• other

[liukun4515]

By the way, I think there is a bug and #1358

    let name = name.into();
    let arg = coerce(args, input_schema, &signature(fun))?;
    if arg.is_empty() {
        return Err(DataFusionError::Plan(format!(
            "Invalid or wrong number of arguments passed to aggregate: '{}'",
            name,
        )));
    }
    let arg = arg[0].clone();

    let arg_types = args
        .iter()
        .map(|e| e.data_type(input_schema))
        .collect::<Result<Vec<_>>>()?;

    let return_type = return_type(fun, &arg_types)?;

The args is the original physical exprs, and the arg is the coerced physical exprs.
In order to get the result type, we need replace

    let arg_types = args
        .iter()
        .map(|e| e.data_type(input_schema))
        .collect::<Result<Vec<_>>>()?;

with

    let arg_types = arg
        .iter()
        .map(|e| e.data_type(input_schema))
        .collect::<Result<Vec<_>>>()?;

In the functions.rs, I think the usage is right.

[alamb]

I agree that the coercion framework is a bit messy at the moment. It might help to start refactoring by collecting the coercion logic into a single module with some clearly defined entrypoint.

Perhaps something like

pub fn coerce_to(expr: Expr, to_type: &DataType) -> Option<Expr> {
...
}

With appropriate comments and tests

[alamb]

BTW I would love to help centralize the coercion logic -- I have ideas / plans to make it support coercing to/from NULL which is currently a gap.

[liukun4515]

Now, the coercision is in the physical plan or physical expr generation period, but in the spark and other SQL system this action is in the logical period. @alamb

[liukun4515]

BTW I would love to help centralize the coercion logic -- I have ideas / plans to make it support coercing to/from NULL which is currently a gap.

Can you explain or give more details?

[liukun4515]

I agree that the coercion framework is a bit messy at the moment. It might help to start refactoring by collecting the coercion logic into a single module with some clearly defined entrypoint.

Perhaps something like

pub fn coerce_to(expr: Expr, to_type: &DataType) -> Option<Expr> {
...
}

With appropriate comments and tests

Now all the coercion logic are in one the rule file, and it can't fit many case. For example

select abs('123');
select '123'+10;
select 12.3+'1.23';
....

Now all the coercion logic are in one the rule file, and it can't fit many case. For example
I think we can create a new mod for coercion and type inferring.
The new mod should be pub crate and should not public to out of the datafusion crate.

I think we should create diff coercion rule for diff expr type, and we can summary the common logic to the same file.

In the AggFunction, ScalarFuntion, ScalarUDF, we can define diff coercion to fit each expr and operation.

[liukun4515]

In the binary expr, the input has the op type and left,right expr, we need these three component to get the result type and coercion;

In the build-in function expr, the input has the function type and exprs, we need these three component to get the result type and coercion.

The AggregateFunction is the same as above.

[liukun4515]

@andygrove @jimexist @Dandandan @houqp PTAL

[liukun4515]

@jorgecarleitao Do you have time to take look it?

[alamb]

Now, the coercision is in the physical plan or physical expr generation period, but in the spark and other SQL system this action is in the logical period. @alamb

Yes, this is something @jorgecarleitao wrote -- I think it originally came in as part of apache/arrow#8024, which has much more of the rationale. Other systems I have worked on do the coercion at the LogicalPlan / Expr level, but I think doing coercion at the ExeuctioPlan / PhysicalExpr level makes sense too. Unless we discover some compelling reason to move coercion to LogicalPlan I suggest we do not revisit this decision.

I think we can create a new mod for coercion and type inferring.
The new mod should be pub crate and should not public to out of the datafusion crate.

This makes sense to me

I think we should create diff coercion rule for diff expr type, and we can summary the common logic to the same file.

Yes I agree

In the AggFunction, ScalarFuntion, ScalarUDF, we can define diff coercion to fit each expr and operation.

I think it might make sense to keep all the coercion logic in the same place (e.g. in the coercion.rs module rather than splitting it across AggFunction, ScalarFunction, etc) but I do not feel super strongly about this

[liukun4515]

Now, the coercision is in the physical plan or physical expr generation period, but in the spark and other SQL system this action is in the logical period. @alamb

Yes, this is something @jorgecarleitao wrote -- I think it originally came in as part of apache/arrow#8024, which has much more of the rationale. Other systems I have worked on do the coercion at the LogicalPlan / Expr level, but I think doing coercion at the ExeuctioPlan / PhysicalExpr level makes sense too. Unless we discover some compelling reason to move coercion to LogicalPlan I suggest we do not revisit this decision.

Yes, now we have not meet some issues which should change the coercion to logical period. if we meet them, we can discuss this again.

I think we can create a new mod for coercion and type inferring.
The new mod should be pub crate and should not public to out of the datafusion crate.

This makes sense to me

I think we should create diff coercion rule for diff expr type, and we can summary the common logic to the same file.

Yes I agree

In the AggFunction, ScalarFuntion, ScalarUDF, we can define diff coercion to fit each expr and operation.

I think it might make sense to keep all the coercion logic in the same place (e.g. in the coercion.rs module rather than splitting it across AggFunction, ScalarFunction, etc) but I do not feel super strongly about this

I think we have aligned that we can split coercion logic into a independent mod (one sub directory in the physical_plan directory).
I will try to implement the Agg function to as a sample.