You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "davisusanibar (via GitHub)" <gi...@apache.org> on 2023/07/28 12:34:38 UTC
[GitHub] [arrow] davisusanibar opened a new pull request, #36929: GH-36927: [Java][Docs] Enable Gandiva build as part of Java maven commands
davisusanibar opened a new pull request, #36929:
URL: https://github.com/apache/arrow/pull/36929
### Rationale for this change
To close: https://github.com/apache/arrow/issues/36927
### What changes are included in this PR?
- Enable MacOS Gandiva build as part of Java maven commands
- Enable Windows ORC build as part of Java maven commands
### Are these changes tested?
Yes:
- Gandiva: mvn generate-resources -Pgenerate-libs-jni-macos-linux -N
- ORC: mvn generate-resources -Pgenerate-libs-jni-windows -N
### Are there any user-facing changes?
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] davisusanibar commented on pull request #36929: GH-36927: [Java][Docs] Enable Gandiva build as part of Java maven commands
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar commented on PR #36929:
URL: https://github.com/apache/arrow/pull/36929#issuecomment-1659184174
Issues open for:
- Enable Gandiva support on Windows OS - https://github.com/apache/arrow/issues/36958
- Improve current cmake java libraries dependencies that needs Protobuf_ROOT and Protobuf_USE_STATIC_LIBS variables - https://github.com/apache/arrow/issues/36957
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #36929: GH-36927: [Java][Docs] Enable Gandiva build as part of Java maven commands
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #36929:
URL: https://github.com/apache/arrow/pull/36929#discussion_r1278068475
##########
docs/source/developers/java/building.rst:
##########
@@ -227,6 +223,7 @@ CMake
-DCMAKE_INSTALL_PREFIX=java-dist \
-DCMAKE_UNITY_BUILD=ON
$ cmake --build cpp-jni --target install --config Release
+ $ export JAVA_JNI_CMAKE_ARGS="-DProtobuf_ROOT=$PWD/cpp-jni/protobuf_ep-install"
Review Comment:
We need to specify this in the following `cmake ...` command line because `JAVA_JNI_CMAKE_ARGS` is only for `ci/scripts/java_*_build.sh`.
##########
java/pom.xml:
##########
@@ -1145,6 +1149,9 @@
-DCMAKE_INSTALL_LIBDIR=lib/${os.detected.arch}
-DCMAKE_INSTALL_PREFIX=java-dist
-DCMAKE_UNITY_BUILD=ON
+ -DPARQUET_BUILD_EXAMPLES=OFF
+ -DPARQUET_BUILD_EXECUTABLES=OFF
+ -DPARQUET_REQUIRE_ENCRYPTION=OFF
Review Comment:
FYI: They are not harmful but they are redundant because they are `OFF` by default.
##########
docs/source/developers/java/building.rst:
##########
@@ -272,6 +271,9 @@ CMake
-DCMAKE_INSTALL_LIBDIR=lib/x86_64 ^
-DCMAKE_INSTALL_PREFIX=java-dist ^
-DCMAKE_UNITY_BUILD=ON ^
+ -DPARQUET_BUILD_EXAMPLES=OFF ^
+ -DPARQUET_BUILD_EXECUTABLES=OFF ^
+ -DPARQUET_REQUIRE_ENCRYPTION=OFF ^
Review Comment:
FYI: They are not harmful but they are redundant because they are `OFF` by default.
##########
java/pom.xml:
##########
@@ -1070,6 +1070,8 @@
-DCMAKE_INSTALL_LIBDIR=lib/${os.detected.arch}
-DCMAKE_INSTALL_PREFIX=${arrow.dataset.jni.dist.dir}
-DCMAKE_PREFIX_PATH=${project.basedir}/../java-dist/lib/${os.detected.arch}/cmake
+ -DProtobuf_USE_STATIC_LIBS=ON
+ -DProtobuf_ROOT=${project.basedir}/../cpp-jni/protobuf_ep-install
Review Comment:
Ah, is this for https://github.com/apache/arrow/blob/main/java/gandiva/CMakeLists.txt#L41 ?
We can use this solution for now but we should find a better solution that doesn't require additional CMake options such as `Protobuf_ROOT` and `Protobuf_USE_STATIC_LIBS`. Could you open an issue for it?
(We may add support for installing bundled `protoc` for this case.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] davisusanibar commented on pull request #36929: GH-36927: [Java][Docs] Enable Gandiva build as part of Java maven commands
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar commented on PR #36929:
URL: https://github.com/apache/arrow/pull/36929#issuecomment-1655622685
Hi @danepitkin please could you help me to validate if this changes works properly on your OS environment?
Steps:
1. Make sure the local folder used by Java mvn commands is clean::
$ cd arrow
$ rm -rf cpp-jni java-dist java-jni
3. Build MacOS artifacts:
$ mvn generate-resources -Pgenerate-libs-jni-macos-linux -N
Thank you in advance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] danepitkin commented on a diff in pull request #36929: GH-36927: [Java][Docs] Enable Gandiva build as part of Java maven commands
Posted by "danepitkin (via GitHub)" <gi...@apache.org>.
danepitkin commented on code in PR #36929:
URL: https://github.com/apache/arrow/pull/36929#discussion_r1278023696
##########
java/pom.xml:
##########
@@ -1070,6 +1070,8 @@
-DCMAKE_INSTALL_LIBDIR=lib/${os.detected.arch}
-DCMAKE_INSTALL_PREFIX=${arrow.dataset.jni.dist.dir}
-DCMAKE_PREFIX_PATH=${project.basedir}/../java-dist/lib/${os.detected.arch}/cmake
+ -DProtobuf_USE_STATIC_LIBS=ON
+ -DProtobuf_ROOT=${project.basedir}/../cpp-jni/protobuf_ep-install
Review Comment:
Just out of curiosity, why are we making this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] danepitkin commented on a diff in pull request #36929: GH-36927: [Java][Docs] Enable Gandiva build as part of Java maven commands
Posted by "danepitkin (via GitHub)" <gi...@apache.org>.
danepitkin commented on code in PR #36929:
URL: https://github.com/apache/arrow/pull/36929#discussion_r1277643951
##########
java/pom.xml:
##########
@@ -1099,13 +1101,14 @@
<properties>
<arrow.dataset.jni.dist.dir>java-dist</arrow.dataset.jni.dist.dir>
<cpp.dependencies.builded>false</cpp.dependencies.builded>
- <ARROW_CSV>ON</ARROW_CSV>
- <ARROW_ORC>OFF</ARROW_ORC>
+ <ARROW_DATASET>ON</ARROW_DATASET>
+ <ARROW_GANDIVA>OFF</ARROW_GANDIVA>
Review Comment:
Or is it not supported on Windows?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou merged pull request #36929: GH-36927: [Java][Docs] Enable Gandiva build as part of Java maven commands
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #36929:
URL: https://github.com/apache/arrow/pull/36929
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] danepitkin commented on pull request #36929: GH-36927: [Java][Docs] Enable Gandiva build as part of Java maven commands
Posted by "danepitkin (via GitHub)" <gi...@apache.org>.
danepitkin commented on PR #36929:
URL: https://github.com/apache/arrow/pull/36929#issuecomment-1655822451
> Hi @danepitkin please could you help me to validate if this changes works properly on your OS environment?
>
> Steps:
>
> 1. Make sure the local folder used by Java mvn commands is clean::
> $ cd arrow
> $ rm -rf cpp-jni java-dist java-jni
> 2. Build MacOS artifacts:
> $ mvn generate-resources -Pgenerate-libs-jni-macos-linux -N
>
> Thank you in advance.
Works for me! I checked our your branch, deleted local artifacts, and can confirm it successfully built dataset/gandiva/orc modules.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #36929: GH-36927: [Java][Docs] Enable Gandiva build as part of Java maven commands
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36929:
URL: https://github.com/apache/arrow/pull/36929#issuecomment-1655618691
:warning: GitHub issue #36927 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] davisusanibar commented on a diff in pull request #36929: GH-36927: [Java][Docs] Enable Gandiva build as part of Java maven commands
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar commented on code in PR #36929:
URL: https://github.com/apache/arrow/pull/36929#discussion_r1277906727
##########
java/pom.xml:
##########
@@ -1099,13 +1101,14 @@
<properties>
<arrow.dataset.jni.dist.dir>java-dist</arrow.dataset.jni.dist.dir>
<cpp.dependencies.builded>false</cpp.dependencies.builded>
- <ARROW_CSV>ON</ARROW_CSV>
- <ARROW_ORC>OFF</ARROW_ORC>
+ <ARROW_DATASET>ON</ARROW_DATASET>
+ <ARROW_GANDIVA>OFF</ARROW_GANDIVA>
Review Comment:
Doesn't have JNI yet because we need to install LLVM to build Gandiva ([Could NOT find LLVM (missing: LLVM_DIR)](https://github.com/apache/arrow/pull/14203#issuecomment-1257161265)).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #36929: GH-36927: [Java][Docs] Enable Gandiva build as part of Java maven commands
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #36929:
URL: https://github.com/apache/arrow/pull/36929#discussion_r1280324005
##########
docs/source/developers/java/building.rst:
##########
@@ -236,7 +232,9 @@ CMake
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_INSTALL_LIBDIR=lib/<your system's architecture> \
-DCMAKE_INSTALL_PREFIX=java-dist \
- -DCMAKE_PREFIX_PATH=$PWD/java-dist
+ -DCMAKE_PREFIX_PATH=$PWD/java-dist \
+ -DProtobuf_USE_STATIC_LIBS=ON \
+ -DProtobuf_ROOT=$PWD/../cpp-jni/protobuf_ep-install
Review Comment:
```suggestion
-DProtobuf_ROOT=$PWD/../cpp-jni/protobuf_ep-install \
-DProtobuf_USE_STATIC_LIBS=ON
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] conbench-apache-arrow[bot] commented on pull request #36929: GH-36927: [Java][Docs] Enable Gandiva build as part of Java maven commands
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #36929:
URL: https://github.com/apache/arrow/pull/36929#issuecomment-1671658882
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 8273f7abd44d4ed582650166de1c6ea59ed759b6.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/15753969469) has more details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] danepitkin commented on a diff in pull request #36929: GH-36927: [Java][Docs] Enable Gandiva build as part of Java maven commands
Posted by "danepitkin (via GitHub)" <gi...@apache.org>.
danepitkin commented on code in PR #36929:
URL: https://github.com/apache/arrow/pull/36929#discussion_r1277632770
##########
java/pom.xml:
##########
@@ -1099,13 +1101,14 @@
<properties>
<arrow.dataset.jni.dist.dir>java-dist</arrow.dataset.jni.dist.dir>
<cpp.dependencies.builded>false</cpp.dependencies.builded>
- <ARROW_CSV>ON</ARROW_CSV>
- <ARROW_ORC>OFF</ARROW_ORC>
+ <ARROW_DATASET>ON</ARROW_DATASET>
+ <ARROW_GANDIVA>OFF</ARROW_GANDIVA>
Review Comment:
I think ARROW_GANDIVA is accidentally set to OFF here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org