You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "vibhatha (via GitHub)" <gi...@apache.org> on 2024/08/06 10:26:34 UTC
[PR] GH-43577: [Java] getBuffers method needs correction on clear flag usage [arrow]
vibhatha opened a new pull request, #43583:
URL: https://github.com/apache/arrow/pull/43583
### Rationale for this change
`getBuffers` method provides the capability to clear the buffers in the vector, this has not been properly tested while clear flag is not properly used in the implementation across various types of vectors.
### What changes are included in this PR?
Updating the vector `getBuffers` method to use `clear` flag as expected and adding corresponding test cases.
### Are these changes tested?
Yes, via existing test cases and new test cases.
### Are there any user-facing changes?
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43577: [Java] getBuffers method needs correction on clear flag usage [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #43583:
URL: https://github.com/apache/arrow/pull/43583#issuecomment-2277055185
Also, are we certain the original behavior wasn't intentional? How is clear currently used?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43577: [Java] getBuffers method needs correction on clear flag usage [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #43583:
URL: https://github.com/apache/arrow/pull/43583#issuecomment-2287467447
@lidavidm I updated the PR with the doc change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43577: [Java] getBuffers method needs correction on clear flag usage [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #43583:
URL: https://github.com/apache/arrow/pull/43583#discussion_r1716132147
##########
java/vector/src/main/java/org/apache/arrow/vector/complex/AbstractStructVector.java:
##########
@@ -382,12 +382,23 @@ public VectorWithOrdinal getChildVectorWithOrdinal(String name) {
return new VectorWithOrdinal(vector, ordinal);
}
+ /**
+ * Return the underlying buffers associated with this vector. Note that this doesn't impact the
+ * reference counts for this buffer, so it only should be used for in-context access. Also note
+ * that this buffer changes regularly, thus external classes shouldn't hold a reference to it
+ * (unless they change it).
+ *
+ * @param clear Whether to clear vector before returning, the buffers will still be refcounted but
+ * the returned array will be the only reference to them. Also, this won't clear the child
+ * buffers.
+ * @return The underlying {@link ArrowBuf buffers} that is used by this vector instance.
+ */
@Override
public ArrowBuf[] getBuffers(boolean clear) {
final List<ArrowBuf> buffers = new ArrayList<>();
for (final ValueVector vector : vectors.values()) {
- for (final ArrowBuf buf : vector.getBuffers(false)) {
+ for (final ArrowBuf buf : vector.getBuffers(clear)) {
Review Comment:
Haven't committed sorry.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43577: [Java] getBuffers method needs correction on clear flag usage [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #43583:
URL: https://github.com/apache/arrow/pull/43583#discussion_r1716123188
##########
java/vector/src/main/java/org/apache/arrow/vector/complex/AbstractStructVector.java:
##########
@@ -382,12 +382,23 @@ public VectorWithOrdinal getChildVectorWithOrdinal(String name) {
return new VectorWithOrdinal(vector, ordinal);
}
+ /**
+ * Return the underlying buffers associated with this vector. Note that this doesn't impact the
+ * reference counts for this buffer, so it only should be used for in-context access. Also note
+ * that this buffer changes regularly, thus external classes shouldn't hold a reference to it
+ * (unless they change it).
+ *
+ * @param clear Whether to clear vector before returning, the buffers will still be refcounted but
+ * the returned array will be the only reference to them. Also, this won't clear the child
+ * buffers.
+ * @return The underlying {@link ArrowBuf buffers} that is used by this vector instance.
+ */
@Override
public ArrowBuf[] getBuffers(boolean clear) {
final List<ArrowBuf> buffers = new ArrayList<>();
for (final ValueVector vector : vectors.values()) {
- for (final ArrowBuf buf : vector.getBuffers(false)) {
+ for (final ArrowBuf buf : vector.getBuffers(clear)) {
Review Comment:
We _don't_ want to do this now, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43577: [Java] getBuffers method needs correction on clear flag usage [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #43583:
URL: https://github.com/apache/arrow/pull/43583#issuecomment-2277061836
If we allow this, we need to document and remind the user that when clear is true, there is a responsibility in clearing the retrieved buffers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43577: [Java] getBuffers method needs correction on clear flag usage [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #43583:
URL: https://github.com/apache/arrow/pull/43583#discussion_r1715211518
##########
java/vector/src/test/java/org/apache/arrow/vector/TestVectorReset.java:
##########
@@ -136,6 +140,13 @@ public void testListTypeReset() {
fixedList.setNull(0);
fixedList.setValueCount(1);
resetVectorAndVerify(fixedList, fixedList.getBuffers(false));
+
+ // ListViewVector
Review Comment:
@lidavidm I didn't add the `LargeList` and `LargeListView` as `LargeList` wasn't there in the first place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43577: [Java] getBuffers method needs correction on clear flag usage [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #43583:
URL: https://github.com/apache/arrow/pull/43583#issuecomment-2277060770
> Also, are we certain the original behavior wasn't intentional? How is clear currently used?
This came up in the `LargeListVIewVector` review. Up to that point, I also didn't think about it that much, I was actually using the existing logic from other vectors. Clear is just used to determine whether or not to clear the vector but not the `FieldVector` assoicated with that vector.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43577: [Java] getBuffers method needs correction on clear flag usage [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #43583:
URL: https://github.com/apache/arrow/pull/43583#issuecomment-2285994529
Sounds good to me. Thanks for looking!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43577: [Java] getBuffers method needs correction on clear flag usage [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #43583:
URL: https://github.com/apache/arrow/pull/43583
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43577: [Java] getBuffers method needs correction on clear flag usage [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #43583:
URL: https://github.com/apache/arrow/pull/43583#issuecomment-2289102544
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 4d200dc17daf268863df6f0d7c458cb460904a7c.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/28770794736) has more details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43577: [Java] getBuffers method needs correction on clear flag usage [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #43583:
URL: https://github.com/apache/arrow/pull/43583#issuecomment-2277068081
> > Also, are we certain the original behavior wasn't intentional? How is clear currently used?
>
> This came up in the `LargeListVIewVector` review. Up to that point, I also didn't think about it that much, I was actually using the existing logic from other vectors. Clear is just used to determine whether or not to clear the vector but not the `FieldVector` assoicated with that vector.
Right, I pointed it out since it didn't make immediate sense. But I'm asking if the codebase gives a reason why clear works this way. It might not be obvious.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43577: [Java] getBuffers method needs correction on clear flag usage [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #43583:
URL: https://github.com/apache/arrow/pull/43583#issuecomment-2285978929
Looking into this, I think, clearing the buffers of the child vector might release memory that is still in use elsewhere. And this might lead to unexpected behavior or errors as we are explicitly enforcing a buffer clear on the extracted ones. Also, I think the clear was only meant to be used to clear the vector's buffer not its child buffers. But I feel like this is not precisely documented, as it only says "Return the underlying buffers associated with this vector".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43577: [Java] getBuffers method needs correction on clear flag usage [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #43583:
URL: https://github.com/apache/arrow/pull/43583#discussion_r1716132022
##########
java/vector/src/main/java/org/apache/arrow/vector/complex/AbstractStructVector.java:
##########
@@ -382,12 +382,23 @@ public VectorWithOrdinal getChildVectorWithOrdinal(String name) {
return new VectorWithOrdinal(vector, ordinal);
}
+ /**
+ * Return the underlying buffers associated with this vector. Note that this doesn't impact the
+ * reference counts for this buffer, so it only should be used for in-context access. Also note
+ * that this buffer changes regularly, thus external classes shouldn't hold a reference to it
+ * (unless they change it).
+ *
+ * @param clear Whether to clear vector before returning, the buffers will still be refcounted but
+ * the returned array will be the only reference to them. Also, this won't clear the child
+ * buffers.
+ * @return The underlying {@link ArrowBuf buffers} that is used by this vector instance.
+ */
@Override
public ArrowBuf[] getBuffers(boolean clear) {
final List<ArrowBuf> buffers = new ArrayList<>();
for (final ValueVector vector : vectors.values()) {
- for (final ArrowBuf buf : vector.getBuffers(false)) {
+ for (final ArrowBuf buf : vector.getBuffers(clear)) {
Review Comment:
yeah I have missed this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43577: [Java] getBuffers method needs correction on clear flag usage [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #43583:
URL: https://github.com/apache/arrow/pull/43583#issuecomment-2277056311
> I think this should be considered a breaking change
Yes it is. Sorry I didn't make it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43577: [Java] getBuffers method needs correction on clear flag usage [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #43583:
URL: https://github.com/apache/arrow/pull/43583#issuecomment-2277054436
I think this should be considered a breaking change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43577: [Java] getBuffers method needs correction on clear flag usage [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #43583:
URL: https://github.com/apache/arrow/pull/43583#issuecomment-2277079264
Let me investigate this a little more.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43577: [Java] getBuffers method needs correction on clear flag usage [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #43583:
URL: https://github.com/apache/arrow/pull/43583#issuecomment-2285980642
Reconsidering my position, I would like to propose a documentation update and reverting the change made for `LargeListViewVector` in addressing reviews in the previous PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org