-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-44626: [Java] fix SplitAndTransfer throws for empty MapVector #44627
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we just return if length=0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do though my personal preference would be to avoid multiple returns. Current implementation is in keeping with how splitAndTransfer is implemented for other complex types (e.g. ListVector).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use try-with-resources
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that there're several closeable resources here and toListVector cannot be constructed inside the same try as fromListVector. This is just to say that there'd be some ugly nesting if using try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Sure.
|
@lidavidm need some help to run the CIs. |
Signed-off-by: Maksim Yegorov <findmaksim@gmail.com>
| MapVector dataVector = (MapVector) fromListVector.getDataVector(); | ||
| dataVector.allocateNew(); | ||
| // unset capacity to mimic observed failure mode | ||
| dataVector.getOffsetBuffer().capacity(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, a list vector is supposed to have 1 offset when it's empty. But I suppose other parts of the library have this issue/"optimization"
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 5fe87a3. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
apache#44627) Empty MapVector.splitAndTransfer throws `java.lang.IndexOutOfBoundsException`. Details in apache#44626 Fixed for MapVector as for other vector types in apache#41066 Added unit test mimicking the scenario we've observed where MapVector's offset buffer capacity is 0. * GitHub Issue: apache#44626 Authored-by: Maksim Yegorov <59841139+maksimyego-db@users.noreply.github.com> Signed-off-by: David Li <li.davidm96@gmail.com>
apache#44627) (#93) Empty MapVector.splitAndTransfer throws `java.lang.IndexOutOfBoundsException`. Details in apache#44626 Fixed for MapVector as for other vector types in apache#41066 Added unit test mimicking the scenario we've observed where MapVector's offset buffer capacity is 0. * GitHub Issue: apache#44626 Authored-by: Maksim Yegorov <59841139+maksimyego-db@users.noreply.github.com> Signed-off-by: David Li <li.davidm96@gmail.com> Co-authored-by: Maksim Yegorov <997437+myegorov@users.noreply.github.com>
…tor (apache#44631) (#94) * apacheGH-44626: [Java] fix SplitAndTransfer throws for empty MapVector (apache#44627) Empty MapVector.splitAndTransfer throws `java.lang.IndexOutOfBoundsException`. Details in apache#44626 Fixed for MapVector as for other vector types in apache#41066 Added unit test mimicking the scenario we've observed where MapVector's offset buffer capacity is 0. * GitHub Issue: apache#44626 Authored-by: Maksim Yegorov <59841139+maksimyego-db@users.noreply.github.com> Signed-off-by: David Li <li.davidm96@gmail.com> * apacheGH-44344: [Java] fix VectorSchemaRoot.getTransferPair for NullVector (apache#44631) Do not throw [UnsupportedOperationException("Tried to get allocator from NullVector")](https://github.com/apache/arrow/blob/release-18.0.0-rc0/java/vector/src/main/java/org/apache/arrow/vector/NullVector.java#L160) from [VectorSchemaRoot.slice()](https://github.com/apache/arrow/blob/release-18.0.0-rc0/java/vector/src/main/java/org/apache/arrow/vector/VectorSchemaRoot.java#L341) when slicing a VSR containing a NullVector or ZeroVector. Details in apache#44344 Added unit test that would trigger an UnsupportedOperationException on the legacy path. * GitHub Issue: apache#44344 Authored-by: Maksim Yegorov <59841139+maksimyego-db@users.noreply.github.com> Signed-off-by: David Li <li.davidm96@gmail.com> --------- Signed-off-by: David Li <li.davidm96@gmail.com> Co-authored-by: Maksim Yegorov <997437+myegorov@users.noreply.github.com>
apache#44627) ### Rationale for this change Empty MapVector.splitAndTransfer throws `java.lang.IndexOutOfBoundsException`. Details in apache#44626 ### What changes are included in this PR? Fixed for MapVector as for other vector types in apache#41066 ### Are these changes tested? Added unit test mimicking the scenario we've observed where MapVector's offset buffer capacity is 0. * GitHub Issue: apache#44626 Authored-by: Maksim Yegorov <59841139+maksimyego-db@users.noreply.github.com> Signed-off-by: David Li <li.davidm96@gmail.com>
Rationale for this change
Empty MapVector.splitAndTransfer throws
java.lang.IndexOutOfBoundsException. Details in #44626What changes are included in this PR?
Fixed for MapVector as for other vector types in #41066
Are these changes tested?
Added unit test mimicking the scenario we've observed where MapVector's offset buffer capacity is 0.