ARROW-3329: [C++] Added casts Decimal128 to Decimal128 and Int64 #6427
Conversation
|
Thanks for your first contribution. Could you update the pull request description to describe this change? |
Could you tell me how to do it? I've pasted the description into the first comment. But I can not see any way to add description to the PR itself. Normally I work with gitlab where there is button for it - can not find it here. |
|
You did update the description ("the first comment" is the description of this pull request). It's enough. Someone will review this. Please wait for a while. |
pitrou
left a comment
There was a problem hiding this comment.
Hi, and thanks for doing this. Here are some comments.
There was a problem hiding this comment.
We shouldn't log errors but rather save them on the context. This is how it's done in another kernel:
https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/cast.cc#L295-L298
There was a problem hiding this comment.
Corrected - hopefully correctly.
There was a problem hiding this comment.
If options.allow_int_overflow is false, you should also check that the result value doesn't overflow out_type. You should be able to use Decimal::ToInteger for that.
There was a problem hiding this comment.
Corrected - hopefully correctly.
There was a problem hiding this comment.
Same here: should save the error on the context.
There was a problem hiding this comment.
Only Int64? I would expect all signed integer types.
There was a problem hiding this comment.
Added other ints though they are not really needed as there is cast from int64 to them.
There was a problem hiding this comment.
Added Int32 Int16 Int8
cpp/src/arrow/type.h
Outdated
There was a problem hiding this comment.
Adding this include will worsen compile times. You should be able to add a forward declaration to type_fwd.h if needed, instead.
There was a problem hiding this comment.
This depends on the above.
There was a problem hiding this comment.
Moved to type_fwd.h
There was a problem hiding this comment.
I would expect more test cases here, especially some examples where conversion fails because of overflow, but also examples including nulls.
There was a problem hiding this comment.
Added more tests: nulls, overflow, truncation
There was a problem hiding this comment.
Same remark here: I would expect more tricky conversion cases, failures (overflow) and nulls.
There was a problem hiding this comment.
Added more tests: nulls, truncation
cpp/src/arrow/compute/kernels/cast.h
Outdated
There was a problem hiding this comment.
You shouldn't add this if this isn't used anywhere.
There was a problem hiding this comment.
Actually it is used now
|
@JacekPliszka Did you forget to push your changes? |
No. Handling overflow and truncation requires more work and it would take more time at my C++ skill level than I have. |
|
What about casting the other way (integer to decimal)? I was recently looking at adding some tests for decimal types in R but couldn't figure out how to make a decimal array to begin with. |
I am not planning to do it - the scope is already larger than I had planned. |
Well, in this case, someone else will have to do it, but they may not make this a priority :-) |
|
@pitrou can we merge this and make a followup Jira for the outstanding questions? |
|
Why should we merge a deficient PR? There is no proper error handling here. |
|
Yes if @JacekPliszka isn't able to complete it we should wait for someone to pick up the changes and add the necessary error handling etc. |
|
I will try to find some time during weekend. I believe have decimal to decimal done with options and error handling. decimal to int still needs some work. |
|
OK, I did decimal to int but still need some time for nulls handling in decimal to decimal - probably another day. I have a question though about loop optimization - currently I move the loops inside ifs but maybe I can assume that compilers will do it for me - will they? |
Ideally, they will. But optimization is always based on heuristics and you never know what the compiler will decide. So in some cases it makes sense to duplicate loops by hand. |
|
OK, tests should be all green soon so please review. I am not happy with all solutions I've applied there but I believe it is more or less correct. Notes;
|
|
Thank you @JacekPliszka. I'm taking a look now. |
|
So, everything was good functionally, thank you :-) I just pushed a number of simplifications in the code. Will merge if CI is green. |
Minimal implementation of casts: