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

Need to type coercion for arithmetic op when the left and right type are same #3387

Closed
liukun4515 opened this issue Sep 7, 2022 · 6 comments · Fixed by #5980
Closed

Need to type coercion for arithmetic op when the left and right type are same #3387

liukun4515 opened this issue Sep 7, 2022 · 6 comments · Fixed by #5980
Assignees
Labels
bug Something isn't working

Comments

@liukun4515
Copy link
Contributor

Describe the bug

#3254 (comment)

bug code: https://github.com/apache/arrow-datafusion/blob/master/datafusion/expr/src/binary_rule.rs#L293

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.

@liukun4515 liukun4515 added the bug Something isn't working label Sep 7, 2022
@liukun4515
Copy link
Contributor Author

current plan for datafusion

❯ explain select cast(1 as decimal(10,1)) + cast(2 as decimal(10,1));
+---------------+--------------------------------------------------------------+
| plan_type     | plan                                                         |
+---------------+--------------------------------------------------------------+
| logical_plan  | Projection: Decimal128(Some(30),10,1) AS Int64(1) + Int64(2) |
|               |   EmptyRelation                                              |
| physical_plan | ProjectionExec: expr=[Some(30),10,1 as Int64(1) + Int64(2)]  |
|               |   EmptyExec: produce_one_row=true                            |
|               |                                                              |
+---------------+--------------------------------------------------------------+

spark plan:

spark-sql> explain extended select cast(1 as decimal(10,1)) + cast(2 as decimal(10,1));
== Parsed Logical Plan ==
'Project [unresolvedalias((cast(1 as decimal(10,1)) + cast(2 as decimal(10,1))), None)]
+- OneRowRelation

== Analyzed Logical Plan ==
(CAST(1 AS DECIMAL(10,1)) + CAST(2 AS DECIMAL(10,1))): decimal(11,1)
Project [CheckOverflow((promote_precision(cast(cast(1 as decimal(10,1)) as decimal(11,1))) + promote_precision(cast(cast(2 as decimal(10,1)) as decimal(11,1)))), DecimalType(11,1), true) AS (CAST(1 AS DECIMAL(10,1)) + CAST(2 AS DECIMAL(10,1)))#166]
+- OneRowRelation

== Optimized Logical Plan ==
Project [3.0 AS (CAST(1 AS DECIMAL(10,1)) + CAST(2 AS DECIMAL(10,1)))#166]
+- OneRowRelation

== Physical Plan ==
*(1) Project [3.0 AS (CAST(1 AS DECIMAL(10,1)) + CAST(2 AS DECIMAL(10,1)))#166]
+- *(1) Scan OneRowRelation[]

Time taken: 0.017 seconds, Fetched 1 row(s)

@liukun4515
Copy link
Contributor Author

@liukun4515
Copy link
Contributor Author

cc @alamb @andygrove

I will fix it later.

@liukun4515 liukun4515 self-assigned this Sep 7, 2022
@liukun4515
Copy link
Contributor Author

Can we fix this and #3388 together?

@alamb
Copy link
Contributor

alamb commented Sep 8, 2022

There is more context about this issue here: #3254 (comment)

@liukun4515
Copy link
Contributor Author

When I try to fix this bug with remove below code

    // same type => all good
    if lhs_type == rhs_type {
        return Some(lhs_type.clone());
    }

When move the type coercion to the logical phase, the result data type of the expr can be determined in the logical phase.

In the physical phase, we must not use the type coercion to coerce the data type.

liukun4515 added a commit to liukun4515/arrow-datafusion that referenced this issue Sep 16, 2022
alamb added a commit that referenced this issue Sep 24, 2022
* remove type coercion binary from phy

* fix test case

* revert the fix for #3387

* type coercion before simplify expression

* complete remove the type coercion in the physical plan

* refactor

* merge master

* refactor

* do type coercion in the simplify expression

* Add comments

* fix: fmt

Co-authored-by: Andrew Lamb <[email protected]>
@andygrove andygrove modified the milestone: 14.0.0 Oct 30, 2022
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

Successfully merging a pull request may close this issue.

3 participants