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

[NSE-176]Arrow install order issue #231

Merged
merged 9 commits into from
Apr 13, 2021
Merged

[NSE-176]Arrow install order issue #231

merged 9 commits into from
Apr 13, 2021

Conversation

weiting-chen
Copy link
Collaborator

What changes were proposed in this pull request?

Add Arrow install script before arrow data source install.

  • Use the same parameters to deliver from mvn command.
  • Default value is cpp_tests=OFF, build_arrow=ON, static_arrow=OFF, build_protobuf=ON, arrow_root=/usr/local
  • Default Arrow build from source installation dir is under arrow-data-source/common/script/build
  • Keep Protobuf installation in native SQL engine module.

How was this patch tested?

Tested in my developer machine.

@weiting-chen weiting-chen requested a review from zhouyuan April 6, 2021 10:20
@github-actions
Copy link

github-actions bot commented Apr 6, 2021

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/oap-project/native-sql-engine/issues

Then could you also rename commit message and pull request title in the following format?

[NSE-${ISSUES_ID}] ${detailed message}

See also:

@weiting-chen weiting-chen changed the title Wip install order [NSE-176]Arrow install order issue Apr 6, 2021
@github-actions
Copy link

github-actions bot commented Apr 6, 2021

#176

@github-actions
Copy link

github-actions bot commented Apr 6, 2021


zhouyuan
zhouyuan reviewed Apr 7, 2021
View reviewed changes
native-sql-engine/cpp/src/CMakeLists.txt
@@ -459,7 +462,7 @@ set_target_properties(spark_columnar_jni PROPERTIES
)

# Build Arrow
message(STATUS "Building ARROW from Source: ${BUILD_ARROW}")
#message(STATUS "Building ARROW from Source: ${BUILD_ARROW}")
if(BUILD_ARROW)
Copy link
Collaborator

Choose a reason for hiding this comment

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

will build arrow again here?

native-sql-engine/cpp/src/CMakeLists.txt Outdated
set(ARROW_LIB_DIR "${ARROW_ROOT}/lib")
set(ARROW_LIB64_DIR "${ARROW_ROOT}/lib64")
message(STATUS "Set Arrow Library Directory in ${ARROW_LIB_DIR} or ${ARROW_LIB64_DIR}")
message(STATUS "Set Arrow Library Directory in ${ARROW_BUILD_FROM_SOURCE_LIB_DIR} or ${ARROW_LIB_DIR} or ${ARROW_LIB64_DIR}")
set(ARROW_BUILD_FROM_SOURCE_INCLUDE_DIR "${ARROW_ROOT}/include")
Copy link
Collaborator

Choose a reason for hiding this comment

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

${ARROW_BFS_INSTALL_DIR}/include

@github-actions
Copy link

github-actions bot commented Apr 8, 2021


@zhouyuan
Copy link
Collaborator

@weiting-chen the scala ut failed due to timeout, maybe we could simplify the dependency installation in this case

@weiting-chen
Copy link
Collaborator Author

@weiting-chen the scala ut failed due to timeout, maybe we could simplify the dependency installation in this case

Done to chang build_arrow=OFF in Scala UT, lets check if this commit can pass scala ut testing.

@github-actions
Copy link


@weiting-chen
Copy link
Collaborator Author

Thanks, @zhouyuan
I can see the error message in Scala UT is cannot find the parent file.
The root cause is because we install both projects one by one, not use the parent mvn.
I have added the instruction to install the parent pom file. Let's check if the problem can be solved.

@zhouyuan
Copy link
Collaborator

@weiting-chen below change should fix the missing parent pom i think

-          cd arrow-data-source
-          mvn clean install -DskipTests -Dbuild_arrow=OFF
-          cd ..
+          mvn clean install -pl arrow-data-source -am -DskipTests -Dbuild_arrow=OFF
           mvn clean package -am -pl native-sql-engine/core -DskipTests -Dbuild_arrow=OFF

@weiting-chen
Copy link
Collaborator Author

@weiting-chen below change should fix the missing parent pom i think

-          cd arrow-data-source
-          mvn clean install -DskipTests -Dbuild_arrow=OFF
-          cd ..
+          mvn clean install -pl arrow-data-source -am -DskipTests -Dbuild_arrow=OFF
           mvn clean package -am -pl native-sql-engine/core -DskipTests -Dbuild_arrow=OFF

+ mvn clean install -N
cd arrow-data-source

  •     mvn clean install -DskipTests

  •    mvn clean install -DskipTests -Dbuild_arrow=OFF
    cd ..
    mvn clean package -am -pl native-sql-engine/core -DskipTests -Dbuild_arrow=OFF

I add the parent pom install before arrow-data-source, just in case the error coming from arrow-data-source compile.
The testing has been passed, if no other problem, we can merge the code.

@github-actions
Copy link


zhouyuan
zhouyuan approved these changes Apr 13, 2021
View reviewed changes
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.

👍

@weiting-chen weiting-chen merged commit 566048a into oap-project:master Apr 13, 2021
zhouyuan pushed a commit to zhouyuan/native-sql-engine that referenced this pull request Apr 15, 2021
@weiting-chen @zhouyuan
[NSE-176]Arrow install order issue (oap-project#231)
c2205a8 
* Add Arrow install script

* [NSE-176]Add Arrow install Script in Arrow Data Source

* remove popd parameter for ubuntu

* Change variables to BFS

* Update Arrow Header and Find Arrow function

* Fix one issue with reading wrong Arrow Path

* Update ARROW_CSV=ON

* Update build_arrow=OFF in Scala UT

* Install parent pom file in Scala UT
zhouyuan added a commit that referenced this pull request Apr 19, 2021
@zhouyuan @JkSelf
@zhztheplayer @weiting-chen
[NSE-186]backport to 1.1 branch batch3 (#252)
c96aebb 
* [NSE-229] Fix the deprecated code warning in shuffle_split_test (#230)

* fix the deprecated code warning in shuffle_split_test

* fix the code style

* format update

* [NSE-239] Adopt ARROW-7011 (#240)

* [NSE-224] update third party code (#242)

* update third party code

Signed-off-by: Yuan Zhou <yuan.zhou@intel.com>

* fix format

Signed-off-by: Yuan Zhou <yuan.zhou@intel.com>

* [NSE-176]Arrow install order issue (#231)

* Add Arrow install script

* [NSE-176]Add Arrow install Script in Arrow Data Source

* remove popd parameter for ubuntu

* Change variables to BFS

* Update Arrow Header and Find Arrow function

* Fix one issue with reading wrong Arrow Path

* Update ARROW_CSV=ON

* Update build_arrow=OFF in Scala UT

* Install parent pom file in Scala UT

* [NSE-196] clean up native sql options (#215)

* clean up native sql options

Signed-off-by: Yuan Zhou <yuan.zhou@intel.com>

* adding more options

Signed-off-by: Yuan Zhou <yuan.zhou@intel.com>

* adding more options

Signed-off-by: Yuan Zhou <yuan.zhou@intel.com>

* adding warning log for running on non-intel cpu

Signed-off-by: Yuan Zhou <yuan.zhou@intel.com>

* [NSE-206]doc update on feature support status (#253)

* update operators support status

Signed-off-by: Yuan Zhou <yuan.zhou@intel.com>

* update docs on operators supporting status

Signed-off-by: Yuan Zhou <yuan.zhou@intel.com>

* fix

Signed-off-by: Yuan Zhou <yuan.zhou@intel.com>

* [NSE-241] fix hashagg result length (#249)

* fix hashagg result length

Signed-off-by: Yuan Zhou <yuan.zhou@intel.com>

* optimize on getting batch size

Signed-off-by: Yuan Zhou <yuan.zhou@intel.com>

* using fixed sized output len for hashagg

Signed-off-by: Yuan Zhou <yuan.zhou@intel.com>

* fix format

Signed-off-by: Yuan Zhou <yuan.zhou@intel.com>

* [NSE-248] fix arrow dependency order (#259)

* Only read .so.300.0.0

* Fix arroow dataset dependency issue

* Add ARROW_S3=ON, Add symlink copy in CMakeList.

Co-authored-by: JiaKe <ke.a.jia@intel.com>
Co-authored-by: Hongze Zhang <hongze.zhang@intel.com>
Co-authored-by: Wei-Ting Chen <weiting.chen@intel.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zhouyuan zhouyuan zhouyuan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@weiting-chen @zhouyuan