GH-37056: [Java] Fix importing an empty data array from c-data by janosik47 · Pull Request #37531 · apache/arrow

janosik47 · 2023-09-03T20:25:05Z

Rationale for this change

Fix java lib bug that prevents from importing specific data via c-data interface.
Currently an attempt to load a vector with empty data buffer results in an IllegalStateException error.

What changes are included in this PR?

Updated BufferImportTypeVisitor to correctly handle a situation when underlying c-data array pointer is NULL (0) and the expected length of data is zero (0).

Are these changes tested?

Yes, updated the existing unit tests

Are there any user-facing changes?

No

Closes: [Java][C][Dataset] Exception when importing a vector with empty data array from c-data #37056

github-actions · 2023-09-03T20:25:32Z

⚠️ GitHub issue #37056 has been automatically assigned in GitHub to PR creator.

pitrou · 2023-09-04T08:27:21Z

Thanks @janosik47 . Would you also like to add a new test to https://github.com/apache/arrow/blob/main/java/c/src/test/python/integration_tests.py ?

lidavidm

Java LGTM, I left some nits

java/c/src/test/java/org/apache/arrow/c/ArrowArrayUtilityTest.java

lidavidm · 2023-09-04T19:38:59Z

Just to make sure, do you plan on adding the integration tests in this PR, or later?

janosik47 · 2023-09-04T19:42:31Z

hi - I think I will try to add an integration test ...

…

On Mon, 4 Sept 2023 at 20:39, David Li ***@***.***> wrote: Just to make sure, do you plan on adding the integration tests in this PR, or later? — Reply to this email directly, view it on GitHub <#37531 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJHBBKI6DTDJAXOXYYII26TXYYU57ANCNFSM6AAAAAA4JRCGSA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

pitrou · 2023-09-04T19:47:40Z

hi - I think I will try to add an integration test ...
…

For the record, we can help if necessary.

java/c/src/test/python/integration_tests.py

pitrou

Thanks a lot @janosik47 for diagnosing and fixing this! Let's wait for CI now.

pitrou · 2023-09-07T16:52:05Z

Hmm, I assume the CI failure on AMD64 Windows Server 2022 Java JDK 11 is unrelated, but I'm not sure. @lidavidm could you perhaps take a look?

lidavidm · 2023-09-07T16:54:47Z

That module shouldn't run any JNI code, so I don't think it relates here, but it should be investigated separately. CC @davisusanibar @danepitkin

pitrou · 2023-09-07T17:16:56Z

FTR, I ran the same job again on the main branch and it succeeded:
https://github.com/apache/arrow/actions/runs/6110911458/job/16590554484

pitrou · 2023-09-07T17:44:01Z

... all the while it failed 3 times in a row in this PR.

So it might deserve investigating before merging?

…java Co-authored-by: David Li <[email protected]>

Co-authored-by: David Li <[email protected]>

pitrou · 2023-09-07T17:47:33Z

Ok, I've rebased first to make sure it wasn't due to being based on an old commit.

danepitkin · 2023-09-07T18:36:45Z

Looks like several PRs were failing on this job recently, but is now fixed on main?

pitrou · 2023-09-07T19:03:19Z

Rebasing fixed it, so I assume some older commits had the issue. Will merge!

conbench-apache-arrow · 2023-09-08T22:55:33Z

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 5a78169.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

…pache#37531) ### Rationale for this change Fix java lib bug that prevents from importing specific data via c-data interface. Currently an attempt to load a vector with empty data buffer results in an IllegalStateException error. ### What changes are included in this PR? Updated BufferImportTypeVisitor to correctly handle a situation when underlying c-data array pointer is NULL (0) and the expected length of data is zero (0). ### Are these changes tested? Yes, updated the existing unit tests ### Are there any user-facing changes? No * Closes: apache#37056 Lead-authored-by: Jacek Stania <[email protected]> Co-authored-by: Jacek Stania <[email protected]> Co-authored-by: David Li <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

janosik47 requested a review from lidavidm as a code owner September 3, 2023 20:25

github-actions bot added Component: Java awaiting review Awaiting review labels Sep 3, 2023

lidavidm reviewed Sep 4, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting review Awaiting review awaiting changes Awaiting changes labels Sep 4, 2023

lidavidm changed the title ~~GH-37056 [Java] Fix importing an empty data array from c-data~~ GH-37056: [Java] Fix importing an empty data array from c-data Sep 4, 2023

janosik47 commented Sep 4, 2023

View reviewed changes

java/c/src/test/python/integration_tests.py Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Sep 5, 2023

pitrou reviewed Sep 5, 2023

View reviewed changes

java/c/src/test/python/integration_tests.py Outdated Show resolved Hide resolved

java/c/src/test/python/integration_tests.py Outdated Show resolved Hide resolved

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 6, 2023

pitrou approved these changes Sep 7, 2023

View reviewed changes

janosik47 and others added 5 commits September 7, 2023 19:46

apacheGH-37056 [Java] Fix importing an empty data array from c-data

da67fea

relay on expected capacity

688a0d0

Update java/c/src/test/java/org/apache/arrow/c/ArrowArrayUtilityTest.…

57cdf1b

…java Co-authored-by: David Li <[email protected]>

Apply suggestions from code review

cc4f6f6

Co-authored-by: David Li <[email protected]>

add integration test

1524ef6

rework integration test

2334517

pitrou force-pushed the c_data_java_import_empty_list branch from b4f50dd to 2334517 Compare September 7, 2023 17:47

pitrou merged commit 5a78169 into apache:main Sep 7, 2023

pitrou removed the awaiting change review Awaiting change review label Sep 7, 2023

janosik47 deleted the c_data_java_import_empty_list branch May 2, 2024 19:14

Conversation

janosik47 commented Sep 3, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Sep 3, 2023

Uh oh!

pitrou commented Sep 4, 2023

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lidavidm commented Sep 4, 2023

Uh oh!

janosik47 commented Sep 4, 2023 via email

Uh oh!

pitrou commented Sep 4, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou commented Sep 7, 2023

Uh oh!

lidavidm commented Sep 7, 2023

Uh oh!

pitrou commented Sep 7, 2023

Uh oh!

pitrou commented Sep 7, 2023

Uh oh!

pitrou commented Sep 7, 2023

Uh oh!

danepitkin commented Sep 7, 2023

Uh oh!

pitrou commented Sep 7, 2023

Uh oh!

conbench-apache-arrow bot commented Sep 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

janosik47 commented Sep 3, 2023 •

edited by github-actions bot

Loading