ARROW-1283: [JAVA] Allow VectorSchemaRoot to close more than once by BryanCutler · Pull Request #898 · apache/arrow

BryanCutler · 2017-07-27T01:50:38Z

This change allows the VectorSchemaRoot/FieldVectors to close more than once, even if the allocator has already been closed. Before, an empty ArrowBuf was created during closing which required the allocator to not be closed, however this empty buffer is not needed once the FieldVector has been closed.

…ated again after closing

…Root-close-twice-ARROW-1283

BryanCutler · 2017-07-27T01:51:06Z

cc @julienledem @elahrvivaz

elahrvivaz

looks good to me

wesm · 2017-07-28T02:42:09Z

@jacques-n @StevenMPhillips @siddharthteotia do you see any issues with this?

siddharthteotia · 2017-07-28T23:47:11Z

@BryanCutler

I would like to know why is there a need to close a vector twice. The current change is structured such that following works and the last statement doesn't raise an error.

vector.close()
allocator.close()
vector.close()

Once close() operation is invoked on the vector and allocator, I expect the resources associated to be garbage collected properly. Under what conditions, do we still the 3rd statement?

Thanks,
Siddharth

jacques-n · 2017-07-28T23:51:18Z

- data = null;
- }
- super.close();
+ data = null;


I would prefer doing this change differently. Maybe by allowing allocator to return an empty buffer even if closed. This is because it makes bugs/issues much easier to understand than getting an NPE.

Sure, that sounds fine. As far as I can this would not cause any issues.

BryanCutler · 2017-07-31T17:48:33Z

I would like to know why is there a need to close a vector twice.

@siddharthteotia , this came from a discussion in Spark here. The root/allocator are being used in an iterator and are closed normally after the last iteration, however there are some ways this wouldn't happen (say if a task was cancelled), so they are also closed in a callback to prevent any leaks. Right now an extra flag is being used, which is fine but it would be cleaner to just allow them to call close a second time and not do anything.

…s and assigns an empty buffer

BryanCutler · 2017-08-01T00:17:53Z

 }

 private ArrowBuf createEmpty() {
- assertOpen();


this did not seem necessary either, because an allocator will always hold an instance of an empty buffer. Does this seem ok?

BryanCutler · 2017-08-01T00:41:25Z

@jacques-n , I made the change to use an empty buffer instead of assigning null. This essentially made close() the same as clear() and could be removed. Now in the super class BaseValueVector.close() will invoke clear() to release any buffer and assign data to an empty buffer.

It also seems to me that all Nullable vector would now have a close() method that is identical to clear(). Is that maybe something to remove and cleanup here also? Should there be any different meaning to closing vs. clearing a vector?

…Root-close-twice-ARROW-1283

BryanCutler · 2017-08-03T18:24:42Z

 }

- // TODO: Nullable vectors extend BaseDataValueVector but do not use the data field
- // We should fix the inheritance tree


@siddharthteotia I believe you resolved this comment in #892. Does this PR look ok to you?

wesm · 2017-08-04T18:25:14Z

The CI failure is unrelated to these changes. Can this be merged?

wesm

+1

BryanCutler · 2017-08-07T21:57:19Z

Thanks @wesm!

This change allows the VectorSchemaRoot/FieldVectors to close more than once, even if the allocator has already been closed. Before, an empty ArrowBuf was created during closing which required the allocator to not be closed, however this empty buffer is not needed once the FieldVector has been closed. Author: Bryan Cutler <cutlerb@gmail.com> Closesapache#898 from BryanCutler/java-vectorSchemaRoot-close-twice-ARROW-1283 and squashes the following commits: 2921d84 [Bryan Cutler] removed resolved comment 3b3718b [Bryan Cutler] Merge remote-tracking branch 'upstream/master' into java-vectorSchemaRoot-close-twice-ARROW-1283 e992fc7 [Bryan Cutler] BaseDataValueVector.close will now just clear, which releases previous and assigns an empty buffer 8ecfce2 [Bryan Cutler] Merge remote-tracking branch 'upstream/master' into java-vectorSchemaRoot-close-twice-ARROW-1283 ca38d3d [Bryan Cutler] use clear to release data, ensure that an empty buffer is never allocated again after closing 10ff7c3 [Bryan Cutler] Added regression test

BryanCutler · 2017-07-27T01:50:38Z

This change allows the VectorSchemaRoot/FieldVectors to close more than once, even if the allocator has already been closed. Before, an empty ArrowBuf was created during closing which required the allocator to not be closed, however this empty buffer is not needed once the FieldVector has been closed.

…ated again after closing

…Root-close-twice-ARROW-1283

BryanCutler · 2017-07-27T01:51:06Z

cc @julienledem @elahrvivaz

elahrvivaz

looks good to me

wesm · 2017-07-28T02:42:09Z

@jacques-n @StevenMPhillips @siddharthteotia do you see any issues with this?

siddharthteotia · 2017-07-28T23:47:11Z

@BryanCutler

I would like to know why is there a need to close a vector twice. The current change is structured such that following works and the last statement doesn't raise an error.

vector.close()
allocator.close()
vector.close()

Once close() operation is invoked on the vector and allocator, I expect the resources associated to be garbage collected properly. Under what conditions, do we still the 3rd statement?

Thanks,
Siddharth

jacques-n · 2017-07-28T23:51:18Z

- data = null;
- }
- super.close();
+ data = null;


I would prefer doing this change differently. Maybe by allowing allocator to return an empty buffer even if closed. This is because it makes bugs/issues much easier to understand than getting an NPE.

Sure, that sounds fine. As far as I can this would not cause any issues.

BryanCutler · 2017-07-31T17:48:33Z

I would like to know why is there a need to close a vector twice.

@siddharthteotia , this came from a discussion in Spark here. The root/allocator are being used in an iterator and are closed normally after the last iteration, however there are some ways this wouldn't happen (say if a task was cancelled), so they are also closed in a callback to prevent any leaks. Right now an extra flag is being used, which is fine but it would be cleaner to just allow them to call close a second time and not do anything.

…s and assigns an empty buffer

BryanCutler · 2017-08-01T00:17:53Z

 }

 private ArrowBuf createEmpty() {
- assertOpen();


this did not seem necessary either, because an allocator will always hold an instance of an empty buffer. Does this seem ok?

BryanCutler · 2017-08-01T00:41:25Z

@jacques-n , I made the change to use an empty buffer instead of assigning null. This essentially made close() the same as clear() and could be removed. Now in the super class BaseValueVector.close() will invoke clear() to release any buffer and assign data to an empty buffer.

It also seems to me that all Nullable vector would now have a close() method that is identical to clear(). Is that maybe something to remove and cleanup here also? Should there be any different meaning to closing vs. clearing a vector?

…Root-close-twice-ARROW-1283

BryanCutler · 2017-08-03T18:24:42Z

 }

- // TODO: Nullable vectors extend BaseDataValueVector but do not use the data field
- // We should fix the inheritance tree


@siddharthteotia I believe you resolved this comment in #892. Does this PR look ok to you?

wesm · 2017-08-04T18:25:14Z

The CI failure is unrelated to these changes. Can this be merged?

wesm

+1

BryanCutler · 2017-08-07T21:57:19Z

Thanks @wesm!

This change allows the VectorSchemaRoot/FieldVectors to close more than once, even if the allocator has already been closed. Before, an empty ArrowBuf was created during closing which required the allocator to not be closed, however this empty buffer is not needed once the FieldVector has been closed. Author: Bryan Cutler <cutlerb@gmail.com> Closesapache#898 from BryanCutler/java-vectorSchemaRoot-close-twice-ARROW-1283 and squashes the following commits: 2921d84 [Bryan Cutler] removed resolved comment 3b3718b [Bryan Cutler] Merge remote-tracking branch 'upstream/master' into java-vectorSchemaRoot-close-twice-ARROW-1283 e992fc7 [Bryan Cutler] BaseDataValueVector.close will now just clear, which releases previous and assigns an empty buffer 8ecfce2 [Bryan Cutler] Merge remote-tracking branch 'upstream/master' into java-vectorSchemaRoot-close-twice-ARROW-1283 ca38d3d [Bryan Cutler] use clear to release data, ensure that an empty buffer is never allocated again after closing 10ff7c3 [Bryan Cutler] Added regression test

BryanCutler · 2017-07-27T01:50:38Z

This change allows the VectorSchemaRoot/FieldVectors to close more than once, even if the allocator has already been closed. Before, an empty ArrowBuf was created during closing which required the allocator to not be closed, however this empty buffer is not needed once the FieldVector has been closed.

…ated again after closing

…Root-close-twice-ARROW-1283

BryanCutler · 2017-07-27T01:51:06Z

cc @julienledem @elahrvivaz

elahrvivaz

looks good to me

wesm · 2017-07-28T02:42:09Z

@jacques-n @StevenMPhillips @siddharthteotia do you see any issues with this?

siddharthteotia · 2017-07-28T23:47:11Z

@BryanCutler

I would like to know why is there a need to close a vector twice. The current change is structured such that following works and the last statement doesn't raise an error.

vector.close()
allocator.close()
vector.close()

Once close() operation is invoked on the vector and allocator, I expect the resources associated to be garbage collected properly. Under what conditions, do we still the 3rd statement?

Thanks,
Siddharth

jacques-n · 2017-07-28T23:51:18Z

- data = null;
- }
- super.close();
+ data = null;


I would prefer doing this change differently. Maybe by allowing allocator to return an empty buffer even if closed. This is because it makes bugs/issues much easier to understand than getting an NPE.

Sure, that sounds fine. As far as I can this would not cause any issues.

BryanCutler · 2017-07-31T17:48:33Z

I would like to know why is there a need to close a vector twice.

@siddharthteotia , this came from a discussion in Spark here. The root/allocator are being used in an iterator and are closed normally after the last iteration, however there are some ways this wouldn't happen (say if a task was cancelled), so they are also closed in a callback to prevent any leaks. Right now an extra flag is being used, which is fine but it would be cleaner to just allow them to call close a second time and not do anything.

…s and assigns an empty buffer

BryanCutler · 2017-08-01T00:17:53Z

 }

 private ArrowBuf createEmpty() {
- assertOpen();


this did not seem necessary either, because an allocator will always hold an instance of an empty buffer. Does this seem ok?

BryanCutler · 2017-08-01T00:41:25Z

@jacques-n , I made the change to use an empty buffer instead of assigning null. This essentially made close() the same as clear() and could be removed. Now in the super class BaseValueVector.close() will invoke clear() to release any buffer and assign data to an empty buffer.

It also seems to me that all Nullable vector would now have a close() method that is identical to clear(). Is that maybe something to remove and cleanup here also? Should there be any different meaning to closing vs. clearing a vector?

…Root-close-twice-ARROW-1283

BryanCutler · 2017-08-03T18:24:42Z

 }

- // TODO: Nullable vectors extend BaseDataValueVector but do not use the data field
- // We should fix the inheritance tree


@siddharthteotia I believe you resolved this comment in #892. Does this PR look ok to you?

wesm · 2017-08-04T18:25:14Z

The CI failure is unrelated to these changes. Can this be merged?

wesm

+1

BryanCutler · 2017-08-07T21:57:19Z

Thanks @wesm!

This change allows the VectorSchemaRoot/FieldVectors to close more than once, even if the allocator has already been closed. Before, an empty ArrowBuf was created during closing which required the allocator to not be closed, however this empty buffer is not needed once the FieldVector has been closed. Author: Bryan Cutler <cutlerb@gmail.com> Closesapache#898 from BryanCutler/java-vectorSchemaRoot-close-twice-ARROW-1283 and squashes the following commits: 2921d84 [Bryan Cutler] removed resolved comment 3b3718b [Bryan Cutler] Merge remote-tracking branch 'upstream/master' into java-vectorSchemaRoot-close-twice-ARROW-1283 e992fc7 [Bryan Cutler] BaseDataValueVector.close will now just clear, which releases previous and assigns an empty buffer 8ecfce2 [Bryan Cutler] Merge remote-tracking branch 'upstream/master' into java-vectorSchemaRoot-close-twice-ARROW-1283 ca38d3d [Bryan Cutler] use clear to release data, ensure that an empty buffer is never allocated again after closing 10ff7c3 [Bryan Cutler] Added regression test

BryanCutler · 2017-07-27T01:50:38Z

This change allows the VectorSchemaRoot/FieldVectors to close more than once, even if the allocator has already been closed. Before, an empty ArrowBuf was created during closing which required the allocator to not be closed, however this empty buffer is not needed once the FieldVector has been closed.

…ated again after closing

…Root-close-twice-ARROW-1283

BryanCutler · 2017-07-27T01:51:06Z

cc @julienledem @elahrvivaz

elahrvivaz

looks good to me

wesm · 2017-07-28T02:42:09Z

@jacques-n @StevenMPhillips @siddharthteotia do you see any issues with this?

siddharthteotia · 2017-07-28T23:47:11Z

@BryanCutler

I would like to know why is there a need to close a vector twice. The current change is structured such that following works and the last statement doesn't raise an error.

vector.close()
allocator.close()
vector.close()

Once close() operation is invoked on the vector and allocator, I expect the resources associated to be garbage collected properly. Under what conditions, do we still the 3rd statement?

Thanks,
Siddharth

jacques-n · 2017-07-28T23:51:18Z

- data = null;
- }
- super.close();
+ data = null;


I would prefer doing this change differently. Maybe by allowing allocator to return an empty buffer even if closed. This is because it makes bugs/issues much easier to understand than getting an NPE.

Sure, that sounds fine. As far as I can this would not cause any issues.

BryanCutler · 2017-07-31T17:48:33Z

I would like to know why is there a need to close a vector twice.

@siddharthteotia , this came from a discussion in Spark here. The root/allocator are being used in an iterator and are closed normally after the last iteration, however there are some ways this wouldn't happen (say if a task was cancelled), so they are also closed in a callback to prevent any leaks. Right now an extra flag is being used, which is fine but it would be cleaner to just allow them to call close a second time and not do anything.

…s and assigns an empty buffer

BryanCutler · 2017-08-01T00:17:53Z

 }

 private ArrowBuf createEmpty() {
- assertOpen();


this did not seem necessary either, because an allocator will always hold an instance of an empty buffer. Does this seem ok?

BryanCutler · 2017-08-01T00:41:25Z

@jacques-n , I made the change to use an empty buffer instead of assigning null. This essentially made close() the same as clear() and could be removed. Now in the super class BaseValueVector.close() will invoke clear() to release any buffer and assign data to an empty buffer.

It also seems to me that all Nullable vector would now have a close() method that is identical to clear(). Is that maybe something to remove and cleanup here also? Should there be any different meaning to closing vs. clearing a vector?

…Root-close-twice-ARROW-1283

BryanCutler · 2017-08-03T18:24:42Z

 }

- // TODO: Nullable vectors extend BaseDataValueVector but do not use the data field
- // We should fix the inheritance tree


@siddharthteotia I believe you resolved this comment in #892. Does this PR look ok to you?

wesm · 2017-08-04T18:25:14Z

The CI failure is unrelated to these changes. Can this be merged?

wesm

+1

BryanCutler · 2017-08-07T21:57:19Z

Thanks @wesm!

This change allows the VectorSchemaRoot/FieldVectors to close more than once, even if the allocator has already been closed. Before, an empty ArrowBuf was created during closing which required the allocator to not be closed, however this empty buffer is not needed once the FieldVector has been closed. Author: Bryan Cutler <cutlerb@gmail.com> Closesapache#898 from BryanCutler/java-vectorSchemaRoot-close-twice-ARROW-1283 and squashes the following commits: 2921d84 [Bryan Cutler] removed resolved comment 3b3718b [Bryan Cutler] Merge remote-tracking branch 'upstream/master' into java-vectorSchemaRoot-close-twice-ARROW-1283 e992fc7 [Bryan Cutler] BaseDataValueVector.close will now just clear, which releases previous and assigns an empty buffer 8ecfce2 [Bryan Cutler] Merge remote-tracking branch 'upstream/master' into java-vectorSchemaRoot-close-twice-ARROW-1283 ca38d3d [Bryan Cutler] use clear to release data, ensure that an empty buffer is never allocated again after closing 10ff7c3 [Bryan Cutler] Added regression test

BryanCutler · 2017-07-27T01:50:38Z

This change allows the VectorSchemaRoot/FieldVectors to close more than once, even if the allocator has already been closed. Before, an empty ArrowBuf was created during closing which required the allocator to not be closed, however this empty buffer is not needed once the FieldVector has been closed.

…ated again after closing

…Root-close-twice-ARROW-1283

BryanCutler · 2017-07-27T01:51:06Z

cc @julienledem @elahrvivaz

elahrvivaz

looks good to me

wesm · 2017-07-28T02:42:09Z

@jacques-n @StevenMPhillips @siddharthteotia do you see any issues with this?

siddharthteotia · 2017-07-28T23:47:11Z

@BryanCutler

I would like to know why is there a need to close a vector twice. The current change is structured such that following works and the last statement doesn't raise an error.

vector.close()
allocator.close()
vector.close()

Once close() operation is invoked on the vector and allocator, I expect the resources associated to be garbage collected properly. Under what conditions, do we still the 3rd statement?

Thanks,
Siddharth

jacques-n · 2017-07-28T23:51:18Z

- data = null;
- }
- super.close();
+ data = null;


I would prefer doing this change differently. Maybe by allowing allocator to return an empty buffer even if closed. This is because it makes bugs/issues much easier to understand than getting an NPE.

Sure, that sounds fine. As far as I can this would not cause any issues.

BryanCutler · 2017-07-31T17:48:33Z

I would like to know why is there a need to close a vector twice.

@siddharthteotia , this came from a discussion in Spark here. The root/allocator are being used in an iterator and are closed normally after the last iteration, however there are some ways this wouldn't happen (say if a task was cancelled), so they are also closed in a callback to prevent any leaks. Right now an extra flag is being used, which is fine but it would be cleaner to just allow them to call close a second time and not do anything.

…s and assigns an empty buffer

BryanCutler · 2017-08-01T00:17:53Z

 }

 private ArrowBuf createEmpty() {
- assertOpen();


this did not seem necessary either, because an allocator will always hold an instance of an empty buffer. Does this seem ok?

BryanCutler · 2017-08-01T00:41:25Z

@jacques-n , I made the change to use an empty buffer instead of assigning null. This essentially made close() the same as clear() and could be removed. Now in the super class BaseValueVector.close() will invoke clear() to release any buffer and assign data to an empty buffer.

It also seems to me that all Nullable vector would now have a close() method that is identical to clear(). Is that maybe something to remove and cleanup here also? Should there be any different meaning to closing vs. clearing a vector?

…Root-close-twice-ARROW-1283

BryanCutler · 2017-08-03T18:24:42Z

 }

- // TODO: Nullable vectors extend BaseDataValueVector but do not use the data field
- // We should fix the inheritance tree


@siddharthteotia I believe you resolved this comment in #892. Does this PR look ok to you?

wesm · 2017-08-04T18:25:14Z

The CI failure is unrelated to these changes. Can this be merged?

wesm

+1

BryanCutler · 2017-08-07T21:57:19Z

Thanks @wesm!

This change allows the VectorSchemaRoot/FieldVectors to close more than once, even if the allocator has already been closed. Before, an empty ArrowBuf was created during closing which required the allocator to not be closed, however this empty buffer is not needed once the FieldVector has been closed. Author: Bryan Cutler <cutlerb@gmail.com> Closesapache#898 from BryanCutler/java-vectorSchemaRoot-close-twice-ARROW-1283 and squashes the following commits: 2921d84 [Bryan Cutler] removed resolved comment 3b3718b [Bryan Cutler] Merge remote-tracking branch 'upstream/master' into java-vectorSchemaRoot-close-twice-ARROW-1283 e992fc7 [Bryan Cutler] BaseDataValueVector.close will now just clear, which releases previous and assigns an empty buffer 8ecfce2 [Bryan Cutler] Merge remote-tracking branch 'upstream/master' into java-vectorSchemaRoot-close-twice-ARROW-1283 ca38d3d [Bryan Cutler] use clear to release data, ensure that an empty buffer is never allocated again after closing 10ff7c3 [Bryan Cutler] Added regression test

BryanCutler · 2017-07-27T01:50:38Z

This change allows the VectorSchemaRoot/FieldVectors to close more than once, even if the allocator has already been closed. Before, an empty ArrowBuf was created during closing which required the allocator to not be closed, however this empty buffer is not needed once the FieldVector has been closed.

…ated again after closing

…Root-close-twice-ARROW-1283

BryanCutler · 2017-07-27T01:51:06Z

cc @julienledem @elahrvivaz

elahrvivaz

looks good to me

wesm · 2017-07-28T02:42:09Z

@jacques-n @StevenMPhillips @siddharthteotia do you see any issues with this?

siddharthteotia · 2017-07-28T23:47:11Z

@BryanCutler

I would like to know why is there a need to close a vector twice. The current change is structured such that following works and the last statement doesn't raise an error.

vector.close()
allocator.close()
vector.close()

Once close() operation is invoked on the vector and allocator, I expect the resources associated to be garbage collected properly. Under what conditions, do we still the 3rd statement?

Thanks,
Siddharth

jacques-n · 2017-07-28T23:51:18Z

- data = null;
- }
- super.close();
+ data = null;


I would prefer doing this change differently. Maybe by allowing allocator to return an empty buffer even if closed. This is because it makes bugs/issues much easier to understand than getting an NPE.

Sure, that sounds fine. As far as I can this would not cause any issues.

BryanCutler · 2017-07-31T17:48:33Z

I would like to know why is there a need to close a vector twice.

@siddharthteotia , this came from a discussion in Spark here. The root/allocator are being used in an iterator and are closed normally after the last iteration, however there are some ways this wouldn't happen (say if a task was cancelled), so they are also closed in a callback to prevent any leaks. Right now an extra flag is being used, which is fine but it would be cleaner to just allow them to call close a second time and not do anything.

…s and assigns an empty buffer

BryanCutler · 2017-08-01T00:17:53Z

 }

 private ArrowBuf createEmpty() {
- assertOpen();


this did not seem necessary either, because an allocator will always hold an instance of an empty buffer. Does this seem ok?

BryanCutler · 2017-08-01T00:41:25Z

@jacques-n , I made the change to use an empty buffer instead of assigning null. This essentially made close() the same as clear() and could be removed. Now in the super class BaseValueVector.close() will invoke clear() to release any buffer and assign data to an empty buffer.

It also seems to me that all Nullable vector would now have a close() method that is identical to clear(). Is that maybe something to remove and cleanup here also? Should there be any different meaning to closing vs. clearing a vector?

…Root-close-twice-ARROW-1283

BryanCutler · 2017-08-03T18:24:42Z

 }

- // TODO: Nullable vectors extend BaseDataValueVector but do not use the data field
- // We should fix the inheritance tree


@siddharthteotia I believe you resolved this comment in #892. Does this PR look ok to you?

wesm · 2017-08-04T18:25:14Z

The CI failure is unrelated to these changes. Can this be merged?

wesm

+1

BryanCutler · 2017-08-07T21:57:19Z

Thanks @wesm!

This change allows the VectorSchemaRoot/FieldVectors to close more than once, even if the allocator has already been closed. Before, an empty ArrowBuf was created during closing which required the allocator to not be closed, however this empty buffer is not needed once the FieldVector has been closed. Author: Bryan Cutler <cutlerb@gmail.com> Closesapache#898 from BryanCutler/java-vectorSchemaRoot-close-twice-ARROW-1283 and squashes the following commits: 2921d84 [Bryan Cutler] removed resolved comment 3b3718b [Bryan Cutler] Merge remote-tracking branch 'upstream/master' into java-vectorSchemaRoot-close-twice-ARROW-1283 e992fc7 [Bryan Cutler] BaseDataValueVector.close will now just clear, which releases previous and assigns an empty buffer 8ecfce2 [Bryan Cutler] Merge remote-tracking branch 'upstream/master' into java-vectorSchemaRoot-close-twice-ARROW-1283 ca38d3d [Bryan Cutler] use clear to release data, ensure that an empty buffer is never allocated again after closing 10ff7c3 [Bryan Cutler] Added regression test

BryanCutler · 2017-07-27T01:50:38Z

This change allows the VectorSchemaRoot/FieldVectors to close more than once, even if the allocator has already been closed. Before, an empty ArrowBuf was created during closing which required the allocator to not be closed, however this empty buffer is not needed once the FieldVector has been closed.

…ated again after closing

…Root-close-twice-ARROW-1283

BryanCutler · 2017-07-27T01:51:06Z

cc @julienledem @elahrvivaz

elahrvivaz

looks good to me

wesm · 2017-07-28T02:42:09Z

@jacques-n @StevenMPhillips @siddharthteotia do you see any issues with this?

siddharthteotia · 2017-07-28T23:47:11Z

@BryanCutler

I would like to know why is there a need to close a vector twice. The current change is structured such that following works and the last statement doesn't raise an error.

vector.close()
allocator.close()
vector.close()

Once close() operation is invoked on the vector and allocator, I expect the resources associated to be garbage collected properly. Under what conditions, do we still the 3rd statement?

Thanks,
Siddharth

jacques-n · 2017-07-28T23:51:18Z

- data = null;
- }
- super.close();
+ data = null;


I would prefer doing this change differently. Maybe by allowing allocator to return an empty buffer even if closed. This is because it makes bugs/issues much easier to understand than getting an NPE.

Sure, that sounds fine. As far as I can this would not cause any issues.

BryanCutler · 2017-07-31T17:48:33Z

I would like to know why is there a need to close a vector twice.

@siddharthteotia , this came from a discussion in Spark here. The root/allocator are being used in an iterator and are closed normally after the last iteration, however there are some ways this wouldn't happen (say if a task was cancelled), so they are also closed in a callback to prevent any leaks. Right now an extra flag is being used, which is fine but it would be cleaner to just allow them to call close a second time and not do anything.

…s and assigns an empty buffer

BryanCutler · 2017-08-01T00:17:53Z

 }

 private ArrowBuf createEmpty() {
- assertOpen();


this did not seem necessary either, because an allocator will always hold an instance of an empty buffer. Does this seem ok?

BryanCutler · 2017-08-01T00:41:25Z

@jacques-n , I made the change to use an empty buffer instead of assigning null. This essentially made close() the same as clear() and could be removed. Now in the super class BaseValueVector.close() will invoke clear() to release any buffer and assign data to an empty buffer.

It also seems to me that all Nullable vector would now have a close() method that is identical to clear(). Is that maybe something to remove and cleanup here also? Should there be any different meaning to closing vs. clearing a vector?

…Root-close-twice-ARROW-1283

BryanCutler · 2017-08-03T18:24:42Z

 }

- // TODO: Nullable vectors extend BaseDataValueVector but do not use the data field
- // We should fix the inheritance tree


@siddharthteotia I believe you resolved this comment in #892. Does this PR look ok to you?

wesm · 2017-08-04T18:25:14Z

The CI failure is unrelated to these changes. Can this be merged?

wesm

+1

BryanCutler · 2017-08-07T21:57:19Z

Thanks @wesm!

This change allows the VectorSchemaRoot/FieldVectors to close more than once, even if the allocator has already been closed. Before, an empty ArrowBuf was created during closing which required the allocator to not be closed, however this empty buffer is not needed once the FieldVector has been closed. Author: Bryan Cutler <cutlerb@gmail.com> Closesapache#898 from BryanCutler/java-vectorSchemaRoot-close-twice-ARROW-1283 and squashes the following commits: 2921d84 [Bryan Cutler] removed resolved comment 3b3718b [Bryan Cutler] Merge remote-tracking branch 'upstream/master' into java-vectorSchemaRoot-close-twice-ARROW-1283 e992fc7 [Bryan Cutler] BaseDataValueVector.close will now just clear, which releases previous and assigns an empty buffer 8ecfce2 [Bryan Cutler] Merge remote-tracking branch 'upstream/master' into java-vectorSchemaRoot-close-twice-ARROW-1283 ca38d3d [Bryan Cutler] use clear to release data, ensure that an empty buffer is never allocated again after closing 10ff7c3 [Bryan Cutler] Added regression test

BryanCutler · 2017-07-27T01:50:38Z

This change allows the VectorSchemaRoot/FieldVectors to close more than once, even if the allocator has already been closed. Before, an empty ArrowBuf was created during closing which required the allocator to not be closed, however this empty buffer is not needed once the FieldVector has been closed.

…ated again after closing

…Root-close-twice-ARROW-1283

BryanCutler · 2017-07-27T01:51:06Z

cc @julienledem @elahrvivaz

elahrvivaz

looks good to me

wesm · 2017-07-28T02:42:09Z

@jacques-n @StevenMPhillips @siddharthteotia do you see any issues with this?

siddharthteotia · 2017-07-28T23:47:11Z

@BryanCutler

I would like to know why is there a need to close a vector twice. The current change is structured such that following works and the last statement doesn't raise an error.

vector.close()
allocator.close()
vector.close()

Once close() operation is invoked on the vector and allocator, I expect the resources associated to be garbage collected properly. Under what conditions, do we still the 3rd statement?

Thanks,
Siddharth

jacques-n · 2017-07-28T23:51:18Z

- data = null;
- }
- super.close();
+ data = null;


I would prefer doing this change differently. Maybe by allowing allocator to return an empty buffer even if closed. This is because it makes bugs/issues much easier to understand than getting an NPE.

Sure, that sounds fine. As far as I can this would not cause any issues.

BryanCutler · 2017-07-31T17:48:33Z

I would like to know why is there a need to close a vector twice.

@siddharthteotia , this came from a discussion in Spark here. The root/allocator are being used in an iterator and are closed normally after the last iteration, however there are some ways this wouldn't happen (say if a task was cancelled), so they are also closed in a callback to prevent any leaks. Right now an extra flag is being used, which is fine but it would be cleaner to just allow them to call close a second time and not do anything.

…s and assigns an empty buffer

BryanCutler · 2017-08-01T00:17:53Z

 }

 private ArrowBuf createEmpty() {
- assertOpen();


this did not seem necessary either, because an allocator will always hold an instance of an empty buffer. Does this seem ok?

BryanCutler · 2017-08-01T00:41:25Z

@jacques-n , I made the change to use an empty buffer instead of assigning null. This essentially made close() the same as clear() and could be removed. Now in the super class BaseValueVector.close() will invoke clear() to release any buffer and assign data to an empty buffer.

It also seems to me that all Nullable vector would now have a close() method that is identical to clear(). Is that maybe something to remove and cleanup here also? Should there be any different meaning to closing vs. clearing a vector?

…Root-close-twice-ARROW-1283

BryanCutler · 2017-08-03T18:24:42Z

 }

- // TODO: Nullable vectors extend BaseDataValueVector but do not use the data field
- // We should fix the inheritance tree


@siddharthteotia I believe you resolved this comment in #892. Does this PR look ok to you?

wesm · 2017-08-04T18:25:14Z

The CI failure is unrelated to these changes. Can this be merged?

wesm

+1

BryanCutler · 2017-08-07T21:57:19Z

Thanks @wesm!

This change allows the VectorSchemaRoot/FieldVectors to close more than once, even if the allocator has already been closed. Before, an empty ArrowBuf was created during closing which required the allocator to not be closed, however this empty buffer is not needed once the FieldVector has been closed. Author: Bryan Cutler <cutlerb@gmail.com> Closesapache#898 from BryanCutler/java-vectorSchemaRoot-close-twice-ARROW-1283 and squashes the following commits: 2921d84 [Bryan Cutler] removed resolved comment 3b3718b [Bryan Cutler] Merge remote-tracking branch 'upstream/master' into java-vectorSchemaRoot-close-twice-ARROW-1283 e992fc7 [Bryan Cutler] BaseDataValueVector.close will now just clear, which releases previous and assigns an empty buffer 8ecfce2 [Bryan Cutler] Merge remote-tracking branch 'upstream/master' into java-vectorSchemaRoot-close-twice-ARROW-1283 ca38d3d [Bryan Cutler] use clear to release data, ensure that an empty buffer is never allocated again after closing 10ff7c3 [Bryan Cutler] Added regression test

BryanCutler added 3 commits July 26, 2017 18:37

Added regression test
10ff7c3

use clear to release data, ensure that an empty buffer is never alloc…
ca38d3d
…ated again after closing

Merge remote-tracking branch 'upstream/master' into java-vectorSchema…
8ecfce2
…Root-close-twice-ARROW-1283

elahrvivaz reviewed Jul 27, 2017
View reviewed changes

jacques-n reviewed Jul 28, 2017
View reviewed changes

BaseDataValueVector.close will now just clear, which releases previou…
e992fc7
…s and assigns an empty buffer

BryanCutler commented Aug 1, 2017
View reviewed changes

BryanCutler added 2 commits August 3, 2017 10:35

Merge remote-tracking branch 'upstream/master' into java-vectorSchema…
3b3718b
…Root-close-twice-ARROW-1283

removed resolved comment
2921d84

BryanCutler force-pushed the java-vectorSchemaRoot-close-twice-ARROW-1283 branch from 9dd1f07 to 2921d84Compare August 3, 2017 18:23

BryanCutler commented Aug 3, 2017
View reviewed changes

wesm mentioned this pull request Aug 7, 2017
ARROW-1304: [Java] Fix Indentation, WhitespaceAround and EmptyLineSeparator checkstyle warnings in Java #930
Closed

wesm approved these changes Aug 7, 2017
View reviewed changes

asfgit closed this in f9d9833Aug 7, 2017

BryanCutler deleted the java-vectorSchemaRoot-close-twice-ARROW-1283 branch November 7, 2017 23:50

jdimeo mentioned this pull request Nov 3, 2023
SNOW-755767: snowflake-jdbc driver fails with JDK >= 16 snowflakedb/snowflake-jdbc#589
Closed

BryanCutler added 3 commits July 26, 2017 18:37

Added regression test
10ff7c3

use clear to release data, ensure that an empty buffer is never alloc…
ca38d3d
…ated again after closing

Merge remote-tracking branch 'upstream/master' into java-vectorSchema…
8ecfce2
…Root-close-twice-ARROW-1283

elahrvivaz reviewed Jul 27, 2017
View reviewed changes

jacques-n reviewed Jul 28, 2017
View reviewed changes

BaseDataValueVector.close will now just clear, which releases previou…
e992fc7
…s and assigns an empty buffer

BryanCutler commented Aug 1, 2017
View reviewed changes

BryanCutler added 2 commits August 3, 2017 10:35

Merge remote-tracking branch 'upstream/master' into java-vectorSchema…
3b3718b
…Root-close-twice-ARROW-1283

removed resolved comment
2921d84

BryanCutler force-pushed the java-vectorSchemaRoot-close-twice-ARROW-1283 branch from 9dd1f07 to 2921d84Compare August 3, 2017 18:23

BryanCutler commented Aug 3, 2017
View reviewed changes

wesm mentioned this pull request Aug 7, 2017
ARROW-1304: [Java] Fix Indentation, WhitespaceAround and EmptyLineSeparator checkstyle warnings in Java #930
Closed

wesm approved these changes Aug 7, 2017
View reviewed changes

asfgit closed this in f9d9833Aug 7, 2017

BryanCutler deleted the java-vectorSchemaRoot-close-twice-ARROW-1283 branch November 7, 2017 23:50

jdimeo mentioned this pull request Nov 3, 2023
SNOW-755767: snowflake-jdbc driver fails with JDK >= 16 snowflakedb/snowflake-jdbc#589
Closed

BryanCutler added 3 commits July 26, 2017 18:37

Added regression test
10ff7c3

use clear to release data, ensure that an empty buffer is never alloc…
ca38d3d
…ated again after closing

Merge remote-tracking branch 'upstream/master' into java-vectorSchema…
8ecfce2
…Root-close-twice-ARROW-1283

elahrvivaz reviewed Jul 27, 2017
View reviewed changes

jacques-n reviewed Jul 28, 2017
View reviewed changes

BaseDataValueVector.close will now just clear, which releases previou…
e992fc7
…s and assigns an empty buffer

BryanCutler commented Aug 1, 2017
View reviewed changes

BryanCutler added 2 commits August 3, 2017 10:35

Merge remote-tracking branch 'upstream/master' into java-vectorSchema…
3b3718b
…Root-close-twice-ARROW-1283

removed resolved comment
2921d84

BryanCutler force-pushed the java-vectorSchemaRoot-close-twice-ARROW-1283 branch from 9dd1f07 to 2921d84Compare August 3, 2017 18:23

BryanCutler commented Aug 3, 2017
View reviewed changes

wesm mentioned this pull request Aug 7, 2017
ARROW-1304: [Java] Fix Indentation, WhitespaceAround and EmptyLineSeparator checkstyle warnings in Java #930
Closed

wesm approved these changes Aug 7, 2017
View reviewed changes

asfgit closed this in f9d9833Aug 7, 2017

BryanCutler deleted the java-vectorSchemaRoot-close-twice-ARROW-1283 branch November 7, 2017 23:50

jdimeo mentioned this pull request Nov 3, 2023
SNOW-755767: snowflake-jdbc driver fails with JDK >= 16 snowflakedb/snowflake-jdbc#589
Closed

BryanCutler added 3 commits July 26, 2017 18:37

Added regression test
10ff7c3

use clear to release data, ensure that an empty buffer is never alloc…
ca38d3d
…ated again after closing

Merge remote-tracking branch 'upstream/master' into java-vectorSchema…
8ecfce2
…Root-close-twice-ARROW-1283

elahrvivaz reviewed Jul 27, 2017
View reviewed changes

jacques-n reviewed Jul 28, 2017
View reviewed changes

BaseDataValueVector.close will now just clear, which releases previou…
e992fc7
…s and assigns an empty buffer

BryanCutler commented Aug 1, 2017
View reviewed changes

BryanCutler added 2 commits August 3, 2017 10:35

Merge remote-tracking branch 'upstream/master' into java-vectorSchema…
3b3718b
…Root-close-twice-ARROW-1283

removed resolved comment
2921d84

BryanCutler force-pushed the java-vectorSchemaRoot-close-twice-ARROW-1283 branch from 9dd1f07 to 2921d84Compare August 3, 2017 18:23

BryanCutler commented Aug 3, 2017
View reviewed changes

wesm mentioned this pull request Aug 7, 2017
ARROW-1304: [Java] Fix Indentation, WhitespaceAround and EmptyLineSeparator checkstyle warnings in Java #930
Closed

wesm approved these changes Aug 7, 2017
View reviewed changes

asfgit closed this in f9d9833Aug 7, 2017

BryanCutler deleted the java-vectorSchemaRoot-close-twice-ARROW-1283 branch November 7, 2017 23:50

jdimeo mentioned this pull request Nov 3, 2023
SNOW-755767: snowflake-jdbc driver fails with JDK >= 16 snowflakedb/snowflake-jdbc#589
Closed

BryanCutler added 3 commits July 26, 2017 18:37

Added regression test
10ff7c3

use clear to release data, ensure that an empty buffer is never alloc…
ca38d3d
…ated again after closing

Merge remote-tracking branch 'upstream/master' into java-vectorSchema…
8ecfce2
…Root-close-twice-ARROW-1283

elahrvivaz reviewed Jul 27, 2017
View reviewed changes

jacques-n reviewed Jul 28, 2017
View reviewed changes

BaseDataValueVector.close will now just clear, which releases previou…
e992fc7
…s and assigns an empty buffer

BryanCutler commented Aug 1, 2017
View reviewed changes

BryanCutler added 2 commits August 3, 2017 10:35

Merge remote-tracking branch 'upstream/master' into java-vectorSchema…
3b3718b
…Root-close-twice-ARROW-1283

removed resolved comment
2921d84

BryanCutler force-pushed the java-vectorSchemaRoot-close-twice-ARROW-1283 branch from 9dd1f07 to 2921d84Compare August 3, 2017 18:23

BryanCutler commented Aug 3, 2017
View reviewed changes

wesm mentioned this pull request Aug 7, 2017
ARROW-1304: [Java] Fix Indentation, WhitespaceAround and EmptyLineSeparator checkstyle warnings in Java #930
Closed

wesm approved these changes Aug 7, 2017
View reviewed changes

asfgit closed this in f9d9833Aug 7, 2017

BryanCutler deleted the java-vectorSchemaRoot-close-twice-ARROW-1283 branch November 7, 2017 23:50

jdimeo mentioned this pull request Nov 3, 2023
SNOW-755767: snowflake-jdbc driver fails with JDK >= 16 snowflakedb/snowflake-jdbc#589
Closed

BryanCutler added 3 commits July 26, 2017 18:37

Added regression test
10ff7c3

use clear to release data, ensure that an empty buffer is never alloc…
ca38d3d
…ated again after closing

Merge remote-tracking branch 'upstream/master' into java-vectorSchema…
8ecfce2
…Root-close-twice-ARROW-1283

elahrvivaz reviewed Jul 27, 2017
View reviewed changes

jacques-n reviewed Jul 28, 2017
View reviewed changes

BaseDataValueVector.close will now just clear, which releases previou…
e992fc7
…s and assigns an empty buffer

BryanCutler commented Aug 1, 2017
View reviewed changes

BryanCutler added 2 commits August 3, 2017 10:35

Merge remote-tracking branch 'upstream/master' into java-vectorSchema…
3b3718b
…Root-close-twice-ARROW-1283

removed resolved comment
2921d84

BryanCutler force-pushed the java-vectorSchemaRoot-close-twice-ARROW-1283 branch from 9dd1f07 to 2921d84Compare August 3, 2017 18:23

BryanCutler commented Aug 3, 2017
View reviewed changes

wesm mentioned this pull request Aug 7, 2017
ARROW-1304: [Java] Fix Indentation, WhitespaceAround and EmptyLineSeparator checkstyle warnings in Java #930
Closed

wesm approved these changes Aug 7, 2017
View reviewed changes

asfgit closed this in f9d9833Aug 7, 2017

BryanCutler deleted the java-vectorSchemaRoot-close-twice-ARROW-1283 branch November 7, 2017 23:50

jdimeo mentioned this pull request Nov 3, 2023
SNOW-755767: snowflake-jdbc driver fails with JDK >= 16 snowflakedb/snowflake-jdbc#589
Closed

BryanCutler added 3 commits July 26, 2017 18:37

Added regression test
10ff7c3

use clear to release data, ensure that an empty buffer is never alloc…
ca38d3d
…ated again after closing

Merge remote-tracking branch 'upstream/master' into java-vectorSchema…
8ecfce2
…Root-close-twice-ARROW-1283

elahrvivaz reviewed Jul 27, 2017
View reviewed changes

jacques-n reviewed Jul 28, 2017
View reviewed changes

BaseDataValueVector.close will now just clear, which releases previou…
e992fc7
…s and assigns an empty buffer

BryanCutler commented Aug 1, 2017
View reviewed changes

BryanCutler added 2 commits August 3, 2017 10:35

Merge remote-tracking branch 'upstream/master' into java-vectorSchema…
3b3718b
…Root-close-twice-ARROW-1283

removed resolved comment
2921d84

BryanCutler force-pushed the java-vectorSchemaRoot-close-twice-ARROW-1283 branch from 9dd1f07 to 2921d84Compare August 3, 2017 18:23

BryanCutler commented Aug 3, 2017
View reviewed changes

wesm mentioned this pull request Aug 7, 2017
ARROW-1304: [Java] Fix Indentation, WhitespaceAround and EmptyLineSeparator checkstyle warnings in Java #930
Closed

wesm approved these changes Aug 7, 2017
View reviewed changes

asfgit closed this in f9d9833Aug 7, 2017

BryanCutler deleted the java-vectorSchemaRoot-close-twice-ARROW-1283 branch November 7, 2017 23:50

jdimeo mentioned this pull request Nov 3, 2023
SNOW-755767: snowflake-jdbc driver fails with JDK >= 16 snowflakedb/snowflake-jdbc#589
Closed

BryanCutler added 3 commits July 26, 2017 18:37

Added regression test
10ff7c3

use clear to release data, ensure that an empty buffer is never alloc…
ca38d3d
…ated again after closing

Merge remote-tracking branch 'upstream/master' into java-vectorSchema…
8ecfce2
…Root-close-twice-ARROW-1283

elahrvivaz reviewed Jul 27, 2017
View reviewed changes

jacques-n reviewed Jul 28, 2017
View reviewed changes

BaseDataValueVector.close will now just clear, which releases previou…
e992fc7
…s and assigns an empty buffer

BryanCutler commented Aug 1, 2017
View reviewed changes

BryanCutler added 2 commits August 3, 2017 10:35

Merge remote-tracking branch 'upstream/master' into java-vectorSchema…
3b3718b
…Root-close-twice-ARROW-1283

removed resolved comment
2921d84

BryanCutler force-pushed the java-vectorSchemaRoot-close-twice-ARROW-1283 branch from 9dd1f07 to 2921d84Compare August 3, 2017 18:23

BryanCutler commented Aug 3, 2017
View reviewed changes

wesm mentioned this pull request Aug 7, 2017
ARROW-1304: [Java] Fix Indentation, WhitespaceAround and EmptyLineSeparator checkstyle warnings in Java #930
Closed

wesm approved these changes Aug 7, 2017
View reviewed changes

asfgit closed this in f9d9833Aug 7, 2017

BryanCutler deleted the java-vectorSchemaRoot-close-twice-ARROW-1283 branch November 7, 2017 23:50

jdimeo mentioned this pull request Nov 3, 2023
SNOW-755767: snowflake-jdbc driver fails with JDK >= 16 snowflakedb/snowflake-jdbc#589
Closed

Uh oh!

Conversation

BryanCutler commented Jul 27, 2017

Uh oh!

BryanCutler commented Jul 27, 2017

Uh oh!

elahrvivaz left a comment

Choose a reason for hiding this comment

Uh oh!

wesm commented Jul 28, 2017

Uh oh!

siddharthteotia commented Jul 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryanCutler commented Jul 31, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BryanCutlerAug 1, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryanCutler commented Aug 1, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Aug 4, 2017

Uh oh!

wesm left a comment

Choose a reason for hiding this comment

Uh oh!

BryanCutler commented Aug 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

BryanCutler commented Jul 27, 2017

Uh oh!

BryanCutler commented Jul 27, 2017

Uh oh!

elahrvivaz left a comment

Choose a reason for hiding this comment

Uh oh!

wesm commented Jul 28, 2017

Uh oh!

siddharthteotia commented Jul 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryanCutler commented Jul 31, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BryanCutlerAug 1, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryanCutler commented Aug 1, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Aug 4, 2017

Uh oh!

wesm left a comment

Choose a reason for hiding this comment

Uh oh!

BryanCutler commented Aug 7, 2017

Uh oh!

BryanCutler commented Jul 31, 2017•
edited
Loading

BryanCutlerAug 1, 2017•
edited
Loading

BryanCutler commented Aug 1, 2017•
edited
Loading

BryanCutler commented Jul 31, 2017•
edited
Loading

BryanCutlerAug 1, 2017•
edited
Loading

BryanCutler commented Aug 1, 2017•
edited
Loading

BryanCutler commented Jul 31, 2017•
edited
Loading

BryanCutlerAug 1, 2017•
edited
Loading

BryanCutler commented Aug 1, 2017•
edited
Loading