-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
support array with scalar arithmetic operation for decimal data type #2233
support array with scalar arithmetic operation for decimal data type #2233
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @liukun4515
@@ -280,25 +280,64 @@ where | |||
.collect() | |||
} | |||
|
|||
fn arith_decimal_scalar<F>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to move some / all of this decimal manipulation into the arrow-rs crate at some point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for your reminder and I have a plan to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But now I find a bug in the arithmetic operation. https://github.com/apache/arrow-datafusion/blob/c91efc27658e58264c4f346a5cfdec8810179e90/datafusion/physical-expr/src/expressions/binary.rs#L302 and https://github.com/apache/arrow-datafusion/blob/c91efc27658e58264c4f346a5cfdec8810179e90/datafusion/physical-expr/src/expressions/binary.rs#L307
In the divide method, we may lose of precision by converting the i128
to f64
.
We use the f64
to store the larger range of data, but we lose the precision.
I will try to figure out this issue and resolve this.
I think It can be merged |
Sounds good to me -- feel free to do it yourself next time; I normally wait for at least one approving review from a maintainer or active contributor. If the change is trivial I'll merge it then and if a little more involved I try and leave it open for 24 hours. |
Thanks again @liukun4515 |
Which issue does this PR close?
part of #122
This query
select a+b from table
was supported, but the queryselect a+10 from table
is not supported.Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?