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/05/07 05:41:00 UTC

Review Request 33925: DRILL-2150: Create an abstraction for repeated value vectors.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33925/
-----------------------------------------------------------

Review request for drill, Mehant Baid and Parth Chandra.


Bugs: DRILL-2150
    https://issues.apache.org/jira/browse/DRILL-2150


Repository: drill-git


Description
-------

DRILL-2150: Create an abstraction for repeated value vectors.

All value vectors
- Rely on getMetadataBuilder to iteratively construct vector metadata rather than overriding getMetadata in each vector
- Fixed misc issues enhacing code re-use
- Removed generic template reference to concrete VV type from VV interface.

ComplexWriters
- Fixed an off by one bug

FixedValueVectors
- Fixed a value count bug

RepeatedValueVectors
- Fixed wrong implementation of getValueCount. A value is whatever is stored in the vector regardless if it is another vector or scalar type. The behavior is consistent everywhere.
- Introduced getInnerValueCount and getInnerValueCountAt for values sitting at second level to repeated types.
- All subclasses implement RepeatedVV interface
- Common functionality is abstracted out to base classes

FlattenRB
- Refactored set flatten field block to make it more readable
- Fixed a wrong getValueCount call

ProjectRB
- Fixed an issue regarding multiple projections not initializing nested vectors.

ZeroVector
- Used as initial vector value in repeated vectors instead of a "null" reference

ContainerVectorLike
- Common trait/mixin shared by nested vectors.
- Centralized logic to add vectors to nested types via use of VectorDescriptor


Diffs
-----

  exec/java-exec/src/main/codegen/templates/ComplexReaders.java fa1dac4ecce88e99b4c0b8cfcaec6095c6fed469 
  exec/java-exec/src/main/codegen/templates/ComplexWriters.java 49c75d132ac015931486c16913c327cc8f69678a 
  exec/java-exec/src/main/codegen/templates/FixedValueVectors.java 6a924b762c99c0699dfb45eb50622a4b0e692a79 
  exec/java-exec/src/main/codegen/templates/ListWriters.java 6df4248d1fe527a3c2da86b880a13366f29aa5fb 
  exec/java-exec/src/main/codegen/templates/MapWriters.java 6ee80351dbab5c16abd5ed2cc8fd40879a953afe 
  exec/java-exec/src/main/codegen/templates/RepeatedValueVectors.java c06e29c3b1903016c1cd500ff3c5bebc47b8a1de 
  exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/flatten/FlattenRecordBatch.java 7a5b352b0b1210626cd19a62d63b73afdd756409 
  exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/flatten/FlattenTemplate.java 96209a23b3eb4f61dd9bc98a3dad53dc3938f5bd 
  exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/flatten/Flattener.java 2141ca2c47b19d66dc6cfce3fe088d7299c576ed 
  exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/project/ProjectRecordBatch.java 7b9fffb3a734f0063faec9f681b257ec98083da5 
  exec/java-exec/src/main/java/org/apache/drill/exec/store/VectorHolder.java 8387d49a61cb50c55863238fd9759daacb928cfc 
  exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/FixedWidthRepeatedReader.java 7f8b6117570e21d84c952438807c51a52c900d92 
  exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/ParquetRecordReader.java 11d0042b5eef7977ba666100c0c5d66d69225266 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/AllocationHelper.java 7c77ca215b63a8dc3b921d0ada99c564f9fd81c1 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseDataValueVector.java 0c6097c480a4ac0bdc6e37c16f0f7a643d1ca175 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseRepeatedValueVector.java PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseValueVector.java 22f0fe7406139ac0bbe89fa51b1ff60591fb4df8 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/ContainerVectorLike.java PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedFixedWidthVector.java eaae7ad6032f52ea3a2a718563096dc853bc1628 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedValueVector.java PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedVariableWidthVector.java a49934168af1120b75cbc99911551f89bf56ea12 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedVector.java df4279ac4566a55b9451cd2be6a7e57d3bcc4db8 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/SchemaChangeCallBack.java 386ee34954b08f85e0ac2bc0db7e263566eb1ef6 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/ValueVector.java e4a0997ed12585ae27e06daf9bc242d9e1e8f6b6 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/VectorDescriptor.java PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/ZeroVector.java PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/MapVector.java 41388393c217973b9be910c69f15ee8e099f9d22 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedListVector.java c061029991f145751b686a9738a4021ec1cb12f6 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedMapVector.java e5d48dd6fb07e7bd08b1b06e87ae24d839ffcaf2 
  exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestValueVector.java 1564aea4d41df3016b6b179162d0a0001bf62ca9 

Diff: https://reviews.apache.org/r/33925/diff/


Testing
-------

All tests pass.


Thanks,

Hanifi Gunes

Re: Review Request 33925: DRILL-2150: Create an abstraction for repeated value vectors.

Posted by Hanifi Gunes <ha...@gmail.com>.

> On May 8, 2015, 4:42 a.m., Parth Chandra wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseRepeatedValueVector.java, line 132
> > <https://reviews.apache.org/r/33925/diff/1/?file=951997#file951997line132>
> >
> >     Add a comment explaining what size is supposed to return.

ContainerVectorLike#size defines the behavior but I will give more context here.


> On May 8, 2015, 4:42 a.m., Parth Chandra wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedValueVector.java, line 38
> > <https://reviews.apache.org/r/33925/diff/1/?file=952001#file952001line38>
> >
> >     Ideally, we should not expose this. Perhaps we can add a TODO (and JIRA) to eliminate the need to access the offeset vector.

Filed D-2995 for family of issues regarding elimination of low-level access to RVVs.


> On May 8, 2015, 4:42 a.m., Parth Chandra wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/FixedWidthRepeatedReader.java, line 206
> > <https://reviews.apache.org/r/33925/diff/1/?file=951993#file951993line206>
> >
> >     Isn't the old implementation cleaner? There is no reason for a reader to be aware of the internal representation of a repeated vector, let alone manipulate it.

Please see my comment below regarding low-level API access.


> On May 8, 2015, 4:42 a.m., Parth Chandra wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedListVector.java, line 244
> > <https://reviews.apache.org/r/33925/diff/1/?file=952009#file952009line244>
> >
> >     We should log a JIRA for this TODO

Filed D-2997 for this.


> On May 8, 2015, 4:42 a.m., Parth Chandra wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseRepeatedValueVector.java, line 149
> > <https://reviews.apache.org/r/33925/diff/1/?file=951997#file951997line149>
> >
> >     throw UserException here?

After a quick discussion with Hakeem, we decided to throw a more specific exception considering it is too early to throw a UserException in the VVs.


On May 8, 2015, 4:42 a.m., Hanifi Gunes wrote:
> > RB won't let me see the diff, so this comment may be inaccurate. It appears that RepeatedVariableWidthVectorLike.java is not consistent with RepeatedFixedWidthVectorLike. The former is a Vector (extends ValueVector) while the latter is not a Vector (hence VectorLike).

Just checked on this. It does not seem to be the case.


- Hanifi


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33925/#review82796
-----------------------------------------------------------


On May 11, 2015, 6:49 a.m., Hanifi Gunes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33925/
> -----------------------------------------------------------
> 
> (Updated May 11, 2015, 6:49 a.m.)
> 
> 
> Review request for drill, Mehant Baid and Parth Chandra.
> 
> 
> Bugs: DRILL-2150
>     https://issues.apache.org/jira/browse/DRILL-2150
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> DRILL-2150: Create an abstraction for repeated value vectors.
> 
> All value vectors
> - Rely on getMetadataBuilder to iteratively construct vector metadata rather than overriding getMetadata in each vector
> - Fixed misc issues enhacing code re-use
> - Removed generic template reference to concrete VV type from VV interface.
> 
> ComplexWriters
> - Fixed an off by one bug
> 
> FixedValueVectors
> - Fixed a value count bug
> 
> RepeatedValueVectors
> - Fixed wrong implementation of getValueCount. A value is whatever is stored in the vector regardless if it is another vector or scalar type. The behavior is consistent everywhere.
> - Introduced getInnerValueCount and getInnerValueCountAt for values sitting at second level to repeated types.
> - All subclasses implement RepeatedVV interface
> - Common functionality is abstracted out to base classes
> 
> FlattenRB
> - Refactored set flatten field block to make it more readable
> - Fixed a wrong getValueCount call
> 
> ProjectRB
> - Fixed an issue regarding multiple projections not initializing nested vectors.
> 
> ZeroVector
> - Used as initial vector value in repeated vectors instead of a "null" reference
> 
> ContainerVectorLike
> - Common trait/mixin shared by nested vectors.
> - Centralized logic to add vectors to nested types via use of VectorDescriptor
> 
> 
> Diffs
> -----
> 
>   exec/java-exec/src/main/codegen/templates/ComplexReaders.java fa1dac4ecce88e99b4c0b8cfcaec6095c6fed469 
>   exec/java-exec/src/main/codegen/templates/ComplexWriters.java 49c75d132ac015931486c16913c327cc8f69678a 
>   exec/java-exec/src/main/codegen/templates/FixedValueVectors.java a805b8e239d9db2f2d203f5a653919aaaa4a1149 
>   exec/java-exec/src/main/codegen/templates/ListWriters.java 6df4248d1fe527a3c2da86b880a13366f29aa5fb 
>   exec/java-exec/src/main/codegen/templates/MapWriters.java 38df84bf2bc0fd49ce18a115c512ad6098e11976 
>   exec/java-exec/src/main/codegen/templates/RepeatedValueVectors.java c0fba660c734d9c35e577b3c3b8f9b81f3a7f65c 
>   exec/java-exec/src/main/java/org/apache/drill/exec/exception/SchemaChangeRuntimeException.java PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/flatten/FlattenRecordBatch.java 7a5b352b0b1210626cd19a62d63b73afdd756409 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/flatten/FlattenTemplate.java 96209a23b3eb4f61dd9bc98a3dad53dc3938f5bd 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/flatten/Flattener.java 2141ca2c47b19d66dc6cfce3fe088d7299c576ed 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/project/ProjectRecordBatch.java 32ffb6f0b6e29ea3eb095906cdf5e705158924dc 
>   exec/java-exec/src/main/java/org/apache/drill/exec/store/VectorHolder.java 8387d49a61cb50c55863238fd9759daacb928cfc 
>   exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/RepeatedVarCharOutput.java 3ad5c2a36b65e13165947c7a60e9847cbb9a5bf9 
>   exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/FixedWidthRepeatedReader.java 7f8b6117570e21d84c952438807c51a52c900d92 
>   exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/ParquetRecordReader.java 2f07fb3527dbc4e4de25f8af07bba7c1c85d88f8 
>   exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java e25bd74084c25188d20b0cfd91a890fa816ede1d 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/AllocationHelper.java bf465c7994bba8e225ed5ed1fb4402c636e8cf46 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseDataValueVector.java 0c6097c480a4ac0bdc6e37c16f0f7a643d1ca175 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseRepeatedValueVector.java PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseValueVector.java 22f0fe7406139ac0bbe89fa51b1ff60591fb4df8 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/ContainerVectorLike.java PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedFixedWidthVector.java eaae7ad6032f52ea3a2a718563096dc853bc1628 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedValueVector.java PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedVariableWidthVector.java a49934168af1120b75cbc99911551f89bf56ea12 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedVector.java df4279ac4566a55b9451cd2be6a7e57d3bcc4db8 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/SchemaChangeCallBack.java 386ee34954b08f85e0ac2bc0db7e263566eb1ef6 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/ValueVector.java e4a0997ed12585ae27e06daf9bc242d9e1e8f6b6 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/VectorDescriptor.java PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/ZeroVector.java PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/MapVector.java 41388393c217973b9be910c69f15ee8e099f9d22 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedListVector.java c061029991f145751b686a9738a4021ec1cb12f6 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedMapVector.java e5d48dd6fb07e7bd08b1b06e87ae24d839ffcaf2 
>   exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestValueVector.java 1564aea4d41df3016b6b179162d0a0001bf62ca9 
> 
> Diff: https://reviews.apache.org/r/33925/diff/
> 
> 
> Testing
> -------
> 
> All tests pass.
> 
> 
> Thanks,
> 
> Hanifi Gunes
> 
>

Re: Review Request 33925: DRILL-2150: Create an abstraction for repeated value vectors.

Posted by Parth Chandra <pc...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33925/#review82796
-----------------------------------------------------------



exec/java-exec/src/main/codegen/templates/RepeatedValueVectors.java
<https://reviews.apache.org/r/33925/#comment133615>

    Should this be startNewValue instead of startNewGroup?



exec/java-exec/src/main/codegen/templates/RepeatedValueVectors.java
<https://reviews.apache.org/r/33925/#comment133686>

    Are we retaining the parent/child nomenclature or are we going to use inner/outer. Seems that there is room for confusion.



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/flatten/FlattenRecordBatch.java
<https://reviews.apache.org/r/33925/#comment133706>

    Throw a UserException here?



exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/FixedWidthRepeatedReader.java
<https://reviews.apache.org/r/33925/#comment133808>

    Isn't the old implementation cleaner? There is no reason for a reader to be aware of the internal representation of a repeated vector, let alone manipulate it.



exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseRepeatedValueVector.java
<https://reviews.apache.org/r/33925/#comment133813>

    Add a comment explaining what size is supposed to return.



exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseRepeatedValueVector.java
<https://reviews.apache.org/r/33925/#comment133812>

    throw UserException here?



exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedValueVector.java
<https://reviews.apache.org/r/33925/#comment133817>

    Ideally, we should not expose this. Perhaps we can add a TODO (and JIRA) to eliminate the need to access the offeset vector.



exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedListVector.java
<https://reviews.apache.org/r/33925/#comment133830>

    Should use the new UserException model with a message that can be displayed to the user.



exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedListVector.java
<https://reviews.apache.org/r/33925/#comment133831>

    We should log a JIRA for this TODO


RB won't let me see the diff, so this comment may be inaccurate. It appears that RepeatedVariableWidthVectorLike.java is not consistent with RepeatedFixedWidthVectorLike. The former is a Vector (extends ValueVector) while the latter is not a Vector (hence VectorLike).

- Parth Chandra


On May 7, 2015, 3:40 a.m., Hanifi Gunes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33925/
> -----------------------------------------------------------
> 
> (Updated May 7, 2015, 3:40 a.m.)
> 
> 
> Review request for drill, Mehant Baid and Parth Chandra.
> 
> 
> Bugs: DRILL-2150
>     https://issues.apache.org/jira/browse/DRILL-2150
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> DRILL-2150: Create an abstraction for repeated value vectors.
> 
> All value vectors
> - Rely on getMetadataBuilder to iteratively construct vector metadata rather than overriding getMetadata in each vector
> - Fixed misc issues enhacing code re-use
> - Removed generic template reference to concrete VV type from VV interface.
> 
> ComplexWriters
> - Fixed an off by one bug
> 
> FixedValueVectors
> - Fixed a value count bug
> 
> RepeatedValueVectors
> - Fixed wrong implementation of getValueCount. A value is whatever is stored in the vector regardless if it is another vector or scalar type. The behavior is consistent everywhere.
> - Introduced getInnerValueCount and getInnerValueCountAt for values sitting at second level to repeated types.
> - All subclasses implement RepeatedVV interface
> - Common functionality is abstracted out to base classes
> 
> FlattenRB
> - Refactored set flatten field block to make it more readable
> - Fixed a wrong getValueCount call
> 
> ProjectRB
> - Fixed an issue regarding multiple projections not initializing nested vectors.
> 
> ZeroVector
> - Used as initial vector value in repeated vectors instead of a "null" reference
> 
> ContainerVectorLike
> - Common trait/mixin shared by nested vectors.
> - Centralized logic to add vectors to nested types via use of VectorDescriptor
> 
> 
> Diffs
> -----
> 
>   exec/java-exec/src/main/codegen/templates/ComplexReaders.java fa1dac4ecce88e99b4c0b8cfcaec6095c6fed469 
>   exec/java-exec/src/main/codegen/templates/ComplexWriters.java 49c75d132ac015931486c16913c327cc8f69678a 
>   exec/java-exec/src/main/codegen/templates/FixedValueVectors.java 6a924b762c99c0699dfb45eb50622a4b0e692a79 
>   exec/java-exec/src/main/codegen/templates/ListWriters.java 6df4248d1fe527a3c2da86b880a13366f29aa5fb 
>   exec/java-exec/src/main/codegen/templates/MapWriters.java 6ee80351dbab5c16abd5ed2cc8fd40879a953afe 
>   exec/java-exec/src/main/codegen/templates/RepeatedValueVectors.java c06e29c3b1903016c1cd500ff3c5bebc47b8a1de 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/flatten/FlattenRecordBatch.java 7a5b352b0b1210626cd19a62d63b73afdd756409 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/flatten/FlattenTemplate.java 96209a23b3eb4f61dd9bc98a3dad53dc3938f5bd 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/flatten/Flattener.java 2141ca2c47b19d66dc6cfce3fe088d7299c576ed 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/project/ProjectRecordBatch.java 7b9fffb3a734f0063faec9f681b257ec98083da5 
>   exec/java-exec/src/main/java/org/apache/drill/exec/store/VectorHolder.java 8387d49a61cb50c55863238fd9759daacb928cfc 
>   exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/FixedWidthRepeatedReader.java 7f8b6117570e21d84c952438807c51a52c900d92 
>   exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/ParquetRecordReader.java 11d0042b5eef7977ba666100c0c5d66d69225266 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/AllocationHelper.java 7c77ca215b63a8dc3b921d0ada99c564f9fd81c1 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseDataValueVector.java 0c6097c480a4ac0bdc6e37c16f0f7a643d1ca175 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseRepeatedValueVector.java PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseValueVector.java 22f0fe7406139ac0bbe89fa51b1ff60591fb4df8 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/ContainerVectorLike.java PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedFixedWidthVector.java eaae7ad6032f52ea3a2a718563096dc853bc1628 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedValueVector.java PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedVariableWidthVector.java a49934168af1120b75cbc99911551f89bf56ea12 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedVector.java df4279ac4566a55b9451cd2be6a7e57d3bcc4db8 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/SchemaChangeCallBack.java 386ee34954b08f85e0ac2bc0db7e263566eb1ef6 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/ValueVector.java e4a0997ed12585ae27e06daf9bc242d9e1e8f6b6 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/VectorDescriptor.java PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/ZeroVector.java PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/MapVector.java 41388393c217973b9be910c69f15ee8e099f9d22 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedListVector.java c061029991f145751b686a9738a4021ec1cb12f6 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedMapVector.java e5d48dd6fb07e7bd08b1b06e87ae24d839ffcaf2 
>   exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestValueVector.java 1564aea4d41df3016b6b179162d0a0001bf62ca9 
> 
> Diff: https://reviews.apache.org/r/33925/diff/
> 
> 
> Testing
> -------
> 
> All tests pass.
> 
> 
> Thanks,
> 
> Hanifi Gunes
> 
>

Re: Review Request 33925: DRILL-2150: Create an abstraction for repeated value vectors.

Posted by Parth Chandra <pc...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33925/#review83208
-----------------------------------------------------------

Ship it!


Ship It!

- Parth Chandra


On May 11, 2015, 6:49 a.m., Hanifi Gunes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33925/
> -----------------------------------------------------------
> 
> (Updated May 11, 2015, 6:49 a.m.)
> 
> 
> Review request for drill, Mehant Baid and Parth Chandra.
> 
> 
> Bugs: DRILL-2150
>     https://issues.apache.org/jira/browse/DRILL-2150
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> DRILL-2150: Create an abstraction for repeated value vectors.
> 
> All value vectors
> - Rely on getMetadataBuilder to iteratively construct vector metadata rather than overriding getMetadata in each vector
> - Fixed misc issues enhacing code re-use
> - Removed generic template reference to concrete VV type from VV interface.
> 
> ComplexWriters
> - Fixed an off by one bug
> 
> FixedValueVectors
> - Fixed a value count bug
> 
> RepeatedValueVectors
> - Fixed wrong implementation of getValueCount. A value is whatever is stored in the vector regardless if it is another vector or scalar type. The behavior is consistent everywhere.
> - Introduced getInnerValueCount and getInnerValueCountAt for values sitting at second level to repeated types.
> - All subclasses implement RepeatedVV interface
> - Common functionality is abstracted out to base classes
> 
> FlattenRB
> - Refactored set flatten field block to make it more readable
> - Fixed a wrong getValueCount call
> 
> ProjectRB
> - Fixed an issue regarding multiple projections not initializing nested vectors.
> 
> ZeroVector
> - Used as initial vector value in repeated vectors instead of a "null" reference
> 
> ContainerVectorLike
> - Common trait/mixin shared by nested vectors.
> - Centralized logic to add vectors to nested types via use of VectorDescriptor
> 
> 
> Diffs
> -----
> 
>   exec/java-exec/src/main/codegen/templates/ComplexReaders.java fa1dac4ecce88e99b4c0b8cfcaec6095c6fed469 
>   exec/java-exec/src/main/codegen/templates/ComplexWriters.java 49c75d132ac015931486c16913c327cc8f69678a 
>   exec/java-exec/src/main/codegen/templates/FixedValueVectors.java a805b8e239d9db2f2d203f5a653919aaaa4a1149 
>   exec/java-exec/src/main/codegen/templates/ListWriters.java 6df4248d1fe527a3c2da86b880a13366f29aa5fb 
>   exec/java-exec/src/main/codegen/templates/MapWriters.java 38df84bf2bc0fd49ce18a115c512ad6098e11976 
>   exec/java-exec/src/main/codegen/templates/RepeatedValueVectors.java c0fba660c734d9c35e577b3c3b8f9b81f3a7f65c 
>   exec/java-exec/src/main/java/org/apache/drill/exec/exception/SchemaChangeRuntimeException.java PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/flatten/FlattenRecordBatch.java 7a5b352b0b1210626cd19a62d63b73afdd756409 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/flatten/FlattenTemplate.java 96209a23b3eb4f61dd9bc98a3dad53dc3938f5bd 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/flatten/Flattener.java 2141ca2c47b19d66dc6cfce3fe088d7299c576ed 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/project/ProjectRecordBatch.java 32ffb6f0b6e29ea3eb095906cdf5e705158924dc 
>   exec/java-exec/src/main/java/org/apache/drill/exec/store/VectorHolder.java 8387d49a61cb50c55863238fd9759daacb928cfc 
>   exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/RepeatedVarCharOutput.java 3ad5c2a36b65e13165947c7a60e9847cbb9a5bf9 
>   exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/FixedWidthRepeatedReader.java 7f8b6117570e21d84c952438807c51a52c900d92 
>   exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/ParquetRecordReader.java 2f07fb3527dbc4e4de25f8af07bba7c1c85d88f8 
>   exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java e25bd74084c25188d20b0cfd91a890fa816ede1d 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/AllocationHelper.java bf465c7994bba8e225ed5ed1fb4402c636e8cf46 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseDataValueVector.java 0c6097c480a4ac0bdc6e37c16f0f7a643d1ca175 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseRepeatedValueVector.java PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseValueVector.java 22f0fe7406139ac0bbe89fa51b1ff60591fb4df8 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/ContainerVectorLike.java PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedFixedWidthVector.java eaae7ad6032f52ea3a2a718563096dc853bc1628 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedValueVector.java PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedVariableWidthVector.java a49934168af1120b75cbc99911551f89bf56ea12 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedVector.java df4279ac4566a55b9451cd2be6a7e57d3bcc4db8 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/SchemaChangeCallBack.java 386ee34954b08f85e0ac2bc0db7e263566eb1ef6 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/ValueVector.java e4a0997ed12585ae27e06daf9bc242d9e1e8f6b6 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/VectorDescriptor.java PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/ZeroVector.java PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/MapVector.java 41388393c217973b9be910c69f15ee8e099f9d22 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedListVector.java c061029991f145751b686a9738a4021ec1cb12f6 
>   exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedMapVector.java e5d48dd6fb07e7bd08b1b06e87ae24d839ffcaf2 
>   exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestValueVector.java 1564aea4d41df3016b6b179162d0a0001bf62ca9 
> 
> Diff: https://reviews.apache.org/r/33925/diff/
> 
> 
> Testing
> -------
> 
> All tests pass.
> 
> 
> Thanks,
> 
> Hanifi Gunes
> 
>

Re: Review Request 33925: DRILL-2150: Create an abstraction for repeated value vectors.

Posted by Hanifi Gunes <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33925/
-----------------------------------------------------------

(Updated May 11, 2015, 6:49 a.m.)


Review request for drill, Mehant Baid and Parth Chandra.


Changes
-------

Addressed Parth's concerns. 
Rebased and resolved conflicts
Introduced values variable to cache inner vector for performance reasons.


Bugs: DRILL-2150
    https://issues.apache.org/jira/browse/DRILL-2150


Repository: drill-git


Description
-------

DRILL-2150: Create an abstraction for repeated value vectors.

All value vectors
- Rely on getMetadataBuilder to iteratively construct vector metadata rather than overriding getMetadata in each vector
- Fixed misc issues enhacing code re-use
- Removed generic template reference to concrete VV type from VV interface.

ComplexWriters
- Fixed an off by one bug

FixedValueVectors
- Fixed a value count bug

RepeatedValueVectors
- Fixed wrong implementation of getValueCount. A value is whatever is stored in the vector regardless if it is another vector or scalar type. The behavior is consistent everywhere.
- Introduced getInnerValueCount and getInnerValueCountAt for values sitting at second level to repeated types.
- All subclasses implement RepeatedVV interface
- Common functionality is abstracted out to base classes

FlattenRB
- Refactored set flatten field block to make it more readable
- Fixed a wrong getValueCount call

ProjectRB
- Fixed an issue regarding multiple projections not initializing nested vectors.

ZeroVector
- Used as initial vector value in repeated vectors instead of a "null" reference

ContainerVectorLike
- Common trait/mixin shared by nested vectors.
- Centralized logic to add vectors to nested types via use of VectorDescriptor


Diffs (updated)
-----

  exec/java-exec/src/main/codegen/templates/ComplexReaders.java fa1dac4ecce88e99b4c0b8cfcaec6095c6fed469 
  exec/java-exec/src/main/codegen/templates/ComplexWriters.java 49c75d132ac015931486c16913c327cc8f69678a 
  exec/java-exec/src/main/codegen/templates/FixedValueVectors.java a805b8e239d9db2f2d203f5a653919aaaa4a1149 
  exec/java-exec/src/main/codegen/templates/ListWriters.java 6df4248d1fe527a3c2da86b880a13366f29aa5fb 
  exec/java-exec/src/main/codegen/templates/MapWriters.java 38df84bf2bc0fd49ce18a115c512ad6098e11976 
  exec/java-exec/src/main/codegen/templates/RepeatedValueVectors.java c0fba660c734d9c35e577b3c3b8f9b81f3a7f65c 
  exec/java-exec/src/main/java/org/apache/drill/exec/exception/SchemaChangeRuntimeException.java PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/flatten/FlattenRecordBatch.java 7a5b352b0b1210626cd19a62d63b73afdd756409 
  exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/flatten/FlattenTemplate.java 96209a23b3eb4f61dd9bc98a3dad53dc3938f5bd 
  exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/flatten/Flattener.java 2141ca2c47b19d66dc6cfce3fe088d7299c576ed 
  exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/project/ProjectRecordBatch.java 32ffb6f0b6e29ea3eb095906cdf5e705158924dc 
  exec/java-exec/src/main/java/org/apache/drill/exec/store/VectorHolder.java 8387d49a61cb50c55863238fd9759daacb928cfc 
  exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/RepeatedVarCharOutput.java 3ad5c2a36b65e13165947c7a60e9847cbb9a5bf9 
  exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/FixedWidthRepeatedReader.java 7f8b6117570e21d84c952438807c51a52c900d92 
  exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/ParquetRecordReader.java 2f07fb3527dbc4e4de25f8af07bba7c1c85d88f8 
  exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java e25bd74084c25188d20b0cfd91a890fa816ede1d 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/AllocationHelper.java bf465c7994bba8e225ed5ed1fb4402c636e8cf46 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseDataValueVector.java 0c6097c480a4ac0bdc6e37c16f0f7a643d1ca175 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseRepeatedValueVector.java PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/BaseValueVector.java 22f0fe7406139ac0bbe89fa51b1ff60591fb4df8 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/ContainerVectorLike.java PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedFixedWidthVector.java eaae7ad6032f52ea3a2a718563096dc853bc1628 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedValueVector.java PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedVariableWidthVector.java a49934168af1120b75cbc99911551f89bf56ea12 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedVector.java df4279ac4566a55b9451cd2be6a7e57d3bcc4db8 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/SchemaChangeCallBack.java 386ee34954b08f85e0ac2bc0db7e263566eb1ef6 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/ValueVector.java e4a0997ed12585ae27e06daf9bc242d9e1e8f6b6 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/VectorDescriptor.java PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/ZeroVector.java PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/MapVector.java 41388393c217973b9be910c69f15ee8e099f9d22 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedListVector.java c061029991f145751b686a9738a4021ec1cb12f6 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedMapVector.java e5d48dd6fb07e7bd08b1b06e87ae24d839ffcaf2 
  exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestValueVector.java 1564aea4d41df3016b6b179162d0a0001bf62ca9 

Diff: https://reviews.apache.org/r/33925/diff/


Testing
-------

All tests pass.


Thanks,

Hanifi Gunes