refactor: separate get_result_type from `coerce_type` by jackwener · Pull Request #6221 · apache/datafusion

jackwener · 2023-05-04T06:50:16Z

Which issue does this PR close?

related to #5927

Rationale for this change

coerce_type current mix many motivation in it.

get_result_type
get_common_type
check type convert

I prepare to split it into get_result_type and get_common_type

about check type convertion #6223

What changes are included in this PR?

separate get_result_type from coerce_type.

Are these changes tested?

Are there any user-facing changes?

jackwener · 2023-05-04T06:51:52Z

datafusion/core/src/physical_plan/planner.rs

-            let message = format!(
-                "Expression {case:?} expected to error due to impossible coercion"
-            );
-            assert!(logical_plan.is_err(), "{}", message);


Type checking is done in the Type coercion analyzer rule rather than directly in the build

jackwener · 2023-05-04T07:08:32Z

Next, we should separate getCommonType from coerce_type

jackwener · 2023-05-04T09:01:41Z

datafusion/expr/src/type_coercion/binary.rs

+        | Operator::NotEq
+        | Operator::Lt
+        | Operator::Gt
+        | Operator::GtEq
+        | Operator::LtEq
+        | Operator::IsDistinctFrom


Original, when we call get_data_type, it will return common type instead of result type (Boolean) in coerce_type. So original it's wrong.

alamb

I think this is a great step forward -- thank you @jackwener

I think this also closes #3419

Perhaps we can update the comments here https://github.com/apache/arrow-datafusion/blob/4297547df6dc297d692ca82566cfdf135d4730b5/datafusion/expr/src/type_coercion/binary.rs#L111-L121 and update it to say that 'coerce_types' is returns the type the arguments should be coerced to

I am happy to do so in a follow on PR

Thank you for taking this first step to clean up the coercion logic ❤️

datafusion/expr/src/type_coercion/binary.rs

alamb · 2023-05-04T14:27:56Z

datafusion/expr/src/type_coercion/binary.rs

+                || is_interval(lhs_type)
+                || is_interval(rhs_type) =>
+        {
+            temporal_add_sub_coercion(lhs_type, rhs_type, op)


Eventually I think it would make the code easier to understand and maintain if we can disentangle coerce_types from get_result_types. To start in this PR, calling into coerce_types is fine.

Eventually I think it would keep the logic much simpler if get_result_type is only called on expressions that have their inputs properly coerced.

datafusion/expr/src/type_coercion/binary.rs

alamb · 2023-05-04T14:29:59Z

datafusion/physical-expr/src/expressions/datetime.rs

@@ -77,7 +77,7 @@ impl PhysicalExpr for DateTimeIntervalExpr {
    }

    fn data_type(&self, input_schema: &Schema) -> Result<DataType> {


I would also love to remove DateTimeIntervalExpr out of its own thing and into binary.rs with the other operators, but for now 👍

alamb · 2023-05-04T14:31:55Z

datafusion/expr/src/type_coercion/binary.rs

@@ -42,7 +42,7 @@ pub fn binary_operator_data_type(
    let result_type = if !any_decimal(lhs_type, rhs_type) {


I als find binary_oprator_data_type to be confusing and redundant with get_result_type but we can fix that in a follow on PR perhaps

Great suggestion, I will do in following PR

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

jackwener · 2023-05-05T03:09:09Z

Thanks @alamb review carefully.

jackwener · 2023-05-05T03:11:08Z

I prepare to merge this PR because there are some following PR.

Anyone who wants to review this PR can continue, and they they can also review these content in the following PR

tustvold · 2023-06-27T09:18:19Z

One slightly unfortunate quirk of this separation is it isn't clear if the inputs to get_result_type are before or after type coercion. This has non-trivial implications for decimal arithmetic, in particular for division, as type coercion is not idempotent. I'm still thinking about how to handle this better, but thought I would mention it in case you've already given this some thought

jackwener · 2023-06-27T10:28:07Z

One slightly unfortunate quirk of this separation is it isn't clear if the inputs to get_result_type are before or after type coercion.

Agree with it.

It may be related with #6261

alamb · 2023-06-27T18:35:39Z

One slightly unfortunate quirk of this separation is it isn't clear if the inputs to get_result_type are before or after type coercion.

In my opinion, get_result_type should always be called after type coercion and if the function doesn't support the current input types, it should error.

Basically I think once the "Analyzer" pass has run on the expr tree the rest of the code shouldn't have to guess / coerce types anymore -- the types would always line up or the query would be rejected.

I think this is the standard approach in other databases (and, eg. the type checking / semantic pass in compilers)

github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates labels May 4, 2023

jackwener commented May 4, 2023

View reviewed changes

jackwener changed the title ~~feat: separate get_result_type and coerce_type~~ feat: separate get_result_type from coerce_type May 4, 2023

jackwener force-pushed the seperate branch from 89b6e22 to 8493bca Compare May 4, 2023 07:19

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label May 4, 2023

jackwener force-pushed the seperate branch from 8493bca to c959fa5 Compare May 4, 2023 07:26

jackwener marked this pull request as ready for review May 4, 2023 08:25

jackwener changed the title ~~feat: separate get_result_type from coerce_type~~ refactor: separate get_result_type from coerce_type May 4, 2023

jackwener requested review from alamb, liukun4515 and mingmwang May 4, 2023 08:32

jackwener commented May 4, 2023

View reviewed changes

github-actions bot added the optimizer Optimizer rules label May 4, 2023

jackwener force-pushed the seperate branch from 35ba8b9 to c959fa5 Compare May 4, 2023 09:16

github-actions bot removed the optimizer Optimizer rules label May 4, 2023

alamb mentioned this pull request May 4, 2023

RFC: Distinguish between argument coercion and output type #6206

Closed

alamb reviewed May 4, 2023

View reviewed changes

alamb approved these changes May 4, 2023

View reviewed changes

jackwener and others added 4 commits May 5, 2023 01:07

feat: separate get_result_type and coerce_type

8a31b8c

comment

cee922b

Apply suggestions from code review

aec386c

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

fix slt

441b7d5

jackwener force-pushed the seperate branch from d609b07 to 441b7d5 Compare May 4, 2023 17:10

jackwener merged commit 439863f into apache:main May 5, 2023

jackwener deleted the seperate branch May 5, 2023 03:11

This was referenced May 5, 2023

refactor: use get_result_type to replace binary_operator_data_type. #6241

Merged

refactor: separate get_common_type / get_result_type for temporal type #6250

Merged

		@@ -77,7 +77,7 @@ impl PhysicalExpr for DateTimeIntervalExpr {
		}

		fn data_type(&self, input_schema: &Schema) -> Result<DataType> {

		@@ -42,7 +42,7 @@ pub fn binary_operator_data_type(
		let result_type = if !any_decimal(lhs_type, rhs_type) {

Conversation

jackwener commented May 4, 2023 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

jackwener May 4, 2023

Choose a reason for hiding this comment

Uh oh!

jackwener commented May 4, 2023

Uh oh!

jackwener May 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb May 4, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb May 4, 2023

Choose a reason for hiding this comment

Uh oh!

alamb May 4, 2023

Choose a reason for hiding this comment

Uh oh!

jackwener May 4, 2023

Choose a reason for hiding this comment

Uh oh!

jackwener commented May 5, 2023

Uh oh!

jackwener commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tustvold commented Jun 27, 2023

Uh oh!

jackwener commented Jun 27, 2023

Uh oh!

alamb commented Jun 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jackwener commented May 4, 2023 •

edited by alamb

Loading

jackwener May 4, 2023 •

edited

Loading

jackwener commented May 5, 2023 •

edited

Loading