You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Hanifi Gunes <ha...@gmail.com> on 2015/04/10 01:05:19 UTC
Review Request 33035: DRILL-2719: ValueVector#getBuffers(clear) must
consistently clear vectors & retain buffers
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33035/
-----------------------------------------------------------
Review request for drill, Mehant Baid and Parth Chandra.
Repository: drill-git
Description
-------
DRILL-2719: ValueVector#getBuffers(clear) must consistently clear vectors & retain buffers
BaseDataValueVector
- getBuffers now rely on getBufferSize while determining buffers to return
- getBuffers maintains reference count to underlying buffers while clearing the vector
- getBufferSize relies on value count reported by accessor while determining buffer size
- replaced DeadBuf references with an empty buffer. underlying buffer now should never be *null*.
Templates & VV subtypes
- ensure getBuffers conforms to VV#getBuffers
TestEmptyPopulator
- make mock allocator return an empty buffer when requested
Diffs
-----
exec/java-exec/src/main/codegen/templates/NullableValueVectors.java 075316e4f3ac5327e7893688c5e88cfee98e50bc
exec/java-exec/src/main/codegen/templates/RepeatedValueVectors.java c7cf8e6fe18f1b9813ae22495ac79a447f61cfff
exec/java-exec/src/main/codegen/templates/VariableLengthVectors.java 61a24814fd032e7112f4cf4ca7310517df58cfd3
exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseDataValueVector.java d48ea99237bb822cafc8b835c3af0f4789c6eb29
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractMapVector.java b0783afe57317dcd8dc7a2a8d967dcdb1f305edb
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedListVector.java c0f529961343145d67f835a95f58c4eaf2fae2a4
exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/TestEmptyPopulator.java 8426a6abbf0c20be6f81bc82521f31ed7cde2557
Diff: https://reviews.apache.org/r/33035/diff/
Testing
-------
unit and beyond
Thanks,
Hanifi Gunes
Re: Review Request 33035: DRILL-2719: ValueVector#getBuffers(clear)
must consistently clear vectors & retain buffers
Posted by Hanifi Gunes <ha...@gmail.com>.
> On April 10, 2015, 4:43 p.m., Mehant Baid wrote:
> > exec/java-exec/src/main/codegen/templates/NullableValueVectors.java, line 86
> > <https://reviews.apache.org/r/33035/diff/1/?file=921986#file921986line86>
> >
> > Why are we bubbling up the retain logic in clear to the top level vector from the individual vectors? In the code above the 'if' block we anyway drill down to the individual vectors to get the buffers, might as well do the retain there like we are doing currently?
> >
> > If some of the vectors isn't handling this correctly we should modify those.
We cannot do this as we need to call #clear right after invoking super.getBuffers(true), which will release buffers.
- Hanifi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33035/#review79691
-----------------------------------------------------------
On April 13, 2015, 10:07 p.m., Hanifi Gunes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33035/
> -----------------------------------------------------------
>
> (Updated April 13, 2015, 10:07 p.m.)
>
>
> Review request for drill, Mehant Baid and Parth Chandra.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2719: ValueVector#getBuffers(clear) must consistently clear vectors & retain buffers
>
> BaseDataValueVector
> - getBuffers now rely on getBufferSize while determining buffers to return
> - getBuffers maintains reference count to underlying buffers while clearing the vector
> - getBufferSize relies on value count reported by accessor while determining buffer size
> - replaced DeadBuf references with an empty buffer. underlying buffer now should never be *null*.
>
> Templates & VV subtypes
> - ensure getBuffers conforms to VV#getBuffers
>
> TestEmptyPopulator
> - make mock allocator return an empty buffer when requested
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/codegen/templates/NullableValueVectors.java 075316e4f3ac5327e7893688c5e88cfee98e50bc
> exec/java-exec/src/main/codegen/templates/RepeatedValueVectors.java c7cf8e6fe18f1b9813ae22495ac79a447f61cfff
> exec/java-exec/src/main/codegen/templates/VariableLengthVectors.java edb851eb10be43d889ce5fd98d9bde036707870a
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseDataValueVector.java d48ea99237bb822cafc8b835c3af0f4789c6eb29
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractMapVector.java b0783afe57317dcd8dc7a2a8d967dcdb1f305edb
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedListVector.java c0f529961343145d67f835a95f58c4eaf2fae2a4
> exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/TestEmptyPopulator.java 8426a6abbf0c20be6f81bc82521f31ed7cde2557
>
> Diff: https://reviews.apache.org/r/33035/diff/
>
>
> Testing
> -------
>
> unit and beyond
>
>
> Thanks,
>
> Hanifi Gunes
>
>
Re: Review Request 33035: DRILL-2719: ValueVector#getBuffers(clear)
must consistently clear vectors & retain buffers
Posted by Mehant Baid <ba...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33035/#review79691
-----------------------------------------------------------
exec/java-exec/src/main/codegen/templates/NullableValueVectors.java
<https://reviews.apache.org/r/33035/#comment129212>
Why are we bubbling up the retain logic in clear to the top level vector from the individual vectors? In the code above the 'if' block we anyway drill down to the individual vectors to get the buffers, might as well do the retain there like we are doing currently?
If some of the vectors isn't handling this correctly we should modify those.
- Mehant Baid
On April 9, 2015, 11:05 p.m., Hanifi Gunes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33035/
> -----------------------------------------------------------
>
> (Updated April 9, 2015, 11:05 p.m.)
>
>
> Review request for drill, Mehant Baid and Parth Chandra.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2719: ValueVector#getBuffers(clear) must consistently clear vectors & retain buffers
>
> BaseDataValueVector
> - getBuffers now rely on getBufferSize while determining buffers to return
> - getBuffers maintains reference count to underlying buffers while clearing the vector
> - getBufferSize relies on value count reported by accessor while determining buffer size
> - replaced DeadBuf references with an empty buffer. underlying buffer now should never be *null*.
>
> Templates & VV subtypes
> - ensure getBuffers conforms to VV#getBuffers
>
> TestEmptyPopulator
> - make mock allocator return an empty buffer when requested
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/codegen/templates/NullableValueVectors.java 075316e4f3ac5327e7893688c5e88cfee98e50bc
> exec/java-exec/src/main/codegen/templates/RepeatedValueVectors.java c7cf8e6fe18f1b9813ae22495ac79a447f61cfff
> exec/java-exec/src/main/codegen/templates/VariableLengthVectors.java 61a24814fd032e7112f4cf4ca7310517df58cfd3
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseDataValueVector.java d48ea99237bb822cafc8b835c3af0f4789c6eb29
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractMapVector.java b0783afe57317dcd8dc7a2a8d967dcdb1f305edb
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedListVector.java c0f529961343145d67f835a95f58c4eaf2fae2a4
> exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/TestEmptyPopulator.java 8426a6abbf0c20be6f81bc82521f31ed7cde2557
>
> Diff: https://reviews.apache.org/r/33035/diff/
>
>
> Testing
> -------
>
> unit and beyond
>
>
> Thanks,
>
> Hanifi Gunes
>
>
Re: Review Request 33035: DRILL-2719: ValueVector#getBuffers(clear)
must consistently clear vectors & retain buffers
Posted by Hanifi Gunes <ha...@gmail.com>.
> On April 10, 2015, 5:42 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractMapVector.java, line 222
> > <https://reviews.apache.org/r/33035/diff/1/?file=921990#file921990line222>
> >
> > I thought this needed to call buf.retain() unconditionally (not just if (clear)), since we're adding references by putting these into the resulting array. I also changed the comment in BaseDataVector.getBuffers() about this -- that claims that no new references are added, but that's wrong.
That's not needed. As buffers are not released yet we can hand over without retaining.
Problems may surface when getBuffers(false) is called. Once this call is made both the vector and consumer operate on the same set of buffers, which sounds unsafe.
- Hanifi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33035/#review79714
-----------------------------------------------------------
On April 13, 2015, 10:07 p.m., Hanifi Gunes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33035/
> -----------------------------------------------------------
>
> (Updated April 13, 2015, 10:07 p.m.)
>
>
> Review request for drill, Mehant Baid and Parth Chandra.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2719: ValueVector#getBuffers(clear) must consistently clear vectors & retain buffers
>
> BaseDataValueVector
> - getBuffers now rely on getBufferSize while determining buffers to return
> - getBuffers maintains reference count to underlying buffers while clearing the vector
> - getBufferSize relies on value count reported by accessor while determining buffer size
> - replaced DeadBuf references with an empty buffer. underlying buffer now should never be *null*.
>
> Templates & VV subtypes
> - ensure getBuffers conforms to VV#getBuffers
>
> TestEmptyPopulator
> - make mock allocator return an empty buffer when requested
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/codegen/templates/NullableValueVectors.java 075316e4f3ac5327e7893688c5e88cfee98e50bc
> exec/java-exec/src/main/codegen/templates/RepeatedValueVectors.java c7cf8e6fe18f1b9813ae22495ac79a447f61cfff
> exec/java-exec/src/main/codegen/templates/VariableLengthVectors.java edb851eb10be43d889ce5fd98d9bde036707870a
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseDataValueVector.java d48ea99237bb822cafc8b835c3af0f4789c6eb29
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractMapVector.java b0783afe57317dcd8dc7a2a8d967dcdb1f305edb
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedListVector.java c0f529961343145d67f835a95f58c4eaf2fae2a4
> exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/TestEmptyPopulator.java 8426a6abbf0c20be6f81bc82521f31ed7cde2557
>
> Diff: https://reviews.apache.org/r/33035/diff/
>
>
> Testing
> -------
>
> unit and beyond
>
>
> Thanks,
>
> Hanifi Gunes
>
>
Re: Review Request 33035: DRILL-2719: ValueVector#getBuffers(clear)
must consistently clear vectors & retain buffers
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33035/#review79714
-----------------------------------------------------------
I've been making changes in some of the same files because my work on the allocator rewrite has found some problems with accounting, so it made sense for me to look this over.
exec/java-exec/src/main/codegen/templates/NullableValueVectors.java
<https://reviews.apache.org/r/33035/#comment129240>
From what I've been seeing in my allocator work, it *does* make sense to do this -- you're handing out new references (in this array), so they have to be refcounted. Once getBuffers() returns, we don't know what the caller will do (keep the array, copy the references to somewhere else persistent, etc). So you might change that comment.
exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseDataValueVector.java
<https://reviews.apache.org/r/33035/#comment129242>
I'm glad you got rid of DeadBuf, that was just obscuring what was going on.
exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseDataValueVector.java
<https://reviews.apache.org/r/33035/#comment129246>
For the case where getBufferSize() == 0, this will increment the refcount even though we're not handing out a new reference. It seems like the data.retain should be in the else block where out is assigned new DrillBuf[]{data}, since that is the new reference we're handing out.
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractMapVector.java
<https://reviews.apache.org/r/33035/#comment129249>
I thought this needed to call buf.retain() unconditionally (not just if (clear)), since we're adding references by putting these into the resulting array. I also changed the comment in BaseDataVector.getBuffers() about this -- that claims that no new references are added, but that's wrong.
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedListVector.java
<https://reviews.apache.org/r/33035/#comment129251>
It does make sense.
- Chris Westin
On April 9, 2015, 4:05 p.m., Hanifi Gunes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33035/
> -----------------------------------------------------------
>
> (Updated April 9, 2015, 4:05 p.m.)
>
>
> Review request for drill, Mehant Baid and Parth Chandra.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2719: ValueVector#getBuffers(clear) must consistently clear vectors & retain buffers
>
> BaseDataValueVector
> - getBuffers now rely on getBufferSize while determining buffers to return
> - getBuffers maintains reference count to underlying buffers while clearing the vector
> - getBufferSize relies on value count reported by accessor while determining buffer size
> - replaced DeadBuf references with an empty buffer. underlying buffer now should never be *null*.
>
> Templates & VV subtypes
> - ensure getBuffers conforms to VV#getBuffers
>
> TestEmptyPopulator
> - make mock allocator return an empty buffer when requested
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/codegen/templates/NullableValueVectors.java 075316e4f3ac5327e7893688c5e88cfee98e50bc
> exec/java-exec/src/main/codegen/templates/RepeatedValueVectors.java c7cf8e6fe18f1b9813ae22495ac79a447f61cfff
> exec/java-exec/src/main/codegen/templates/VariableLengthVectors.java 61a24814fd032e7112f4cf4ca7310517df58cfd3
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseDataValueVector.java d48ea99237bb822cafc8b835c3af0f4789c6eb29
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractMapVector.java b0783afe57317dcd8dc7a2a8d967dcdb1f305edb
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedListVector.java c0f529961343145d67f835a95f58c4eaf2fae2a4
> exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/TestEmptyPopulator.java 8426a6abbf0c20be6f81bc82521f31ed7cde2557
>
> Diff: https://reviews.apache.org/r/33035/diff/
>
>
> Testing
> -------
>
> unit and beyond
>
>
> Thanks,
>
> Hanifi Gunes
>
>
Re: Review Request 33035: DRILL-2719: ValueVector#getBuffers(clear)
must consistently clear vectors & retain buffers
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33035/#review79955
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractMapVector.java
<https://reviews.apache.org/r/33035/#comment129647>
Right. It's safer if both maintain their own reference counts, which is what happens if you call retain() unconditionally here. Then the AbstractMapVector has its count, and the caller has its count. The vector isn't necessarily going to operate on it, but for consistency they need to maintain their own counts, otherwise, the history of these calls matters when deciding to release, which is harder to reason about.
- Chris Westin
On April 13, 2015, 3:07 p.m., Hanifi Gunes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33035/
> -----------------------------------------------------------
>
> (Updated April 13, 2015, 3:07 p.m.)
>
>
> Review request for drill, Mehant Baid and Parth Chandra.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2719: ValueVector#getBuffers(clear) must consistently clear vectors & retain buffers
>
> BaseDataValueVector
> - getBuffers now rely on getBufferSize while determining buffers to return
> - getBuffers maintains reference count to underlying buffers while clearing the vector
> - getBufferSize relies on value count reported by accessor while determining buffer size
> - replaced DeadBuf references with an empty buffer. underlying buffer now should never be *null*.
>
> Templates & VV subtypes
> - ensure getBuffers conforms to VV#getBuffers
>
> TestEmptyPopulator
> - make mock allocator return an empty buffer when requested
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/codegen/templates/NullableValueVectors.java 075316e4f3ac5327e7893688c5e88cfee98e50bc
> exec/java-exec/src/main/codegen/templates/RepeatedValueVectors.java c7cf8e6fe18f1b9813ae22495ac79a447f61cfff
> exec/java-exec/src/main/codegen/templates/VariableLengthVectors.java edb851eb10be43d889ce5fd98d9bde036707870a
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseDataValueVector.java d48ea99237bb822cafc8b835c3af0f4789c6eb29
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractMapVector.java b0783afe57317dcd8dc7a2a8d967dcdb1f305edb
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedListVector.java c0f529961343145d67f835a95f58c4eaf2fae2a4
> exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/TestEmptyPopulator.java 8426a6abbf0c20be6f81bc82521f31ed7cde2557
>
> Diff: https://reviews.apache.org/r/33035/diff/
>
>
> Testing
> -------
>
> unit and beyond
>
>
> Thanks,
>
> Hanifi Gunes
>
>
Re: Review Request 33035: DRILL-2719: ValueVector#getBuffers(clear)
must consistently clear vectors & retain buffers
Posted by Mehant Baid <ba...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33035/#review80039
-----------------------------------------------------------
Ship it!
Ship It!
- Mehant Baid
On April 13, 2015, 10:07 p.m., Hanifi Gunes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33035/
> -----------------------------------------------------------
>
> (Updated April 13, 2015, 10:07 p.m.)
>
>
> Review request for drill, Mehant Baid and Parth Chandra.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2719: ValueVector#getBuffers(clear) must consistently clear vectors & retain buffers
>
> BaseDataValueVector
> - getBuffers now rely on getBufferSize while determining buffers to return
> - getBuffers maintains reference count to underlying buffers while clearing the vector
> - getBufferSize relies on value count reported by accessor while determining buffer size
> - replaced DeadBuf references with an empty buffer. underlying buffer now should never be *null*.
>
> Templates & VV subtypes
> - ensure getBuffers conforms to VV#getBuffers
>
> TestEmptyPopulator
> - make mock allocator return an empty buffer when requested
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/codegen/templates/NullableValueVectors.java 075316e4f3ac5327e7893688c5e88cfee98e50bc
> exec/java-exec/src/main/codegen/templates/RepeatedValueVectors.java c7cf8e6fe18f1b9813ae22495ac79a447f61cfff
> exec/java-exec/src/main/codegen/templates/VariableLengthVectors.java edb851eb10be43d889ce5fd98d9bde036707870a
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseDataValueVector.java d48ea99237bb822cafc8b835c3af0f4789c6eb29
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractMapVector.java b0783afe57317dcd8dc7a2a8d967dcdb1f305edb
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedListVector.java c0f529961343145d67f835a95f58c4eaf2fae2a4
> exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/TestEmptyPopulator.java 8426a6abbf0c20be6f81bc82521f31ed7cde2557
>
> Diff: https://reviews.apache.org/r/33035/diff/
>
>
> Testing
> -------
>
> unit and beyond
>
>
> Thanks,
>
> Hanifi Gunes
>
>
Re: Review Request 33035: DRILL-2719: ValueVector#getBuffers(clear)
must consistently clear vectors & retain buffers
Posted by Hanifi Gunes <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33035/
-----------------------------------------------------------
(Updated April 13, 2015, 10:07 p.m.)
Review request for drill, Mehant Baid and Parth Chandra.
Changes
-------
addressed comments.
Repository: drill-git
Description
-------
DRILL-2719: ValueVector#getBuffers(clear) must consistently clear vectors & retain buffers
BaseDataValueVector
- getBuffers now rely on getBufferSize while determining buffers to return
- getBuffers maintains reference count to underlying buffers while clearing the vector
- getBufferSize relies on value count reported by accessor while determining buffer size
- replaced DeadBuf references with an empty buffer. underlying buffer now should never be *null*.
Templates & VV subtypes
- ensure getBuffers conforms to VV#getBuffers
TestEmptyPopulator
- make mock allocator return an empty buffer when requested
Diffs (updated)
-----
exec/java-exec/src/main/codegen/templates/NullableValueVectors.java 075316e4f3ac5327e7893688c5e88cfee98e50bc
exec/java-exec/src/main/codegen/templates/RepeatedValueVectors.java c7cf8e6fe18f1b9813ae22495ac79a447f61cfff
exec/java-exec/src/main/codegen/templates/VariableLengthVectors.java edb851eb10be43d889ce5fd98d9bde036707870a
exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseDataValueVector.java d48ea99237bb822cafc8b835c3af0f4789c6eb29
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractMapVector.java b0783afe57317dcd8dc7a2a8d967dcdb1f305edb
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedListVector.java c0f529961343145d67f835a95f58c4eaf2fae2a4
exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/TestEmptyPopulator.java 8426a6abbf0c20be6f81bc82521f31ed7cde2557
Diff: https://reviews.apache.org/r/33035/diff/
Testing
-------
unit and beyond
Thanks,
Hanifi Gunes