Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: move TypeConversion Checker into cast_to() #6223

Closed
jackwener opened this issue May 4, 2023 · 3 comments
Closed

RFC: move TypeConversion Checker into cast_to() #6223

jackwener opened this issue May 4, 2023 · 3 comments
Labels
bug Something isn't working

Comments

@jackwener
Copy link
Member

jackwener commented May 4, 2023

Describe the bug

Current coerce_type mixed with too many functions.
I want to split check type conversion from coerce_type.

such as code

type_coercion.rs:566

// Support the `IsTrue` `IsNotTrue` `IsFalse` `IsNotFalse` type coercion.
// The above op will be rewrite to the binary op when creating the physical op.
fn get_casted_expr_for_bool_op(expr: &Expr, schema: &DFSchemaRef) -> Result<Expr> {
    let left_type = expr.get_type(schema)?;
    let right_type = DataType::Boolean;
    let coerced_type = coerce_types(&left_type, &Operator::IsDistinctFrom, &right_type)?;
    expr.clone().cast_to(&coerced_type, schema)
}

In above code, coerce_types() is just for checking TypeConversion, because we already know coerced_type is Boolean.


I think we can check TypeConversion in expr.cast_to().

Here is SQLServer Data type conversion Doc.
https://learn.microsoft.com/en-us/sql/t-sql/data-types/data-type-conversion-database-engine?view=sql-server-2017

To Reproduce

No response

Expected behavior

No response

Additional context

No response

@jackwener jackwener added the bug Something isn't working label May 4, 2023
@jackwener jackwener changed the title RFC: Add TypeConversion Checker RFC: move TypeConversion Checker into cast_to() May 4, 2023
@alamb
Copy link
Contributor

alamb commented May 4, 2023

Splitting the "what is the result type of this expression" calculation from "what types should the inputs be coerced to to execute it" is a great idea in my opinion.

I also think it would make the code much easier to reason about if the input arguments were coerced once (during type coercion in the analysis phase) and then never again. Right now I can never quite tell if the inputs to an expr have been coerced or not

@jackwener
Copy link
Member Author

Right now I can never quite tell if the inputs to an expr have been coerced or not

Agree with it, because we should get_type() just for expr have been coerced

@alamb
Copy link
Contributor

alamb commented May 5, 2023

I think this issue may be fixed

@jackwener jackwener reopened this May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants