ARROW-7272: [C++][Java][Dataset] JNI bridge between RecordBatch and VectorSchemaRoot by zhztheplayer · Pull Request #10883 · apache/arrow

zhztheplayer · 2021-08-05T05:33:55Z

Added simple utility API to share data between C++ and Java codes. The methods are directly calling C Data Interface API.

Updated Java dataset codes to use the new API instead of passing buffer pointers over JNI.

This is also a dependency of ARROW-11776 (PR #10201).

github-actions · 2021-08-05T05:34:14Z

https://issues.apache.org/jira/browse/ARROW-7272

zhztheplayer · 2021-08-24T02:45:39Z

Hi @kiszk, @emkornfield, @fsaintjacques. Would you like to help review this PR as a dependency of the ongoing ARROW-11776 fix? Thanks a lot. The major code changes are in second commit 1f4805d without the Nit stuffs which is in another one.

kiszk · 2021-08-24T03:09:18Z

We would like to ask you to avoid these types of the cleanup in this huge PR. Increasing # of changed lines makes us harder to review this PR.
What do you think?

Agreed. The Nit changes was now removed from this PR. I'll make another after this one get merged, because now in this PR we should follow the old style

kiszk · 2021-08-24T03:10:23Z

ditto for changing name.

kiszk · 2021-08-29T18:01:40Z

Would it be good to add tests for DeserializeUnsafeFromJava and SerializeUnsafeFromNative to jni_util_test.cc?
Since we use a few jni methods(GetArrayLength, GetByteArrayElements, NewByteArray, SetByteArrayRegion, ...), we could create a mock jni in the test case.

Test cases make us clear to do things in the methods.

zhztheplayer · 2021-08-31T07:29:05Z

Would it be good to add tests for DeserializeUnsafeFromJava and SerializeUnsafeFromNative to jni_util_test.cc?
Since we use a few jni methods(GetArrayLength, GetByteArrayElements, NewByteArray, SetByteArrayRegion, ...), we could create a mock jni in the test case.
Test cases make us clear to do things in the methods.

I can give it a try to add mock based tests in C++ side.

Actually the round-trip test already covered this? https://github.com/apache/arrow/pull/10883/files#diff-e1e74b2b4a2812b4e273413ec2beb7f89bb62f2a4172f832805d68a87a773107R100-R101

kiszk · 2021-09-05T14:18:20Z

Thank you for your try. We are looking forward to seeing it.

The end-to-end test looks good for the coverage. IMHO, unit tests help the review and narrow down the (future) potential problems.

zhztheplayer · 2021-09-05T18:23:11Z

I completely agree that we should add more tests though I was thinking this part of code could stay here in Dataset code for short term (that is also why I didn't put these codes to a common module), as I preferred to integrate Java C ABI which is pending on another proposal, to replace this half-serialization implementation. Anyway as long as we think the C++ mock test is needed here then I would try adding them to this PR. (Once I get free some time to do it. A bit busy these days. Sorry for letting you wait)

zhztheplayer · 2021-09-06T15:48:56Z

@kiszk

I have added a mock test against batch exporting (unsafe serialization) code from C++ side. 0c1d58f

Unsafe importing part will be even tricky so I didn't add test case for that. The function arrow::dataset::jni::DeserializeUnsafeFromJava require for a byte array that was serialized out from Java side utility method. So we will end up adding magic byte inputs if we want to unit test this part of code in C++. Do you have any thoughts around this?

zhztheplayer · 2021-10-09T06:27:59Z

@kiszk Any thoughts about the current code?

@emkornfield Would you like to take a look on this too? I think It would be great if we can bring the Java dataset write feature #10201 to version 6.0.0.

pitrou · 2021-10-14T13:08:27Z

ARROW-12965 is now solved, so this should be simplified to reuse the C data interface.

zhztheplayer · 2021-10-14T13:13:02Z

ARROW-12965 is now solved, so this should be simplified to reuse the C data interface.

Agreed. I'll update the code.

emkornfield · 2021-11-06T00:38:55Z

@zhztheplayer sorry for the delay here, did you get a chance to update this to the C-FFI usage?

zhztheplayer · 2021-11-06T10:48:34Z

@zhztheplayer sorry for the delay here, did you get a chance to update this to the C-FFI usage?

Sorry, haven't got chance to work on it yet. I will try to see if I can make a patch in next week. Thanks.

zhztheplayer · 2021-11-14T16:43:56Z

@emkornfield I have updated the PR. I think the CI might be broken since I haven't changed the scripts yet. But you can review the codes if you want to. I will update CI scripts later some time in this PR. Thanks.

zhztheplayer · 2022-04-25T07:59:16Z

Travis failure doesn't seem to be related to the changes. Other jobs were passed.

zhztheplayer · 2022-04-25T08:10:47Z

@kiszk Can we bring the patch into release 8.0.0? Hopefully it's not too late. Thanks.

kszucs · 2022-04-25T08:15:55Z

@github-actions crossbow submit java*

github-actions · 2022-04-25T08:17:11Z

Revision: 5fdb38b369d5f7daa8b2e483ae9f1b932460c0ee

Submitted crossbow builds: ursacomputing/crossbow @ actions-1945

Task	Status
java-jars

pitrou · 2022-04-25T08:24:29Z

@zhztheplayer Can you update the PR description?

kiszk · 2022-04-25T08:54:12Z

cc @kszucs

kszucs · 2022-04-25T09:19:31Z

@kiszk can you approve the Java changes? Then we can include it in the release.

kiszk · 2022-04-25T09:34:57Z

Sure, let me see.

pitrou · 2022-04-25T09:40:12Z

@lwhite1 You may want to take a look here.

pitrou · 2022-04-25T09:41:08Z

Also @zhztheplayer please rebase on master so that we can fix Travis-CI builds.

…maRoot

zhztheplayer · 2022-04-25T13:00:28Z

Travis is fixed but some new errors emerged which seems to be related to network issues. @pitrou can you help retrigger the failed jobs?

pitrou · 2022-04-25T13:56:06Z

It seems someone else did it for me :-)
(but, yes, Homebrew is often unreliable...)

pitrou · 2022-04-25T14:04:01Z

+ env->ExceptionDescribe();
+ env->ExceptionClear();
+ return Status::Invalid("Error during calling Java code from native code");


Instead of simply dumping the exception, is it possible to add its description to the Status instance that is being returned?

This can be deferred to a later JIRA btw.

Sure. I'll open another ticket for that.

Ok, I opened https://issues.apache.org/jira/browse/ARROW-16329

Assigned to me. Thanks.

lidavidm

LGTM, some minor comments

lidavidm · 2022-04-25T14:25:48Z

@@ -29,14 +29,7 @@
 public interface ScanTask extends AutoCloseable {


N.B. we should eventually remove this class entirely as the corresponding interface no longer exists in C++ ARROW-15745

I can assign to myself. Thanks.

lidavidm · 2022-04-25T14:31:34Z

- for (int i = 0; i < schema->num_fields(); ++i) {
+ std::vector<std::shared_ptr<arrow::Array>> offset_zeroed_arrays;
+ for (int i = 0; i < record_batch->num_columns(); ++i) {
 // TODO: If the array has an offset then we need to de-offset the array


Is this TODO still relevant? It seems it's not a TODO anymore but an explanatory note

It's still valid. The codes still copy and rebuild offset-buffers because Java C Data Interface doesn't implement exporing/importing for offset-buffers too. And I believe to do that we should make systematic change to Java code since Arrow Java didn't implement the same offset semantic comparing to C++.

lidavidm · 2022-04-25T14:32:38Z

- final List<ArrowRecordBatch> ret = stream(scanner.scan())
- .flatMap(t -> stream(t.execute()))
- .collect(Collectors.toList());
 try {


Seems this should actually be a try-with-resources or a try-finally?

I tried to factor out as try-finally but it seems that it better requires some common changes to AutoCloseables.java.
See my draft:
zhztheplayer@c460f67
Do you think we can have another ticket for the changes too?

Ah, no worries then. Thanks for looking.

lwhite1

Looks good to me.

zhztheplayer · 2022-04-27T02:08:50Z

Thanks everyone!

ursabot · 2022-05-02T04:21:36Z

Benchmark runs are scheduled for baseline = d464425 and contender = dc97883. dc97883 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.08% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.59% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] dc97883d ec2-t3-xlarge-us-east-2
[Finished] dc97883d test-mac-arm
[Finished] dc97883d ursa-i9-9960x
[Finished] dc97883d ursa-thinkcentre-m75q
[Finished] d4644254 ec2-t3-xlarge-us-east-2
[Finished] d4644254 test-mac-arm
[Finished] d4644254 ursa-i9-9960x
[Finished] d4644254 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

…ectorSchemaRoot Added simple utility API to share data between C++ and Java codes. The methods are directly calling C Data Interface API. Updated Java dataset codes to use the new API instead of passing buffer pointers over JNI. This is also a dependency of ARROW-11776 (PR apache#10201). Closesapache#10883 from zhztheplayer/ARROW-7272 Authored-by: Hongze Zhang <hongze.zhang@intel.com> Signed-off-by: Antoine Pitrou <antoine@python.org>

github-actionsBot added Component: C++ Component: Java labels Aug 5, 2021

zhztheplayer force-pushed the ARROW-7272 branch 2 times, most recently from 53ed02e to 28742abCompare August 5, 2021 07:39

zhztheplayer mentioned this pull request Aug 5, 2021
WIP: ARROW-11776: [Java][Dataset] Support writing to files within dataset scanner via JNI #10201
Closed

zhztheplayer force-pushed the ARROW-7272 branch 2 times, most recently from 01b6d3d to 1f4805dCompare August 24, 2021 02:39

kiszk reviewed Aug 24, 2021
View reviewed changes

Comment threadcpp/src/jni/dataset/jni_util.cc Outdated
kiszkAug 24, 2021
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto for changing name.
zhztheplayer reacted with thumbs up emoji

zhztheplayer force-pushed the ARROW-7272 branch 4 times, most recently from fd07e19 to 4f09d18Compare August 24, 2021 06:53

zhztheplayer force-pushed the ARROW-7272 branch from 4f09d18 to c004e81Compare September 6, 2021 15:24

zhztheplayer force-pushed the ARROW-7272 branch from c004e81 to 0c1d58fCompare September 6, 2021 16:19

zhztheplayer force-pushed the ARROW-7272 branch from 9508565 to 51e235fCompare October 9, 2021 06:20

zhztheplayer force-pushed the ARROW-7272 branch from 51e235f to f469192Compare November 14, 2021 16:39

zhztheplayer changed the title ~~ARROW-7272: [C++][Java] JNI bridge between RecordBatch and VectorSchemaRoot~~ARROW-7272: [C++][Java][Dataset] JNI bridge between RecordBatch and VectorSchemaRootApr 25, 2022

zhztheplayer added 6 commits April 25, 2022 17:44

ARROW-7272: [C++][Java] JNI bridge between RecordBatch and VectorSche…
aba7b69
…maRoot

fixup
047df06

checkstyle
699e0bf

fixup
21ec0b0

fixup
74ae92a

fixup
879ccb0

zhztheplayer force-pushed the ARROW-7272 branch from 5fdb38b to 879ccb0Compare April 25, 2022 09:44

pitrou reviewed Apr 25, 2022
View reviewed changes

pitrou requested a review from lidavidm April 25, 2022 14:05

lidavidm approved these changes Apr 25, 2022
View reviewed changes

kiszk approved these changes Apr 26, 2022
View reviewed changes

lwhite1 approved these changes Apr 26, 2022
View reviewed changes

pitrou closed this in dc97883Apr 26, 2022

asfimport mentioned this pull request May 2, 2022
[C++][Java][Dataset] JNI bridge between RecordBatch and VectorSchemaRoot #16978
Closed

		@@ -29,14 +29,7 @@
		public interface ScanTask extends AutoCloseable {

Uh oh!

Conversation

zhztheplayer commented Aug 5, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actionsBot commented Aug 5, 2021

Uh oh!

zhztheplayer commented Aug 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhztheplayerAug 24, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk commented Aug 29, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhztheplayer commented Aug 31, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kiszk commented Sep 5, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhztheplayer commented Sep 5, 2021

Uh oh!

zhztheplayer commented Sep 6, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhztheplayer commented Oct 9, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou commented Oct 14, 2021

Uh oh!

zhztheplayer commented Oct 14, 2021

Uh oh!

emkornfield commented Nov 6, 2021

Uh oh!

zhztheplayer commented Nov 6, 2021

Uh oh!

zhztheplayer commented Nov 14, 2021

Uh oh!

zhztheplayer commented Apr 25, 2022

Uh oh!

zhztheplayer commented Apr 25, 2022

Uh oh!

kszucs commented Apr 25, 2022

Uh oh!

github-actionsBot commented Apr 25, 2022

Uh oh!

pitrou commented Apr 25, 2022

Uh oh!

kiszk commented Apr 25, 2022

Uh oh!

kszucs commented Apr 25, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kiszk commented Apr 25, 2022

Uh oh!

pitrou commented Apr 25, 2022

Uh oh!

pitrou commented Apr 25, 2022

Uh oh!

zhztheplayer commented Apr 25, 2022

Uh oh!

pitrou commented Apr 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lidavidm left a comment

zhztheplayer commented Aug 5, 2021•
edited
Loading

zhztheplayerAug 24, 2021•
edited
Loading

kiszk commented Aug 29, 2021•
edited
Loading

zhztheplayer commented Aug 31, 2021•
edited
Loading

kiszk commented Sep 5, 2021•
edited
Loading

zhztheplayer commented Sep 6, 2021•
edited
Loading

zhztheplayer commented Oct 9, 2021•
edited
Loading

kszucs commented Apr 25, 2022•
edited
Loading

zhztheplayerApr 25, 2022•
edited
Loading