-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
GH-36927: [Java][Docs] Enable Gandiva build as part of Java maven commands #36929
Conversation
|
Hi @danepitkin please could you help me to validate if this changes works properly on your OS environment? Steps:
Thank you in advance. |
<ARROW_CSV>ON</ARROW_CSV> | ||
<ARROW_ORC>OFF</ARROW_ORC> | ||
<ARROW_DATASET>ON</ARROW_DATASET> | ||
<ARROW_GANDIVA>OFF</ARROW_GANDIVA> |
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.
I think ARROW_GANDIVA is accidentally set to OFF here
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.
Or is it not supported on Windows?
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.
Doesn't have JNI yet because we need to install LLVM to build Gandiva (Could NOT find LLVM (missing: LLVM_DIR)).
Works for me! I checked out your branch, deleted local artifacts, and can confirm it successfully built dataset/gandiva/orc modules. |
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.
Overall LGTM. Thanks @davisusanibar !
-DProtobuf_USE_STATIC_LIBS=ON | ||
-DProtobuf_ROOT=${project.basedir}/../cpp-jni/protobuf_ep-install |
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.
Just out of curiosity, why are we making this change?
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.
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.)
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.
+1
java/pom.xml
Outdated
-DPARQUET_BUILD_EXAMPLES=OFF | ||
-DPARQUET_BUILD_EXECUTABLES=OFF | ||
-DPARQUET_REQUIRE_ENCRYPTION=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.
FYI: They are not harmful but they are redundant because they are OFF
by default.
-DPARQUET_BUILD_EXAMPLES=OFF ^ | ||
-DPARQUET_BUILD_EXECUTABLES=OFF ^ | ||
-DPARQUET_REQUIRE_ENCRYPTION=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.
FYI: They are not harmful but they are redundant because they are OFF
by default.
@@ -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" |
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.
We need to specify this in the following cmake ...
command line because JAVA_JNI_CMAKE_ARGS
is only for ci/scripts/java_*_build.sh
.
-DProtobuf_USE_STATIC_LIBS=ON | ||
-DProtobuf_ROOT=${project.basedir}/../cpp-jni/protobuf_ep-install |
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.
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.)
Issues open for:
|
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.
+1
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 8273f7a. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
…en commands (apache#36929) ### Rationale for this change To close: apache#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 * Closes: apache#36927 Lead-authored-by: david dali susanibar arce <davi.sarces@gmail.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…en commands (apache#36929) ### Rationale for this change To close: apache#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 * Closes: apache#36927 Lead-authored-by: david dali susanibar arce <davi.sarces@gmail.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Rationale for this change
To close: #36927
What changes are included in this PR?
Are these changes tested?
Yes:
Are there any user-facing changes?
No