berkaysynnada commented on code in PR #15341:
URL: https://github.com/apache/datafusion/pull/15341#discussion_r2007773712
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -855,21 +855,14 @@ pub fn binary_numeric_coercion(
(UInt64, _) | (_, UInt64) => Some(UInt64),
(Int64, _)
| (_, Int64)
- | (UInt32, Int8)
- | (Int8, UInt32)
- | (UInt32, Int16)
- | (Int16, UInt32)
- | (UInt32, Int32)
- | (Int32, UInt32) => Some(Int64),
Review Comment:
I wanted to emphasize this removed line, as it seemed to me that it is
already the same of your intention:
<img width="453" alt="image"
src="https://github.com/user-attachments/assets/bdddeaf2-5940-46be-bed8-b89ef57d92b6";
/>
but I looked more and see:
1) binary_numeric_coercion(): as I can see, you only did a refactor. There
is a logical change there?
2) mathematics_numerical_coercion(): which was the inaccurate part, and you
are updating the code such that it has the same behavior with
binary_numeric_coercion().
> IIUC, the change makes sense. Further; instead of duplicating these huge
match arm, can we unify them into 1 function?
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -855,21 +855,14 @@ pub fn binary_numeric_coercion(
(UInt64, _) | (_, UInt64) => Some(UInt64),
(Int64, _)
| (_, Int64)
- | (UInt32, Int8)
- | (Int8, UInt32)
- | (UInt32, Int16)
- | (Int16, UInt32)
- | (UInt32, Int32)
- | (Int32, UInt32) => Some(Int64),
Review Comment:
I wanted to emphasize this removed line, as it seemed to me that it is
already the same of your intention:
<img width="453" alt="image"
src="https://github.com/user-attachments/assets/bdddeaf2-5940-46be-bed8-b89ef57d92b6";
/>
but I looked more and see:
1) binary_numeric_coercion(): as I can see, you only did a refactor. There
is a logical change there?
2) mathematics_numerical_coercion(): which was the inaccurate part, and you
are updating the code such that it has the same behavior with
binary_numeric_coercion().
IIUC, the change makes sense. Further; instead of duplicating these huge
match arm, can we unify them into 1 function?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
