WIP: ARROW-11776: [Java][Dataset] Support writing to files within dataset scanner via JNI #10201

zhztheplayer · 2021-04-29T16:39:13Z

Sorry for messing up PR list but the previous PR is accidentally closed and not able to reopen. Let's use this one instead.

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

zhztheplayer · 2021-04-29T17:06:47Z

cpp/src/jni/dataset/proto/record_batch.proto

@emkornfield
I managed to migrate to existing flatbuffers API from protobuf.

previous comment #10108 (comment)

github-actions · 2021-04-29T18:37:53Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW

Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

kiszk · 2021-05-15T15:47:22Z

java/dataset/src/main/java/org/apache/arrow/dataset/jni/JniWrapper.java

nit: spaces are not necessary

Thanks! But I don't see we always make consistent practices of Javadoc alignment in Arrow Java code. E.g.

arrow/java/gandiva/src/main/java/org/apache/arrow/gandiva/evaluator/JniWrapper.java

Lines 34 to 40 in 78c88a9

* @param schemaBuf The schema serialized as a protobuf. See Types.proto

* to see the protobuf specification

* @param exprListBuf The serialized protobuf of the expression vector. Each

* expression is created using TreeBuilder::MakeExpression.

* @param selectionVectorType type of selection vector

* @param configId Configuration to gandiva.

* @return A moduleId that is passed to the evaluateProjector() and closeProjector() methods

Maybe what we need here is to add a relevant rule to checkstyle config and do a complete clean up?

emkornfield · 2021-05-20T05:58:51Z

cpp/src/jni/dataset/jni_util.cc

nit: can you prefix org with "::"

Thanks but did you mean to use namespace flatbuf = ::org::apache::arrow::flatbuf;? I did a search in our C++ code base using RegEx namespace \w* = , and it seems to be a common practice not adding the leading ::

[root@localhost arrow]# grep -R "namespace [a-z A-Z][a-z A-Z]* =.*" cpp/src/ cpp/src/parquet/column_reader.cc:namespace BitUtil = arrow::BitUtil; cpp/src/parquet/column_writer.cc:namespace BitUtil = arrow::BitUtil; cpp/src/parquet/encoding_test.cc:namespace BitUtil = arrow::BitUtil; cpp/src/parquet/arrow/reader.cc:namespace BitUtil = arrow::BitUtil; cpp/src/parquet/arrow/reader_internal.cc:namespace BitUtil = arrow::BitUtil; cpp/src/parquet/encoding.cc:namespace BitUtil = arrow::BitUtil; cpp/src/parquet/column_writer_test.cc:namespace BitUtil = arrow::BitUtil; cpp/src/parquet/statistics_test.cc:namespace BitUtil = arrow::BitUtil; cpp/src/jni/dataset/jni_util.cc:namespace flatbuf = org::apache::arrow::flatbuf; cpp/src/arrow/ipc/json_simple.cc:namespace rj = arrow::rapidjson; cpp/src/arrow/ipc/metadata_internal.cc:namespace flatbuf = org::apache::arrow::flatbuf; cpp/src/arrow/ipc/reader.cc:namespace flatbuf = org::apache::arrow::flatbuf; cpp/src/arrow/ipc/metadata_internal.h:namespace flatbuf = org::apache::arrow::flatbuf; cpp/src/arrow/compute/kernels/aggregate_benchmark.cc:namespace BitUtil = arrow::BitUtil; cpp/src/arrow/filesystem/s3_test_util.h:namespace bp = boost::process; cpp/src/arrow/filesystem/s3fs_test.cc:namespace bp = boost::process; cpp/src/arrow/gpu/cuda_arrow_ipc.cc:namespace flatbuf = org::apache::arrow::flatbuf; cpp/src/arrow/flight/flight_benchmark.cc:namespace perf = arrow::flight::perf; cpp/src/arrow/flight/test_util.cc:namespace bp = boost::process; cpp/src/arrow/flight/test_util.cc:namespace fs = boost::filesystem; cpp/src/arrow/flight/test_util.cc: namespace fs = boost::filesystem; cpp/src/arrow/flight/internal.h:namespace pb = arrow::flight::protocol; cpp/src/arrow/flight/server.cc:namespace pb = arrow::flight::protocol; cpp/src/arrow/flight/serialization_internal.cc:namespace pb = arrow::flight::protocol; cpp/src/arrow/flight/client.cc:namespace pb = arrow::flight::protocol; cpp/src/arrow/flight/client.cc: namespace ge = GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS; cpp/src/arrow/flight/perf_server.cc:namespace perf = arrow::flight::perf; cpp/src/arrow/flight/perf_server.cc:namespace proto = arrow::flight::protocol; cpp/src/arrow/flight/flight_test.cc:namespace pb = arrow::flight::protocol; cpp/src/arrow/adapters/orc/adapter.cc:namespace liborc = orc; cpp/src/arrow/adapters/orc/adapter_util.h:namespace liborc = orc; cpp/src/arrow/adapters/orc/adapter_test.cc:namespace liborc = orc; cpp/src/arrow/adapters/orc/adapter_util.cc:namespace liborc = orc; cpp/src/arrow/json/object_parser.cc:namespace rj = arrow::rapidjson; cpp/src/arrow/json/test_common.h:namespace rj = arrow::rapidjson; cpp/src/arrow/json/chunker.cc:namespace rj = arrow::rapidjson; cpp/src/arrow/json/parser.cc:namespace rj = arrow::rapidjson; cpp/src/arrow/json/object_writer.cc:namespace rj = arrow::rapidjson; cpp/src/arrow/testing/json_internal.h:namespace rj = arrow::rapidjson; cpp/src/plasma/store.cc:namespace fb = plasma::flatbuf; cpp/src/plasma/client.cc:namespace fb = plasma::flatbuf; cpp/src/plasma/common.cc:namespace fb = plasma::flatbuf; cpp/src/plasma/plasma.cc:namespace fb = plasma::flatbuf; cpp/src/plasma/test/serialization_tests.cc:namespace fb = plasma::flatbuf; cpp/src/plasma/protocol.cc:namespace fb = plasma::flatbuf;

emkornfield · 2021-05-20T05:59:11Z

cpp/src/jni/dataset/jni_util.cc

please try to avoid these types of cleanups with large functional changes.

Sorry about that. :( Will make split changes next time.

I agree with @emkornfield. To split these types of changes into another PR make us easier to review this PR at this time.

Hello @kiszk , this part of code (Cross JNI data sharing) was moved to #10883 as a dependency of this PR, would you like to have a look? Thank a lot. I have added a separated style fix commit in that one.

Or you think we should have a individual PR rather than a commit to fix the style stuffs? I am okay to either way.

Sure, let me take a look.

emkornfield · 2021-05-20T06:03:00Z

cpp/src/jni/dataset/jni_util.cc

including the message with Invalid could aid users. Status's have a member for arbitrary details. it might pay to create a new extension of this class that can keep a reference to the exception?

Fixed in commit 922d6dd143c12984daa788d9d737a75914e8a678.

emkornfield · 2021-05-20T06:06:45Z

cpp/src/jni/dataset/jni_util.cc

what is the performance overhead of this. Could another approach be to use to record batches. One that contains the data, and one that contains all the reference pointers?

Or at the very least only one metadata entry and encode all the buffer references as some sort of flatbuffer, protobuf or json list?

Fixed commit 8ef07be7b42cc9a4e83326f6a785457d664678ee.

Here I chose to encode the references within json arrays. It might be possible to use extra arrow arrays/recordbatch to store the reference pointers but we still have to manage the reference pointers to the ref arrays/recordbatch themselves. Let's have a rework on this within implementing C data interface for Java in future.

emkornfield · 2021-05-20T06:08:47Z

cpp/src/jni/dataset/jni_wrapper.cc

CC @lidavidm @westonpace what is the current status of changes in cardinality for ScanTask? is this a use that should be supported long term?

ScanTask is going away. However, from a quick glance, what we want here is a Scanner from a RecordBatchIterator. Use the existing Scanner::FromRecordBatchReader:

arrow/cpp/src/arrow/dataset/scanner.h

Lines 323 to 329 in 8b0e049

/// \brief Make a scanner from a record batch reader.

///

/// The resulting scanner can be scanned only once. This is intended

/// to support writing data from streaming sources or other sources

/// that can be iterated only once.

static std::shared_ptr<ScannerBuilder> FromRecordBatchReader(

std::shared_ptr<RecordBatchReader> reader);

Fixed in 71cba25. Thanks for adding the facility.

emkornfield

It looks like there might be two changes in one here, that potentially could be split up and ease reviewing.

Refactoring to allow passing native java buffers to C++ code. For this, another option that might be simpler/more reliable is to try to use the Arrow C ABI to translate the data across java and C++. Did you consider this?
Allowing writes.

Is this right? would it be hard to make these changes separately.

zhztheplayer · 2021-05-22T13:05:57Z

@emkornfield

Refactoring to allow passing native java buffers to C++ code. For this, another option that might be simpler/more reliable is to try to use the Arrow C ABI to translate the data across java and C++. Did you consider this?

Once we had a short discussion around this https://issues.apache.org/jira/browse/ARROW-7272?focusedCommentId=16983849&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16983849

C data interface does sound to be better. Although I believe a Java implementation will require a design that probably tightly relies on JNI which makes the implementation slightly more complex than in other language's case. I think we can open a separate JIRA ticket for that to do the work in future as we don't yet have the implementation in Java. So far I have extracted the commit that refactored current code within flatbuffers and marked it to resolve the issue ARROW-7272 (if it's OK I may open an individual PR within the commit for 7272). What do you think?

zhztheplayer · 2021-05-22T13:08:50Z

JNI CI failure is related to an existing issue https://issues.apache.org/jira/browse/ARROW-12838

zhztheplayer · 2021-06-28T08:43:47Z

Hi @emkornfield, do you have any other comments on the current code? Sorry for pinging you again as I would like to see if we can make this into 5.0.0.

To ease review, the changes was split into two parts:

Refactoring to allow passing native java buffers to C++ code:
4a0ddbe (the first commit)
Allowing writes:
Other commits. diff

Thanks!

…maRoot

…scanner via JNI

zhztheplayer · 2021-08-05T10:29:56Z

Part of this work around ARROW-7272 has been moved to another PR #10883.

github-actions · 2021-08-24T02:32:34Z

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

zhztheplayer · 2021-08-24T02:32:40Z

Marked title as WIP to let #10883 to be reviewed first.

kszucs · 2022-04-21T18:34:04Z

@zhztheplayer shall we close this as stale?

…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 #10201). Closes #10883 from zhztheplayer/ARROW-7272 Authored-by: Hongze Zhang <hongze.zhang@intel.com> Signed-off-by: Antoine Pitrou <antoine@python.org>

zhztheplayer · 2022-04-27T04:37:05Z

@zhztheplayer shall we close this as stale?

I suggest to keep it open for a while, as its dependency #10108 has been merged. I'm going to update this PR before 9.0.0.

pitrou · 2022-05-04T14:53:08Z

Also cc @lwhite1 for information and in case he's interested.

zhztheplayer · 2022-09-09T08:07:29Z

Sorry still not ready to rework this. Will reopen before I get started.

…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). Closes apache#10883 from zhztheplayer/ARROW-7272 Authored-by: Hongze Zhang <hongze.zhang@intel.com> Signed-off-by: Antoine Pitrou <antoine@python.org>

zhztheplayer mentioned this pull request Apr 29, 2021

ARROW-11776: [Java][Dataset] Support writing to files within dataset scanner via JNI #10108

Closed

zhztheplayer commented Apr 29, 2021

View reviewed changes

github-actions bot added Component: C++ Component: Java labels Apr 29, 2021

kiszk reviewed May 15, 2021

View reviewed changes

emkornfield reviewed May 20, 2021

View reviewed changes

emkornfield requested changes May 20, 2021

View reviewed changes

zhztheplayer force-pushed the ARROW-11776 branch from c16e0cb to 8ef07be Compare May 22, 2021 12:18

zhztheplayer marked this pull request as draft May 25, 2021 01:43

zhztheplayer marked this pull request as ready for review May 27, 2021 02:32

zhztheplayer force-pushed the ARROW-11776 branch 2 times, most recently from 9b9319b to cc13ac2 Compare June 28, 2021 07:31

zhztheplayer requested a review from emkornfield June 28, 2021 08:32

zhztheplayer force-pushed the ARROW-11776 branch from cc13ac2 to 0476994 Compare July 22, 2021 02:33

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

7729a0a

…maRoot

zhztheplayer force-pushed the ARROW-11776 branch from 0476994 to fcf1e14 Compare August 5, 2021 05:31

zhztheplayer mentioned this pull request Aug 5, 2021

ARROW-7272: [C++][Java][Dataset] JNI bridge between RecordBatch and VectorSchemaRoot #10883

Closed

zhztheplayer force-pushed the ARROW-11776 branch 2 times, most recently from b7fcd97 to 0acf50d Compare August 5, 2021 06:42

zhztheplayer added 3 commits August 5, 2021 15:38

style

28742ab

ARROW-11776: [Java][Dataset] Support writing to files within dataset …

9ac17b3

…scanner via JNI

Use ScannerBuilder::FromRecordBatchReader

7bce23a

zhztheplayer added 3 commits August 5, 2021 15:38

JNI error handling

17c18ed

style

445ffdf

fixup

cc5ea4f

zhztheplayer force-pushed the ARROW-11776 branch from 0acf50d to cc5ea4f Compare August 5, 2021 07:40

dependency

3960696

zhztheplayer changed the title ~~ARROW-11776: [Java][Dataset] Support writing to files within dataset scanner via JNI~~ WIP: ARROW-11776: [Java][Dataset] Support writing to files within dataset scanner via JNI Aug 24, 2021

zhztheplayer closed this Sep 9, 2022

This was referenced Oct 29, 2022

[Java][Dataset] Support writing to files within dataset scanner via JNI #27628

Closed

[Java] Java implementation of Arrow C data interface #28685

Closed

	* @param schemaBuf The schema serialized as a protobuf. See Types.proto
	* to see the protobuf specification
	* @param exprListBuf The serialized protobuf of the expression vector. Each
	* expression is created using TreeBuilder::MakeExpression.
	* @param selectionVectorType type of selection vector
	* @param configId Configuration to gandiva.
	* @return A moduleId that is passed to the evaluateProjector() and closeProjector() methods

	/// \brief Make a scanner from a record batch reader.
	///
	/// The resulting scanner can be scanned only once. This is intended
	/// to support writing data from streaming sources or other sources
	/// that can be iterated only once.
	static std::shared_ptr<ScannerBuilder> FromRecordBatchReader(
	std::shared_ptr<RecordBatchReader> reader);

WIP: ARROW-11776: [Java][Dataset] Support writing to files within dataset scanner via JNI #10201

WIP: ARROW-11776: [Java][Dataset] Support writing to files within dataset scanner via JNI #10201

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 29, 2021

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!

Choose a reason for hiding this comment

Uh oh!

kiszk Aug 23, 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!

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!

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!

emkornfield left a comment

Choose a reason for hiding this comment

Uh oh!

zhztheplayer commented May 22, 2021

Uh oh!

zhztheplayer commented May 22, 2021

Uh oh!

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

Uh oh!

Uh oh!

zhztheplayer commented Aug 5, 2021

Uh oh!

github-actions bot commented Aug 24, 2021

Uh oh!

zhztheplayer commented Aug 24, 2021

Uh oh!

kszucs commented Apr 21, 2022

Uh oh!

zhztheplayer commented Apr 27, 2022

Uh oh!

pitrou commented May 4, 2022

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zhztheplayer commented Apr 29, 2021 •

edited

Loading

kiszk Aug 23, 2021 •

edited

Loading

zhztheplayer commented Jun 28, 2021 •

edited

Loading

zhztheplayer commented Sep 9, 2022 •

edited

Loading