ARROW-10606: [C++] Implement Decimal256 casts#9751
ARROW-10606: [C++] Implement Decimal256 casts#9751lidavidm wants to merge 16 commits intoapache:masterfrom
Conversation
lidavidm
changed the title
|
CC @bkietz and @emkornfield. |
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
Thanks a lot for doing this. Here are some comments.
There was a problem hiding this comment.
Hmm, do we want to avoid repeating those kernels using a helper function? e.g.
template <typename OutDecimal, typename InDecimal>
OutDecimal ConvertDecimalType(InDecimal&& val, KernelContext*);
template <>
Decimal128 ConvertDecimalType<Decimal256, Decimal128>(Decimal256&& val, KernelContext*) {
return Decimal128(val.little_endian_array()[1],
val.little_endian_array()[0]);
}
template <typename OutDecimal>
OutDecimal ConvertDecimalType<OutDecimal, OutDecimal>(OutDecimal&& val, KernelContext*) {
return val;
}There was a problem hiding this comment.
Isn't the CastFunctor signature <Out, In>? Here it seems you're implementing the converse (Decimal256 to Decimal128).
There was a problem hiding this comment.
Same comment here: should be CastFunctor<Decimal256Type, Arg0Type>?
There was a problem hiding this comment.
Note the signature of CastFunction::AddKernel:
Status AddKernel(Type::type in_type_id, std::vector<InputType> in_types,
OutputType out_type, ArrayKernelExec exec,
NullHandling::type = NullHandling::INTERSECTION,
MemAllocation::type = MemAllocation::PREALLOCATE);There was a problem hiding this comment.
Don't you want to avoid the code duplication here? This looks like exactly the same test as above.
|
Thanks, looks like I got something backwards…will rework this. |
|
@lidavidm is it still worth me taking a look or is the rework going to change things substantially? |
|
@emkornfield this should be ready again (was just waiting for CI - looks like MacOS flaked) |
| auto func = std::make_shared<CastFunction>("cast_decimal256", Type::DECIMAL256); | ||
| // Needed for Parquet conversion. Full implementation is ARROW-10606 | ||
| // tracks full implementation. | ||
| // Needed for Parquet conversion. |
There was a problem hiding this comment.
I don't think this comment is still useful. These are just the same casts as for Decimal128.
3 participants
This likely needs more testing, especially where I had to implement functionality in (Basic)Decimal256. Also, we may want to extend the scalar cast benchmarks to cover decimals. There's also potentially some redundancy to eliminate in the tests.