drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jacques Nadeau" <jacques.dr...@gmail.com>
Subject Re: Review Request 29519: DRILL-1885: fix a problem regarding ordinal to vector mapping that report incorrect result or fails a query
Date Tue, 06 Jan 2015 04:32:22 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message