-
Notifications
You must be signed in to change notification settings - Fork 75
[NSE-62]Fixing issue0062 for package arrow dependencies in jar #63
Conversation
LINK_PRIVATE protobuf::libprotobuf) | ||
message(STATUS "Building ProtoBuf from Source: ${BUILD_PROTOBUF}") | ||
target_link_libraries(spark_columnar_jni | ||
LINK_PRIVATE protobuf::libprotobuf) |
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.
missing protobuf building func?
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.
The installation function and target_link_libraries function are separated in two parts.
- call Protobuf function is located on line 34.
- link function is located on line 480.
This is the previous way to implement protobuf.
To avoid any ambiguous, let me combine them together in the code and refresh the PR.
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.
The travis log shows missing protoc even build_protobuf=ON
https://github.com/oap-project/native-sql-engine/pull/63/checks?check_run_id=1769022912
-- Building ProtoBuf from Source: ON
-- Configuring done
-- Generating done
-- Build files have been written to: /home/runner/work/native-sql-engine/native-sql-engine/cpp/build
Scanning dependencies of target jni_proto
[ 1%] Running PROTO compiler on Exprs.proto
/bin/sh: 1: protobuf_ep-install/bin/protoc: not found
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.
This is a dependency issue, it happened sometimes in previous code.
The root cause would be "jni_proto" must run after protobuf installed.
Have fixed this issue in 2nd commit.
cpp/src/CMakeLists.txt
Outdated
-DARROW_WITH_ZLIB=OFF | ||
-DARROW_FILESYSTEM=ON | ||
-DARROW_JSON=ON | ||
-DARROW_FLIGHT=OFF |
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.
let's add this missing param, "-DARROW_WITH_FASTPFOR=ON"
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.
Done to turn it on in 2nd commit.
What changes were proposed in this pull request?
Close #62
Add 3 ways when building jar
Build Static Arrow from Source: -DBUILD_ARROW=ON -DSTATIC_ARROW=ON
For Static Arrow, there are some issues in Arrow 0.17.0. So there are still some issues in static library support.
Build Shared Arrow from Source: -DBUILD_ARROW=ON -DSTATIC_ARROW=OFF
In this case, all related dependency so file will be packaged into the jar file.
Use the existing Arrow library: -DBUILD_ARROW=OFF -DARROW_ROOT=/path/to/arrow
How was this patch tested?
Tested in my own developer server and another entire new server for those 3 cases.
Only Static Arrow has some issues and expect to be supported after Arrow version upgrade.