ARROW-16941: [Java] Consolidate Dataset JNI compilation #13481
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
Contributor
Author
|
@lidavidm @davisusanibar Would you please review these changes? Thank you. |
Member
|
@github-actions crossbow submit java-jars |
lidavidm
approved these changes
Jun 30, 2022
|
Revision: 558ed60 Submitted crossbow builds: ursacomputing/crossbow @ actions-58e01587a4
|
lidavidm
requested changes
Jun 30, 2022
Member
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
Hmm, the CI fails. We need to update the CI:
arrow/ci/scripts/java_jni_manylinux_build.sh
Line 120 in 8b298be
Also, we should presumably update the install instructions to match.
Contributor
Author
I will take a look. I see the Conda integration test also failed. |
Contributor
Contributor
Author
|
@lidavidm The error message I see (many linux build) doesn't look to me to have any direct connection to Java. (see below) Does this kind of thing happen randomly at all? There's also a failure on Conda integration, which I'll look at now. |
Member
|
@lwhite1 ignore the integration test. The important one is the JNI pipeline. After this reorganization, what is the expected process to build and test the JNI libraries? |
Contributor
Author
oh that. IDK where that came from. When I built locally, it failed so I switched to master and did a clean build and it failed again. I assumed it was a local issue and the CI builds were unaffected. The issue is that the c-data maven module is expecting a variable with one name (for the build dir) and the other three cpp modules expect a different name for the same value. If you run the c-data-module on its own with the expected argument, it works fine. However, the build that creates the other JNI libs also calls the c-data module under the hood, and then it fails - I believe it's because it can't find the directory where the lib file should be. Locally, I got it to work in two ways: First, after building c-data separately, I deleted that step from the other JNI build and everything was fine as it used the previously compiled code. However, I was afraid to commit that change, thinking it would break the CI build. Instead I called the "other JNI" build passing the same value using both argument names. That also worked. I spent some time trying to figure out why this was happening. I couldn't see any related code changes since May so I thought it might just be my environment. We talked about the fact that the c-data-interface was built redundantly in the command line instructions, and whether one version should be removed, but it seemed risky to make that change.
I didn't (consciously) change anything that would require process changes. I will look at the maven build again and see if I can't figure out how to pass the build dir to the c-data module using the name it expects. |
Member
|
Ok. I'm not sure what I was thinking earlier. Based on the description that @davisusanibar gave in #13458, I think he has the right approach to this and that explains why the other PR passes CI. |
Member
|
The CMake project currently under java/dataset is just for generating the JNI header so I don't think it needs modifications right now |
lidavidm
reviewed
Jun 30, 2022
cpp/CMakeLists.txt
Outdated
|
|
||
| if(ARROW_JNI) | ||
| add_subdirectory(src/jni) | ||
| add_subdirectory(../java/dataset/src/main/cpp ./java/jni) |
Member
There was a problem hiding this comment.
Similar to the other PR, this still needs to be conditioned on if(ARROW_DATASET) right?
Contributor
Author
the change here was to reset it to the original state |
lidavidm
reviewed
Jul 1, 2022
cpp/CMakeLists.txt
Outdated
Comment on lines
983
to
985
| if(ARROW_DATASET) | ||
| add_subdirectory(../java/dataset/src/main/cpp ./java/jni) | ||
| endif() |
Member
There was a problem hiding this comment.
Please use spaces, not tabs, I think that's why the indentation is off:
Suggested change
| if(ARROW_DATASET) | |
| add_subdirectory(../java/dataset/src/main/cpp ./java/jni) | |
| endif() | |
| if(ARROW_DATASET) | |
| add_subdirectory(../java/dataset/src/main/cpp ./java/jni) | |
| endif() |
Member
|
@github-actions crossbow submit java-jars |
|
Revision: f5226ec Submitted crossbow builds: ursacomputing/crossbow @ actions-6e6c6a526d
|
lidavidm
approved these changes
Jul 1, 2022
lidavidm
changed the title
kou
added a commit
to kou/arrow
that referenced
this pull request
Jul 4, 2022
This is a follow-up of apache#13481. We also need to update jni_util.h path in java/dataset/src/main/cpp/jni_util_test.cc. Removing "./" is needless but it's redundant.
kou
added a commit
that referenced
this pull request
Jul 5, 2022
This is a follow-up of #13481. We also need to update jni_util.h path in java/dataset/src/main/cpp/jni_util_test.cc. Removing "./" is needless but it's redundant. Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
vibhatha
pushed a commit
to vibhatha/arrow
that referenced
this pull request
Jul 5, 2022
Moves the Dataset JNI C++ code from the arrow/cpp project to the arrow/java project, updating the CMakeLists.txt files as needed. Authored-by: Larry White <lwhite1@users.noreply.github.com> Signed-off-by: David Li <li.davidm96@gmail.com>
drin
pushed a commit
to drin/arrow
that referenced
this pull request
Jul 5, 2022
This is a follow-up of apache#13481. We also need to update jni_util.h path in java/dataset/src/main/cpp/jni_util_test.cc. Removing "./" is needless but it's redundant. Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
pribor
pushed a commit
to GlobalWebIndex/arrow
that referenced
this pull request
Oct 24, 2025
Moves the Dataset JNI C++ code from the arrow/cpp project to the arrow/java project, updating the CMakeLists.txt files as needed. Authored-by: Larry White <lwhite1@users.noreply.github.com> Signed-off-by: David Li <li.davidm96@gmail.com>
pribor
pushed a commit
to GlobalWebIndex/arrow
that referenced
this pull request
Oct 24, 2025
This is a follow-up of apache#13481. We also need to update jni_util.h path in java/dataset/src/main/cpp/jni_util_test.cc. Removing "./" is needless but it's redundant. Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Moves the Dataset JNI C++ code from the arrow/cpp project to the arrow/java project, updating the CMakeLists.txt files as needed.