support mathematics operation for decimal data type #1554
Conversation
|
will rebase the master after #1483 merged. |
liukun4515
force-pushed
the
mathematics_op_decimal
branch
from
2bc413a to
bd8a149
Compare
liukun4515
force-pushed
the
mathematics_op_decimal
branch
from
bd8a149 to
3d15e8f
Compare
There was a problem hiding this comment.
Thank you @liukun4515 -- this is looking good. The only concern I have is the new split between numerical_coertion and mathematics_numerical_coercion
| @@ -162,12 +162,141 @@ fn get_comparison_common_decimal_type( | |||
| } | |||
| } | |||
|
|
|||
| // Convert the numeric data type to the decimal data type. | |||
| // Now, we just support the signed integer type and floating-point type. | |||
| fn convert_numeric_type_to_decimal(numeric_type: &DataType) -> Option<DataType> { | |||
There was a problem hiding this comment.
maybe this would be better called coerce_numeric_type_to_decimal?
| bool_builder.append_value(true)?; | ||
| } else { | ||
| bool_builder.append_value(left.value(i) != right.value(i))?; | ||
| match (left.is_null(i), right.is_null(i)) { |
| } | ||
| } | ||
|
|
||
| fn mathematics_numerical_coercion( |
There was a problem hiding this comment.
This function looks almost the same as numerical_coercion -- which is used for equality
What is the reason for making a new function rather than extending numerical_coercion with Decimal support?
There was a problem hiding this comment.
The numerical_coercion is used by other coercion functions:
numerical_coercion->eq_coercion->Operator::IsDistinctFrom | Operator::IsNotDistinctFrom =>numerical_coercion->dictionary_value_coercion
If I extend thenumerical_coercion, it may take a side effect to other coercion logical.
I will refactor and merge these two together and make it clear if there is no conflict.
@alamb
There was a problem hiding this comment.
did you try extending numerical_coercion (and if so, did it have any side effects)?
There was a problem hiding this comment.
@alamb I will extend it in the follow-up pull request and support dict and distinct with decimal.
In the follow-up pull request, I think we can refactor out a common part.
Which issue does this PR close?
part of #122
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?