Skip to content

ARROW-3567: [Gandiva][GLib] Add GLib bindings of Gandiva#2800

Closed
shiro615 wants to merge 51 commits intoapache:masterfrom
shiro615:glib-support-gandiva
Closed

ARROW-3567: [Gandiva][GLib] Add GLib bindings of Gandiva#2800
shiro615 wants to merge 51 commits intoapache:masterfrom
shiro615:glib-support-gandiva

Conversation

@shiro615
Copy link
Copy Markdown
Contributor

It's GLib bindings of Gandiva.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it is possible to include <gandiva/*.h> in gandiva-glib.
But I’m not sure whether this change is correct.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks right to me

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's correct.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for checking.

@shiro615 shiro615 force-pushed the glib-support-gandiva branch from 14d89ba to f5da192 Compare October 20, 2018 04:32
Copy link
Copy Markdown
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
It's almost good! Can you check my comments?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

evaluate-expression may be better. Because this part covers not only building expression but also evaluating the built expression.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I've changed it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Evaluate expression may be better.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expression may be better.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can start from 0.12.0.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gandiva is also required.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added gandiva.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expected_output_arrays or expected_outputs is better.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can put output_columns here as literal to clear what we expect for the evaluate output.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've put the following.

assert_equal([
                   [12, 15, 18, 21], [-10, -11, -12, -13]
                 ],
                 outputs.collect(&:values))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better that we put this before Parquet = GI.load("Parquet") because we use the order in other places.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arrow-glib arrow-gpu-glib gandiva-glib parquet-glib order is better.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's correct.

@shiro615
Copy link
Copy Markdown
Contributor Author

Thank you for review.
Please review again.

@shiro615 shiro615 force-pushed the glib-support-gandiva branch 2 times, most recently from aebdf5e to e9cd59a Compare October 21, 2018 06:33
Copy link
Copy Markdown
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1
I'll merge this when we fix CI failure:
https://travis-ci.org/apache/arrow/jobs/444283175#L896-L897

CMake Error at src/gandiva/CMakeLists.txt:19 (cmake_minimum_required):
  CMake 3.11 or higher is required.  You are running version 3.9.2

I don't know why C++ job isn't failed but C GLib job is failed...

@wesm
Copy link
Copy Markdown
Member

wesm commented Oct 22, 2018

The C++ build uses a new CMake from conda-forge

@shiro615 shiro615 force-pushed the glib-support-gandiva branch from 3bf433a to 81fea3d Compare October 22, 2018 13:36
@kou
Copy link
Copy Markdown
Member

kou commented Oct 22, 2018

@wesm Thanks for the information.

@shiro615 I don't want to use conda for GLib CI to test without conda. We don't use conda when we build Linux packages,

@shiro615
Copy link
Copy Markdown
Contributor Author

I see. Thank you for the update.

@kou
Copy link
Copy Markdown
Member

kou commented Oct 23, 2018

It seems that we need Boost newer than 1.54.0 for Gandiva.

@praveenbingo
Copy link
Copy Markdown
Contributor

It seems that we need Boost newer than 1.54.0 for Gandiva.

Yup, we have tested with 1.66 or higher version of boost.

@pravindra
Copy link
Copy Markdown
Contributor

It seems that we need Boost newer than 1.54.0 for Gandiva.

yes, for the cpp build, we get a newer boost version from conda.

@kou
Copy link
Copy Markdown
Member

kou commented Oct 23, 2018

Thanks for the information.
I'm trying to use vendored Boost.

@kou
Copy link
Copy Markdown
Member

kou commented Oct 24, 2018

It seems that we can't build Gandiva with vendrored Boost.
I've created a JIRA ticket: https://issues.apache.org/jira/browse/ARROW-3603

@kou
Copy link
Copy Markdown
Member

kou commented Oct 24, 2018

I've created a pull request: #2827

@kou
Copy link
Copy Markdown
Member

kou commented Oct 25, 2018

#2827 has been merged.
I'll rebase this branch on master.

@kou kou force-pushed the glib-support-gandiva branch 2 times, most recently from db3449d to 3de18f1 Compare October 25, 2018 00:37
@kou
Copy link
Copy Markdown
Member

kou commented Oct 25, 2018

#2828 is also needed to build with vendored Boost.

@kou
Copy link
Copy Markdown
Member

kou commented Oct 25, 2018

I've merged #2828.
I'll rebase on master again.

@kou kou force-pushed the glib-support-gandiva branch from 3de18f1 to 66b2bd1 Compare October 25, 2018 00:52
@kou
Copy link
Copy Markdown
Member

kou commented Oct 25, 2018

Umm... We need more works to build Gandiva.

https://travis-ci.org/apache/arrow/jobs/445955279#L1732

[ 87%] Linking CXX executable ../../../debug/filter_test_gandiva_static
/usr/lib/llvm-6.0/lib/libLLVMX86CodeGen.a(X86FloatingPoint.cpp.o): In function `(anonymous namespace)::FPS::runOnMachineFunction(llvm::MachineFunction&) [clone .part.165]':
(.text._ZN12_GLOBAL__N_13FPS20runOnMachineFunctionERN4llvm15MachineFunctionE.part.165+0x481): undefined reference to `std::__throw_out_of_range_fmt(char const*, ...)'
/usr/lib/llvm-6.0/lib/libLLVMX86CodeGen.a(X86InstructionSelector.cpp.o): In function `llvm::PredicateBitsetImpl<107ul>::PredicateBitsetImpl(std::initializer_list<unsigned int>)':
(.text._ZN4llvm19PredicateBitsetImplILm107EEC2ESt16initializer_listIjE[_ZN4llvm19PredicateBitsetImplILm107EEC5ESt16initializer_listIjE]+0x75): undefined reference to `std::__throw_out_of_range_fmt(char const*, ...)'
/usr/lib/llvm-6.0/lib/libLLVMAsmPrinter.a(CodeViewDebug.cpp.o): In function `llvm::CodeViewDebug::getFullFilepath(llvm::DIFile const*)':
(.text._ZN4llvm13CodeViewDebug15getFullFilepathEPKNS_6DIFileE+0x452): undefined reference to `std::__throw_out_of_range_fmt(char const*, ...)'
/usr/lib/llvm-6.0/lib/libLLVMAsmPrinter.a(CodeViewDebug.cpp.o): In function `llvm::CodeViewDebug::getFullFilepath(llvm::DIFile const*)':
(.text._ZN4llvm13CodeViewDebug15getFullFilepathEPKNS_6DIFileE+0x46a): undefined reference to `std::__throw_out_of_range_fmt(char const*, ...)'
/usr/lib/llvm-6.0/lib/libLLVMCodeGen.a(TargetLoweringBase.cpp.o): In function `getOpEnabled(bool, llvm::EVT, llvm::StringRef)':
(.text._ZL12getOpEnabledbN4llvm3EVTENS_9StringRefE+0x3f3): undefined reference to `std::__throw_out_of_range_fmt(char const*, ...)'
/usr/lib/llvm-6.0/lib/libLLVMCodeGen.a(TargetLoweringBase.cpp.o):(.text._ZL20getOpRefinementStepsbN4llvm3EVTENS_9StringRefE+0x367): more undefined references to `std::__throw_out_of_range_fmt(char const*, ...)' follow
collect2: error: ld returned 1 exit status
make[2]: *** [debug/if_expr_test_gandiva_static] Error 1
make[1]: *** [src/gandiva/tests/CMakeFiles/if_expr_test_gandiva_static.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
/usr/lib/llvm-6.0/lib/libLLVMX86CodeGen.a(X86FloatingPoint.cpp.o): In function `(anonymous namespace)::FPS::runOnMachineFunction(llvm::MachineFunction&) [clone .part.165]':
(.text._ZN12_GLOBAL__N_13FPS20runOnMachineFunctionERN4llvm15MachineFunctionE.part.165+0x481): undefined reference to `std::__throw_out_of_range_fmt(char const*, ...)'
/usr/lib/llvm-6.0/lib/libLLVMX86CodeGen.a(X86InstructionSelector.cpp.o): In function `llvm::PredicateBitsetImpl<107ul>::PredicateBitsetImpl(std::initializer_list<unsigned int>)':
(.text._ZN4llvm19PredicateBitsetImplILm107EEC2ESt16initializer_listIjE[_ZN4llvm19PredicateBitsetImplILm107EEC5ESt16initializer_listIjE]+0x75): undefined reference to `std::__throw_out_of_range_fmt(char const*, ...)'
/usr/lib/llvm-6.0/lib/libLLVMAsmPrinter.a(CodeViewDebug.cpp.o): In function `llvm::CodeViewDebug::getFullFilepath(llvm::DIFile const*)':
(.text._ZN4llvm13CodeViewDebug15getFullFilepathEPKNS_6DIFileE+0x452): undefined reference to `std::__throw_out_of_range_fmt(char const*, ...)'
/usr/lib/llvm-6.0/lib/libLLVMAsmPrinter.a(CodeViewDebug.cpp.o): In function `llvm::CodeViewDebug::getFullFilepath(llvm::DIFile const*)':
(.text._ZN4llvm13CodeViewDebug15getFullFilepathEPKNS_6DIFileE+0x46a): undefined reference to `std::__throw_out_of_range_fmt(char const*, ...)'
/usr/lib/llvm-6.0/lib/libLLVMCodeGen.a(TargetLoweringBase.cpp.o): In function `getOpEnabled(bool, llvm::EVT, llvm::StringRef)':
(.text._ZL12getOpEnabledbN4llvm3EVTENS_9StringRefE+0x3f3): undefined reference to `std::__throw_out_of_range_fmt(char const*, ...)'
/usr/lib/llvm-6.0/lib/libLLVMCodeGen.a(TargetLoweringBase.cpp.o):(.text._ZL20getOpRefinementStepsbN4llvm3EVTENS_9StringRefE+0x367): more undefined references to `std::__throw_out_of_range_fmt(char const*, ...)' follow
collect2: error: ld returned 1 exit status
make[2]: *** [debug/binary_test_gandiva_static] Error 1
make[1]: *** [src/gandiva/tests/CMakeFiles/binary_test_gandiva_static.dir/all] Error 2
/usr/lib/llvm-6.0/lib/libLLVMX86CodeGen.a(X86FloatingPoint.cpp.o): In function `(anonymous namespace)::FPS::runOnMachineFunction(llvm::MachineFunction&) [clone .part.165]':
(.text._ZN12_GLOBAL__N_13FPS20runOnMachineFunctionERN4llvm15MachineFunctionE.part.165+0x481): undefined reference to `std::__throw_out_of_range_fmt(char const*, ...)'
/usr/lib/llvm-6.0/lib/libLLVMX86CodeGen.a(X86InstructionSelector.cpp.o): In function `llvm::PredicateBitsetImpl<107ul>::PredicateBitsetImpl(std::initializer_list<unsigned int>)':
(.text._ZN4llvm19PredicateBitsetImplILm107EEC2ESt16initializer_listIjE[_ZN4llvm19PredicateBitsetImplILm107EEC5ESt16initializer_listIjE]+0x75): undefined reference to `std::__throw_out_of_range_fmt(char const*, ...)'
/usr/lib/llvm-6.0/lib/libLLVMAsmPrinter.a(CodeViewDebug.cpp.o): In function `llvm::CodeViewDebug::getFullFilepath(llvm::DIFile const*)':
(.text._ZN4llvm13CodeViewDebug15getFullFilepathEPKNS_6DIFileE+0x452): undefined reference to `std::__throw_out_of_range_fmt(char const*, ...)'
/usr/lib/llvm-6.0/lib/libLLVMAsmPrinter.a(CodeViewDebug.cpp.o): In function `llvm::CodeViewDebug::getFullFilepath(llvm::DIFile const*)':
(.text._ZN4llvm13CodeViewDebug15getFullFilepathEPKNS_6DIFileE+0x46a): undefined reference to `std::__throw_out_of_range_fmt(char const*, ...)'
/usr/lib/llvm-6.0/lib/libLLVMCodeGen.a(TargetLoweringBase.cpp.o): In function `getOpEnabled(bool, llvm::EVT, llvm::StringRef)':
(.text._ZL12getOpEnabledbN4llvm3EVTENS_9StringRefE+0x3f3): undefined reference to `std::__throw_out_of_range_fmt(char const*, ...)'
/usr/lib/llvm-6.0/lib/libLLVMCodeGen.a(TargetLoweringBase.cpp.o):(.text._ZL20getOpRefinementStepsbN4llvm3EVTENS_9StringRefE+0x367): more undefined references to `std::__throw_out_of_range_fmt(char const*, ...)' follow
collect2: error: ld returned 1 exit status
make[2]: *** [debug/hash_test_gandiva_static] Error 1
make[1]: *** [src/gandiva/tests/CMakeFiles/hash_test_gandiva_static.dir/all] Error 2
/usr/lib/llvm-6.0/lib/libLLVMX86CodeGen.a(X86FloatingPoint.cpp.o): In function `(anonymous namespace)::FPS::runOnMachineFunction(llvm::MachineFunction&) [clone .part.165]':
(.text._ZN12_GLOBAL__N_13FPS20runOnMachineFunctionERN4llvm15MachineFunctionE.part.165+0x481): undefined reference to `std::__throw_out_of_range_fmt(char const*, ...)'
/usr/lib/llvm-6.0/lib/libLLVMX86CodeGen.a(X86InstructionSelector.cpp.o): In function `llvm::PredicateBitsetImpl<107ul>::PredicateBitsetImpl(std::initializer_list<unsigned int>)':
(.text._ZN4llvm19PredicateBitsetImplILm107EEC2ESt16initializer_listIjE[_ZN4llvm19PredicateBitsetImplILm107EEC5ESt16initializer_listIjE]+0x75): undefined reference to `std::__throw_out_of_range_fmt(char const*, ...)'
/usr/lib/llvm-6.0/lib/libLLVMAsmPrinter.a(CodeViewDebug.cpp.o): In function `llvm::CodeViewDebug::getFullFilepath(llvm::DIFile const*)':
(.text._ZN4llvm13CodeViewDebug15getFullFilepathEPKNS_6DIFileE+0x452): undefined reference to `std::__throw_out_of_range_fmt(char const*, ...)'
/usr/lib/llvm-6.0/lib/libLLVMAsmPrinter.a(CodeViewDebug.cpp.o): In function `llvm::CodeViewDebug::getFullFilepath(llvm::DIFile const*)':
(.text._ZN4llvm13CodeViewDebug15getFullFilepathEPKNS_6DIFileE+0x46a): undefined reference to `std::__throw_out_of_range_fmt(char const*, ...)'
/usr/lib/llvm-6.0/lib/libLLVMCodeGen.a(TargetLoweringBase.cpp.o): In function `getOpEnabled(bool, llvm::EVT, llvm::StringRef)':
(.text._ZL12getOpEnabledbN4llvm3EVTENS_9StringRefE+0x3f3): undefined reference to `std::__throw_out_of_range_fmt(char const*, ...)'
/usr/lib/llvm-6.0/lib/libLLVMCodeGen.a(TargetLoweringBase.cpp.o):(.text._ZL20getOpRefinementStepsbN4llvm3EVTENS_9StringRefE+0x367): more undefined references to `std::__throw_out_of_range_fmt(char const*, ...)' follow
collect2: error: ld returned 1 exit status
make[2]: *** [debug/filter_test_gandiva_static] Error 1
make[1]: *** [src/gandiva/tests/CMakeFiles/filter_test_gandiva_static.dir/all] Error 2
make: *** [all] Error 2

.travis.yml Outdated
- ARROW_TRAVIS_USE_VENDORED_BOOST=1
- ARROW_TRAVIS_PARQUET=1
- BUILD_TORCH_EXAMPLE=no
- CC="gcc-4.9"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for some reason, this doesn't work. Can you instead try doing this similar to the C++ gandiva entry ?

  • MATRIX_EVAL="CC=gcc-4.9 && CXX=g++-4.9"

@pravindra
Copy link
Copy Markdown
Contributor

@kou - your build still used the gcc-4.8. I suggested a workaround to use 4.9 - can you please try that ?

@kou
Copy link
Copy Markdown
Member

kou commented Oct 25, 2018

@pravindra Thanks for the information! I've added the configuration.

@kou
Copy link
Copy Markdown
Member

kou commented Oct 25, 2018

CI failure for R job is unrelated.
I'll merge this.

@kou kou closed this in e5122e3 Oct 25, 2018
@kou
Copy link
Copy Markdown
Member

kou commented Oct 25, 2018

Merged.
Thanks all!

@shiro615
Copy link
Copy Markdown
Contributor Author

Thanks!

@shiro615 shiro615 deleted the glib-support-gandiva branch October 26, 2018 05:18
@wesm
Copy link
Copy Markdown
Member

wesm commented Oct 26, 2018

Thanks for doing this! @pcmoritz I guess it's time to rehabilitate the Gandiva Cython bindings to keep up with GLib =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants