Skip to content
This repository has been archived by the owner on Sep 18, 2023. It is now read-only.

[NSE-130] support decimal in project #131

Merged
merged 7 commits into from
Mar 5, 2021

Conversation

rui-mo
Copy link
Collaborator

@rui-mo rui-mo commented Feb 25, 2021

What changes were proposed in this pull request?

fixes: #130

How was this patch tested?

locally verified

@rui-mo rui-mo changed the title [NSE-130] support decimal in project [NSE-130][DNM] support decimal in project Feb 25, 2021
@github-actions
Copy link

#130

@github-actions
Copy link


@rui-mo rui-mo force-pushed the wip_decimal branch 2 times, most recently from 3908ab0 to 6d7ca62 Compare March 2, 2021 03:15
@github-actions
Copy link

github-actions bot commented Mar 2, 2021


@rui-mo rui-mo force-pushed the wip_decimal branch 3 times, most recently from e982cca to 9394509 Compare March 2, 2021 07:00
@github-actions
Copy link

github-actions bot commented Mar 2, 2021


@github-actions
Copy link

github-actions bot commented Mar 2, 2021


@github-actions
Copy link

github-actions bot commented Mar 2, 2021


@github-actions
Copy link

github-actions bot commented Mar 2, 2021


@github-actions
Copy link

github-actions bot commented Mar 2, 2021


@github-actions
Copy link

github-actions bot commented Mar 3, 2021


@github-actions
Copy link

github-actions bot commented Mar 3, 2021


@rui-mo
Copy link
Collaborator Author

rui-mo commented Mar 3, 2021

verified on TPC-DS with correct result

@rui-mo
Copy link
Collaborator Author

rui-mo commented Mar 4, 2021

@xuechendi rebased to master now

@rui-mo rui-mo changed the title [NSE-130][DNM] support decimal in project [NSE-130] support decimal in project Mar 4, 2021
@xuechendi
Copy link
Collaborator

@xuechendi rebased to master now

Got it!

@github-actions
Copy link

github-actions bot commented Mar 4, 2021


Copy link
Collaborator

@zhouyuan zhouyuan left a 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

cpp/src/third_party/gandiva/decimal_ops.cc Show resolved Hide resolved
arrow::BasicDecimal128 out =
gandiva::decimalops::Divide(context, x, y, out_precision, out_scale, &overflow);
return arrow::Decimal128(out);
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

@@ -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
Copy link
Collaborator

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)

Copy link
Collaborator Author

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.

@github-actions
Copy link

github-actions bot commented Mar 4, 2021


@github-actions
Copy link

github-actions bot commented Mar 4, 2021


@github-actions
Copy link

github-actions bot commented Mar 4, 2021


@github-actions
Copy link

github-actions bot commented Mar 5, 2021


@zhouyuan
Copy link
Collaborator

zhouyuan commented Mar 5, 2021

the CI failure is not related, will fix in #140

@zhouyuan zhouyuan merged commit c38528a into oap-project:master Mar 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support decimal in project
3 participants