GH-34610: [Java] Fix valueCount when loading NullVector #34611

sunchao · 2023-03-17T17:02:29Z

We should set valueCount and nullCount when loading NullVector.

Rationale for this change

When importing a NullVector, for example, via Arrow C interface, the valueCount and nullCount is currently not set properly, which will result both values to be 0.

What changes are included in this PR?

Set valueCount when loading the vector.

Are these changes tested?

Added a test. Also had to add extra constructor to facilitate the test.

Are there any user-facing changes?

No.

Closes: [Java] When loading NullVector, valueCount should be properly set #34610

github-actions · 2023-03-17T17:02:52Z

Closes: [Java] When loading NullVector, valueCount should be properly set #34610

github-actions · 2023-03-17T17:02:55Z

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

We should set `valueCount` when loading `NullVector`. Unfortunately there's no `nullCount` field.

viirya · 2023-03-17T17:26:23Z

Error:  Errors: 
Error:    TestBasicOperation.getStreamLargeBatch:273->test:315->test:333 � IllegalState Memory was leaked by query. Memory leaked: (134217728)
Allocator(ROOT) 0/134217728/270532608/9223372036854775807 (res/actual/peak/limit)

Hmm, is this related?

lidavidm · 2023-03-17T17:27:07Z

No, there is a long standing issue for it that nobody has had time to look at.

lidavidm · 2023-03-17T17:46:11Z

The JNI pipeline failure does seem related, though

viirya · 2023-03-17T17:47:36Z

yea, that's the added test testNullVector.

Error:  org.apache.arrow.c.RoundtripTest.testNullVector  Time elapsed: 0.008 s  <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <true> but was: <false>

sunchao · 2023-03-17T17:50:13Z

Hmm thanks, let me take a look.

amol- · 2023-03-30T17:28:18Z

Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍

amol- · 2023-04-13T15:24:42Z

@sunchao are you still planning to take a look at the failing test? The PR has been inactive for the past month, it would be nice to lead it to merge if we have the chance to.

danepitkin · 2023-05-12T17:50:18Z

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

+  @Test
+  public void testNullVector() {
+    try (final NullVector vector = new NullVector("v", 1024)) {
+      assertTrue(roundtrip(vector, NullVector.class));


Hmm, its not obvious to me why this test is failing. I would've expected your change to loadFieldBuffers() to work. Maybe the field comparison is failing? I think you could try passing a custom TypeEqualsVisitor with checkName or checkMetadata disabled to narrow things down on line 155.

return VectorEqualsVisitor.vectorEquals(vector, imported, <custom_visitor_here>);

sunchao · 2023-05-12T17:54:31Z

Sorry @amol- and @danepitkin , we found a workaround to this issue internally so I haven't got a chance to re-visit the test failure. It's indeed weird and I plan to reproduce it locally and see what happened.

danepitkin · 2023-05-12T18:06:29Z

Sorry @amol- and @danepitkin , we found a workaround to this issue internally so I haven't got a chance to re-visit the test failure. It's indeed weird and I plan to reproduce it locally and see what happened.

No worries! I noticed the Java implementation has been a bit quiet in general, so its great to see new fixes coming in.

danepitkin · 2023-11-29T02:47:00Z

I've followed this PR up here: #38973

The issue was in NullVector.getTransferPair(). It was not passing the name when transferring, which resulted in this check failing when comparing field names:

arrow/java/vector/src/main/java/org/apache/arrow/vector/compare/TypeEqualsVisitor.java

Line 134 in 61a4a80

    
           return (!checkName || Objects.equals(leftField.getName(), rightField.getName())) &&

I created a new PR since I didn't have permissions to push to your branch. I hope you don't mind!

…ing NullVector (#38973) ### Rationale for this change This is a follow up of #34611 (thanks @ sunchao) We should set `valueCount` when loading `NullVector`. Unfortunately there's no `nullCount` field. ### What changes are included in this PR? * `valueCount` added to NullVector * `name` added to NullVector's TransferPair ### Are these changes tested? Yes. unit tested. ### Are there any user-facing changes? Yes, a NullVector with a non-null name will maintain the name when transferred via `TransferPair` instead of defaulting to the default name `$data$`. * Closes: #34610 Authored-by: Chao Sun <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

…nsferring NullVector (apache#38973) ### Rationale for this change This is a follow up of apache#34611 (thanks @ sunchao) We should set `valueCount` when loading `NullVector`. Unfortunately there's no `nullCount` field. ### What changes are included in this PR? * `valueCount` added to NullVector * `name` added to NullVector's TransferPair ### Are these changes tested? Yes. unit tested. ### Are there any user-facing changes? Yes, a NullVector with a non-null name will maintain the name when transferred via `TransferPair` instead of defaulting to the default name `$data$`. * Closes: apache#34610 Authored-by: Chao Sun <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

…ing NullVector (#38973) ### Rationale for this change This is a follow up of apache/arrow#34611 (thanks @ sunchao) We should set `valueCount` when loading `NullVector`. Unfortunately there's no `nullCount` field. ### What changes are included in this PR? * `valueCount` added to NullVector * `name` added to NullVector's TransferPair ### Are these changes tested? Yes. unit tested. ### Are there any user-facing changes? Yes, a NullVector with a non-null name will maintain the name when transferred via `TransferPair` instead of defaulting to the default name `$data$`. * Closes: #34610 Authored-by: Chao Sun <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

sunchao requested a review from lidavidm as a code owner March 17, 2023 17:02

github-actions bot added Component: Java awaiting committer review Awaiting committer review labels Mar 17, 2023

Fix valueCount when loading NullVector

9de06ae

We should set `valueCount` when loading `NullVector`. Unfortunately there's no `nullCount` field.

sunchao force-pushed the null-vector branch from 5f78a8c to 9de06ae Compare March 17, 2023 17:04

add period

bb84cd0

lidavidm approved these changes Mar 17, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Mar 17, 2023

viirya approved these changes Mar 17, 2023

View reviewed changes

amol- closed this Mar 30, 2023

jorisvandenbossche reopened this Mar 31, 2023

danepitkin reviewed May 12, 2023

View reviewed changes

danepitkin mentioned this pull request Nov 29, 2023

GH-34610: [Java] Fix valueCount and field name when loading/transferring NullVector #38973

Merged

danepitkin closed this Nov 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-34610: [Java] Fix valueCount when loading NullVector #34611

GH-34610: [Java] Fix valueCount when loading NullVector #34611

sunchao commented Mar 17, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Mar 17, 2023

github-actions bot commented Mar 17, 2023

viirya commented Mar 17, 2023

lidavidm commented Mar 17, 2023

lidavidm commented Mar 17, 2023

viirya commented Mar 17, 2023

sunchao commented Mar 17, 2023

amol- commented Mar 30, 2023

amol- commented Apr 13, 2023

danepitkin May 12, 2023

sunchao commented May 12, 2023

danepitkin commented May 12, 2023

danepitkin commented Nov 29, 2023

GH-34610: [Java] Fix valueCount when loading NullVector #34611

GH-34610: [Java] Fix valueCount when loading NullVector #34611

Conversation

sunchao commented Mar 17, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Mar 17, 2023

github-actions bot commented Mar 17, 2023

viirya commented Mar 17, 2023

lidavidm commented Mar 17, 2023

lidavidm commented Mar 17, 2023

viirya commented Mar 17, 2023

sunchao commented Mar 17, 2023

amol- commented Mar 30, 2023

amol- commented Apr 13, 2023

danepitkin May 12, 2023

Choose a reason for hiding this comment

sunchao commented May 12, 2023

danepitkin commented May 12, 2023

danepitkin commented Nov 29, 2023

sunchao commented Mar 17, 2023 •

edited by github-actions bot

Loading