> 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 > >
