-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-16930: [Java] Move CPP ORC JNI code to Java ORC project #13458
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
Conversation
| # arrow_jni | ||
| # | ||
| if(ARROW_ORC) | ||
| add_subdirectory(orc) |
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 we'll want to keep the if(ARROW_ORC) (and move it to the other file)
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.
At the end all cpp/src/jni directory will be deleted
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.
That's not relevant, right? We only want to build ORC support if ORC is actually enabled.
Though, what is the intended new structure? Larry's PR in #13481 shows that we need to update CI/docs as well?
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.
What I mean is: how are we ensuring we only build ORC when ORC is actually enabled in the new structure?
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.
Oh ok clear, let me add that logic on java/adapter/orc/src/main/cpp not in cpp/src/jni/CMakeLists.txt
Related to the intended: For the ticket related to “Consolidate JNI compilation ORC ARROW-16930 / Dataset ARROW-16941”. Currently: ORC JNI / Dataset JNI are inside of ./cpp directory
Changes to Consolidate JNI compilation ORC / Dataset:
1st Phase:
- Move ORC JNI / Dataset JNI from ./cpp to ./java directory
- Delete ORC JNI / Dataset JNI from ./cpp
- Continue building ORC / Dataset from ./cpp as you see at https://github.com/apache/arrow/blob/6bd03d409a687011b8d4b66e491015128f1ae27e/ci/scripts/java_jni_macos_build.sh#L52:L87 with the aggregation that this is using now ./java directory instead of cpp/src/jni directory
2nd Phase:
- Building ORC / Dataset directly from ./java directory as a maven project “mvn clean install ...”
This phase involve lots of changes Java code + CMakeList + CI CD changes
Phase 1 will be implemented by this PR: Dataset (#13481) / ORC (#13458)
Phase 2: We are going to create ticket and maintain that on the backlog
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.
Added ARROW_ORC parameter
|
@github-actions crossbow submit java-jars |
|
Revision: 5378076 Submitted crossbow builds: ursacomputing/crossbow @ actions-c6ba712f3e
|
|
@github-actions crossbow submit java-jars |
|
Revision: 499f0bb Submitted crossbow builds: ursacomputing/crossbow @ actions-4a99bf50b5
|
|
This needs to be rebased since I merged Larry's PR first |
lidavidm
left a comment
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.
LGTM, let's just check CI
…#13458) Authored-by: david dali susanibar arce <davi.sarces@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>
…#13458) Authored-by: david dali susanibar arce <davi.sarces@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>
No description provided.