GH-34610: [Java] Fix valueCount when loading NullVector #34611
Conversation
|
|
github-actions
bot
added
Component: Java
awaiting committer review
We should set `valueCount` when loading `NullVector`. Unfortunately there's no `nullCount` field.
github-actions
bot
added
awaiting merge
Hmm, is this related? |
|
No, there is a long standing issue for it that nobody has had time to look at. |
|
The JNI pipeline failure does seem related, though |
|
yea, that's the added test |
|
Hmm thanks, let me take a look. |
|
Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍 |
|
@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. |
| @Test | ||
| public void testNullVector() { | ||
| try (final NullVector vector = new NullVector("v", 1024)) { | ||
| assertTrue(roundtrip(vector, NullVector.class)); |
There was a problem hiding this comment.
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>);
|
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. |
|
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: 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 <sunchao@apple.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
…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 <sunchao@apple.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
We should set
valueCountandnullCountwhen loadingNullVector.Rationale for this change
When importing a
NullVector, for example, via Arrow C interface, thevalueCountandnullCountis currently not set properly, which will result both values to be 0.What changes are included in this PR?
Set
valueCountwhen 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.