Skip to content
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.

Sign up for GitHub

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

Jump to bottom

GH-37056: [Java] Fix importing an empty data array from c-data #37531

Merged
merged 6 commits into from
Sep 7, 2023
Merged

GH-37056: [Java] Fix importing an empty data array from c-data #37531

merged 6 commits into from
Sep 7, 2023

Conversation

janosik47
Copy link
Contributor

@janosik47 janosik47 commented Sep 3, 2023 •
edited by github-actions bot
Loading

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

@github-actions
Copy link

github-actions bot commented Sep 3, 2023

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

@pitrou
Copy link
Member

pitrou commented Sep 4, 2023

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
lidavidm reviewed Sep 4, 2023
View reviewed changes
Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Java LGTM, I left some nits

java/c/src/test/java/org/apache/arrow/c/ArrowArrayUtilityTest.java Outdated Show resolved Hide resolved
java/c/src/test/java/org/apache/arrow/c/ArrowArrayUtilityTest.java Outdated Show resolved Hide resolved
java/c/src/test/java/org/apache/arrow/c/ArrowArrayUtilityTest.java Outdated Show resolved Hide resolved
java/c/src/test/java/org/apache/arrow/c/ArrowArrayUtilityTest.java Outdated Show resolved Hide resolved
@github-actions 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 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
@lidavidm
Copy link
Member

lidavidm commented Sep 4, 2023

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

@janosik47
Copy link
Contributor Author

janosik47 commented Sep 4, 2023 via email

@pitrou
Copy link
Member

pitrou commented Sep 4, 2023

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

For the record, we can help if necessary.

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Sep 5, 2023
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 6, 2023
pitrou
pitrou approved these changes Sep 7, 2023
View reviewed changes
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@pitrou
Copy link
Member

pitrou commented Sep 7, 2023

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
Copy link
Member

lidavidm commented Sep 7, 2023

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
Copy link
Member

pitrou commented Sep 7, 2023

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
Copy link
Member

pitrou commented Sep 7, 2023

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

So it might deserve investigating before merging?

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

pitrou commented Sep 7, 2023

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

@danepitkin
Copy link
Member

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

@pitrou
Copy link
Member

pitrou commented Sep 7, 2023

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

@pitrou pitrou merged commit 5a78169 into apache:main Sep 7, 2023
14 checks passed
@pitrou pitrou removed the awaiting change review Awaiting change review label Sep 7, 2023
@conbench-apache-arrow
Copy link

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.

loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
@janosik47 @lidavidm @loicalleyne
apacheGH-37056: [Java] Fix importing an empty data array from c-data (a…
87790ed 
…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 <janosik47@gmail.com>
Co-authored-by: Jacek Stania <38670505+janosik47@users.noreply.github.com>
Co-authored-by: David Li <li.davidm96@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
@janosik47 @lidavidm @dgreiss
apacheGH-37056: [Java] Fix importing an empty data array from c-data (a…
b7f09fc 
…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 <janosik47@gmail.com>
Co-authored-by: Jacek Stania <38670505+janosik47@users.noreply.github.com>
Co-authored-by: David Li <li.davidm96@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@janosik47 janosik47 deleted the c_data_java_import_empty_list branch May 2, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lidavidm lidavidm lidavidm left review comments

@pitrou pitrou pitrou approved these changes

Assignees
No one assigned
Labels
Component: Java
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Java][C][Dataset] Exception when importing a vector with empty data array from c-data
4 participants
@janosik47 @pitrou @lidavidm @danepitkin