Skip to content
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

Build Folly with FOLLY_HAVE_INT128_T ON #4864

Closed
majetideepak opened this issue May 8, 2023 · 7 comments
Closed

Build Folly with FOLLY_HAVE_INT128_T ON #4864

majetideepak opened this issue May 8, 2023 · 7 comments
Assignees
Labels
enhancement New feature or request

Comments

@majetideepak
Copy link
Collaborator

majetideepak commented May 8, 2023

Description

To support HUGEINT(int128_t) natively in Velox, we need to build Folly with FOLLY_HAVE_INT128_T ON.
The need arises from CastExpr.cpp which uses Folly for type conversions.
See PR: #4828

Build issue
CastExpr.cpp:(.text._ZZN8facebook5velox4exec8CastExpr16applyCastWithTryInNS0_10StringViewEEEvRKNS0_17SelectivityVectorERNS1_7EvalCtxERKNS0_10BaseVectorEPNS0_10FlatVectorIT_EEENKUliE_clEi[_ZZN8facebook5velox4exec8CastExpr16applyCastWithTryInNS0_10StringViewEEEvRKNS0_17SelectivityVectorERNS1_7EvalCtxERKNS0_10BaseVectorEPNS0_10FlatVectorIT_EEENKUliE_clEi]+0x6d): undefined reference to `folly::Expected<__int128, folly::ConversionCode> folly::detail::str_to_integral<__int128>(folly::Range<char const*>*)'
We can set this in Folly CMake for Bundled installation.
However, some CI tests use the SYSTEM for VELOX_DEPENDENCY_SOURCE. This will require a new CI image.

@kgpai can you help with this? Thanks!

@majetideepak majetideepak added the enhancement New feature or request label May 8, 2023
@kgpai
Copy link
Contributor

kgpai commented May 8, 2023

Thanks @majetideepak , Should be a straightforward change.
I will send across a PR for this.

cc: @assignUser

@majetideepak
Copy link
Collaborator Author

@kgpai can you please help with this? Thanks.

@kgpai
Copy link
Contributor

kgpai commented Jun 1, 2023

Hi @majetideepak , Apologies, but this is a change that can be done by changing : https://github.com/facebookincubator/velox/blob/main/scripts/setup-circleci.sh to build folly with FOLLY_HAVE_INT128_T on.
Once a change to this script is made and merged with main - the docker images will be automatically updated and dont require any manual intervention , i.e subsequent PRs will use docker images with folly having int128 support.

If you still need me to do this change, I will be happy to do so, but please give me a week or so to get to it.

@majetideepak
Copy link
Collaborator Author

@kgpai My PR already has this change. https://github.com/facebookincubator/velox/pull/4828/files#diff-83e30a74f2113d8c606b03f0c065838e6710306a56292f9ffd227105b44bfb02
Can you help merge that then?

@kgpai
Copy link
Contributor

kgpai commented Jun 1, 2023

Thanks @majetideepak , In this case just move that particular change to a separate PR and I will work with you to merge it asap. After merge this should be available on those images.

@majetideepak
Copy link
Collaborator Author

@kgpai, here is a PR #5113 Thanks.

@majetideepak
Copy link
Collaborator Author

Fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants