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 <hg...@maprtech.com> on 2015/01/01 00:12:21 UTC
Review Request 29519: DRILL-1885: fix a problem regarding ordinal to
vector mapping that report incorrect result or fails a query
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29519/
-----------------------------------------------------------
Review request for drill, Jacques Nadeau, Parth Chandra, and Steven Phillips.
Bugs: DRILL-1885
https://issues.apache.org/jira/browse/DRILL-1885
Repository: drill-git
Description
-------
fix a problem regarding ordinal to vector mapping that report incorrect result or fails a query.
fix failing unittest
refactor code, eliminate redundancy
Diffs
-----
common/src/main/java/org/apache/drill/common/collections/MapWithOrdinal.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java 23833b6
exec/java-exec/src/main/java/org/apache/drill/exec/record/VectorContainer.java d50760a
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractContainerVector.java 1210d90
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/MapVector.java cc3d24c
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedListVector.java 362d806
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedMapVector.java e140c8b
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/VectorWithOrdinal.java PRE-CREATION
exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestParquetComplex.java 8405d0e
exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/fn/TestJsonReaderWithSparseFiles.java 7e4cf4b
Diff: https://reviews.apache.org/r/29519/diff/
Testing
-------
unit tests + all test suites pass.
Thanks,
Hanifi Gunes
Re: Review Request 29519: DRILL-1885: fix a problem regarding ordinal
to vector mapping that report incorrect result or fails a query
Posted by Hanifi Gunes <hg...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29519/
-----------------------------------------------------------
(Updated Jan. 12, 2015, 11:06 p.m.)
Review request for drill, Jacques Nadeau, Parth Chandra, and Steven Phillips.
Changes
-------
addressed the comments + refactored code to make it more robust and readable.
Bugs: DRILL-1885
https://issues.apache.org/jira/browse/DRILL-1885
Repository: drill-git
Description
-------
fix a problem regarding ordinal to vector mapping that report incorrect result or fails a query.
fix failing unittest
refactor code, eliminate redundancy
Diffs (updated)
-----
common/src/main/java/org/apache/drill/common/collections/MapWithOrdinal.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java 23833b6
exec/java-exec/src/main/java/org/apache/drill/exec/record/VectorContainer.java d50760a
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractContainerVector.java 1210d90
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractMapVector.java f126e5c
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/MapVector.java cc3d24c
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedListVector.java 362d806
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedMapVector.java e140c8b
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/VectorWithOrdinal.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/impl/ComplexWriterImpl.java 18b5e9e
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/impl/RepeatedListReaderImpl.java c60730c
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/impl/RepeatedMapReaderImpl.java 15f8a2b
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/impl/SingleListReaderImpl.java c2284ec
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/impl/SingleMapReaderImpl.java 3ec66ff
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/impl/VectorContainerWriter.java 36184a7
exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestParquetComplex.java 8405d0e
exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/fn/TestJsonReaderWithSparseFiles.java 7e4cf4b
exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/writer/TestRepeated.java 3f125fa
Diff: https://reviews.apache.org/r/29519/diff/
Testing (updated)
-------
all test suites.
Thanks,
Hanifi Gunes
Re: Review Request 29519: DRILL-1885: fix a problem regarding ordinal
to vector mapping that report incorrect result or fails a query
Posted by Hanifi Gunes <hg...@maprtech.com>.
> On Jan. 6, 2015, 4:32 a.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/record/VectorContainer.java, line 89
> > <https://reviews.apache.org/r/29519/diff/1/?file=804906#file804906line89>
> >
> > Shouldn't these also be refactored out and shared with the other methods?
This patch works around AbstractContainerVector and its subclasses. You are right that VectorContainer also needs some refactoring. I'd like to take a look at this on a sepearate issue though.
> On Jan. 6, 2015, 4:32 a.m., Jacques Nadeau wrote:
> > common/src/main/java/org/apache/drill/common/collections/MapWithOrdinal.java, line 33
> > <https://reviews.apache.org/r/29519/diff/1/?file=804904#file804904line33>
> >
> > We need javadocs here to describe behaviors
done.
> On Jan. 6, 2015, 4:32 a.m., Jacques Nadeau wrote:
> > common/src/main/java/org/apache/drill/common/collections/MapWithOrdinal.java, line 85
> > <https://reviews.apache.org/r/29519/diff/1/?file=804904#file804904line85>
> >
> > this seems like it will cause weird behavior and unexpected changes in column ordering. it seems like we should have more linkedlist like behavior where each node gets and ordinal from the previous element in the list.
this state is meant to be internal. other pieces of code that used to rely on "implicit" ordering of vectors are now eliminated. ordinal assignment mechanizm is documented as well. should not be a problem.
> On Jan. 6, 2015, 4:32 a.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractContainerVector.java, line 75
> > <https://reviews.apache.org/r/29519/diff/1/?file=804907#file804907line75>
> >
> > You need to add Javadocs on these methods. You've made them behave in a clear way but you should doc so others don't have to review code to understand behavior.
done.
> On Jan. 6, 2015, 4:32 a.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractContainerVector.java, line 114
> > <https://reviews.apache.org/r/29519/diff/1/?file=804907#file804907line114>
> >
> > same as above. e.g. what happens on name conflict.
commented.
> On Jan. 6, 2015, 4:32 a.m., Jacques Nadeau wrote:
> > exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/fn/TestJsonReaderWithSparseFiles.java, line 123
> > <https://reviews.apache.org/r/29519/diff/1/?file=804913#file804913line123>
> >
> > why did this change?
earlier impl didn't conform to field ordering in the schema due to the fact that vectors were assumed to be in order implicitly. now i do read list of fields from schema. so this test case got reverted to normal.
- Hanifi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29519/#review66783
-----------------------------------------------------------
On Jan. 12, 2015, 11:06 p.m., Hanifi Gunes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29519/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2015, 11:06 p.m.)
>
>
> Review request for drill, Jacques Nadeau, Parth Chandra, and Steven Phillips.
>
>
> Bugs: DRILL-1885
> https://issues.apache.org/jira/browse/DRILL-1885
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> fix a problem regarding ordinal to vector mapping that report incorrect result or fails a query.
> fix failing unittest
> refactor code, eliminate redundancy
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/collections/MapWithOrdinal.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java 23833b6
> exec/java-exec/src/main/java/org/apache/drill/exec/record/VectorContainer.java d50760a
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractContainerVector.java 1210d90
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractMapVector.java f126e5c
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/MapVector.java cc3d24c
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedListVector.java 362d806
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedMapVector.java e140c8b
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/VectorWithOrdinal.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/impl/ComplexWriterImpl.java 18b5e9e
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/impl/RepeatedListReaderImpl.java c60730c
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/impl/RepeatedMapReaderImpl.java 15f8a2b
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/impl/SingleListReaderImpl.java c2284ec
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/impl/SingleMapReaderImpl.java 3ec66ff
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/impl/VectorContainerWriter.java 36184a7
> exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestParquetComplex.java 8405d0e
> exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/fn/TestJsonReaderWithSparseFiles.java 7e4cf4b
> exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/writer/TestRepeated.java 3f125fa
>
> Diff: https://reviews.apache.org/r/29519/diff/
>
>
> Testing
> -------
>
> all test suites.
>
>
> Thanks,
>
> Hanifi Gunes
>
>
Re: Review Request 29519: DRILL-1885: fix a problem regarding ordinal
to vector mapping that report incorrect result or fails a query
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29519/#review66783
-----------------------------------------------------------
First pass thoughts. Will do another review in the morning.
common/src/main/java/org/apache/drill/common/collections/MapWithOrdinal.java
<https://reviews.apache.org/r/29519/#comment110404>
We need javadocs here to describe behaviors
common/src/main/java/org/apache/drill/common/collections/MapWithOrdinal.java
<https://reviews.apache.org/r/29519/#comment110403>
this seems like it will cause weird behavior and unexpected changes in column ordering. it seems like we should have more linkedlist like behavior where each node gets and ordinal from the previous element in the list.
exec/java-exec/src/main/java/org/apache/drill/exec/record/VectorContainer.java
<https://reviews.apache.org/r/29519/#comment110399>
Shouldn't these also be refactored out and shared with the other methods?
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractContainerVector.java
<https://reviews.apache.org/r/29519/#comment110400>
You need to add Javadocs on these methods. You've made them behave in a clear way but you should doc so others don't have to review code to understand behavior.
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractContainerVector.java
<https://reviews.apache.org/r/29519/#comment110401>
same as above. e.g. what happens on name conflict.
exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/fn/TestJsonReaderWithSparseFiles.java
<https://reviews.apache.org/r/29519/#comment110405>
why did this change?
- Jacques Nadeau
On Dec. 31, 2014, 11:12 p.m., Hanifi Gunes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29519/
> -----------------------------------------------------------
>
> (Updated Dec. 31, 2014, 11:12 p.m.)
>
>
> Review request for drill, Jacques Nadeau, Parth Chandra, and Steven Phillips.
>
>
> Bugs: DRILL-1885
> https://issues.apache.org/jira/browse/DRILL-1885
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> fix a problem regarding ordinal to vector mapping that report incorrect result or fails a query.
> fix failing unittest
> refactor code, eliminate redundancy
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/collections/MapWithOrdinal.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java 23833b6
> exec/java-exec/src/main/java/org/apache/drill/exec/record/VectorContainer.java d50760a
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractContainerVector.java 1210d90
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/MapVector.java cc3d24c
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedListVector.java 362d806
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/RepeatedMapVector.java e140c8b
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/VectorWithOrdinal.java PRE-CREATION
> exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestParquetComplex.java 8405d0e
> exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/fn/TestJsonReaderWithSparseFiles.java 7e4cf4b
>
> Diff: https://reviews.apache.org/r/29519/diff/
>
>
> Testing
> -------
>
> unit tests + all test suites pass.
>
>
> Thanks,
>
> Hanifi Gunes
>
>