-
Notifications
You must be signed in to change notification settings - Fork 77
Conversation
3908ab0
to
6d7ca62
Compare
e982cca
to
9394509
Compare
verified on TPC-DS with correct result |
@xuechendi rebased to master now |
Got it! |
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, just a few nits
arrow::BasicDecimal128 out = | ||
gandiva::decimalops::Divide(context, x, y, out_precision, out_scale, &overflow); | ||
return arrow::Decimal128(out); |
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 seems you defined a new func for divide here, is there any difference with the one below in decimal_ops.cc? can we directly call it?
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.
We are calling divide in decimal_ops.cc here.
cpp/src/CMakeLists.txt
Outdated
@@ -244,6 +244,8 @@ set(SPARK_COLUMNAR_PLUGIN_SRCS | |||
precompile/hash_arrays_kernel.cc | |||
precompile/unsafe_array.cc | |||
precompile/gandiva_projector.cc | |||
third_party/gandiva/decimal_ops.cc | |||
third_party/gandiva/time.cc |
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.
below line will include the .cc file?
add_subdirectory(third_party/gandiva)
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.
I found if removing this, ut will compile in failure.
the CI failure is not related, will fix in #140 |
What changes were proposed in this pull request?
fixes: #130
How was this patch tested?
locally verified