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: Adding JSONRecordReader
Date Mon, 03 Jun 2013 16:01:33 GMT

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


In general, can you give a quick explanation of your design?  I'm not entirely clear at how
the various schemas relate to all the other data types and schemas available in the code base.
 Clearly, some stuff should be specific.  Other stuff should probably be shared across various
record readers.


sandbox/prototype/common/src/main/java/org/apache/drill/common/physical/schema/Field.java
<https://reviews.apache.org/r/11587/#comment44220>

    It is not clear to me why we need this.  Can't we use the MajorType/MinorType stuff? 
Can you explain why we have this as well?



sandbox/prototype/common/src/main/java/org/apache/drill/common/physical/schema/IdGenerator.java
<https://reviews.apache.org/r/11587/#comment44154>

    remove



sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/record/vector/BaseValueVector.java
<https://reviews.apache.org/r/11587/#comment44217>

    You should never pass a dead byte buf into this method.



sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/record/vector/NullableVarLen4.java
<https://reviews.apache.org/r/11587/#comment44221>

    Why are you overriding?  Isn't this exactly what the above method does?



sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/record/vector/NullableVarLen4.java
<https://reviews.apache.org/r/11587/#comment44222>

    Does this method (here in super class) also set the value to not null?



sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/record/vector/TypeHelper.java
<https://reviews.apache.org/r/11587/#comment44218>

    You shouldn't modify this block.  This block is for REQUIRED types.  For nullable, you
should use the second block below that is commented out.  You can just uncomment the single
NullableFixed4 value.



sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/record/vector/TypeHelper.java
<https://reviews.apache.org/r/11587/#comment44219>

    Same as issue above.



sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/Field.java
<https://reviews.apache.org/r/11587/#comment44223>

    DataMode should OPTIONAL for Nullable fields.



sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java
<https://reviews.apache.org/r/11587/#comment44224>

    Why did you remove these?



sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/store/JSONRecordReader.java
<https://reviews.apache.org/r/11587/#comment44225>

    Ideally, we would maintain non-changing vectors across batches rather than recreating
each time.  This is fine for now, though



sandbox/prototype/pom.xml
<https://reviews.apache.org/r/11587/#comment44153>

    you need to add <scope>test</scope>


- Jacques Nadeau


On May 31, 2013, 11:47 p.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11587/
> -----------------------------------------------------------
> 
> (Updated May 31, 2013, 11:47 p.m.)
> 
> 
> Review request for drill and Jacques Nadeau.
> 
> 
> Description
> -------
> 
> Added the JSONRecordReader based on the previous ScanJson work.
>  Does not support nested fields, maps or lists yet.
>  Currently it detects to move on to the next batch when any of the field batch cannot
hold another item for the current item being written. This also assumes the default batch
size can always hold at least one item from any field (which only is a problem for variable
length vectors).
> 
> 
> Diffs
> -----
> 
>   sandbox/prototype/common/src/main/java/org/apache/drill/common/physical/schema/DiffSchema.java
PRE-CREATION 
>   sandbox/prototype/common/src/main/java/org/apache/drill/common/physical/schema/Field.java
PRE-CREATION 
>   sandbox/prototype/common/src/main/java/org/apache/drill/common/physical/schema/IdGenerator.java
PRE-CREATION 
>   sandbox/prototype/common/src/main/java/org/apache/drill/common/physical/schema/ListSchema.java
PRE-CREATION 
>   sandbox/prototype/common/src/main/java/org/apache/drill/common/physical/schema/NamedField.java
PRE-CREATION 
>   sandbox/prototype/common/src/main/java/org/apache/drill/common/physical/schema/ObjectSchema.java
PRE-CREATION 
>   sandbox/prototype/common/src/main/java/org/apache/drill/common/physical/schema/OrderedField.java
PRE-CREATION 
>   sandbox/prototype/common/src/main/java/org/apache/drill/common/physical/schema/RecordSchema.java
PRE-CREATION 
>   sandbox/prototype/common/src/main/java/org/apache/drill/common/physical/schema/SchemaIdGenerator.java
PRE-CREATION 
>   sandbox/prototype/common/src/main/java/org/apache/drill/common/physical/schema/json/jackson/JacksonHelper.java
PRE-CREATION 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/record/DeadBuf.java
dafb68c 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/record/vector/AbstractFixedValueVector.java
b32f067 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/record/vector/BaseValueVector.java
b001add 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/record/vector/NullableFixed4.java
cc18538 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/record/vector/NullableValueVector.java
692ab87 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/record/vector/NullableVarLen4.java
PRE-CREATION 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/record/vector/TypeHelper.java
8e89c41 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/record/vector/VarLen1.java
d87029d 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/record/vector/VarLen2.java
ebd440a 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/record/vector/VarLen4.java
b3cd712 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/record/vector/VariableVector.java
4247f14 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/DiffSchema.java
016e097 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/Field.java
e19c099 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/NamedField.java
aa0d6aa 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/OrderedField.java
67fd2fa 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/json/jackson/JacksonHelper.java
0643710 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/json/jackson/PhysicalOperator.java
e450ee9 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/json/jackson/PhysicalOperatorIterator.java
bf4053e 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/json/jackson/ScanJson.java
a1c30e9 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java
d5aaab2 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/store/BatchExceededException.java
PRE-CREATION 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/store/JSONRecordReader.java
PRE-CREATION 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/store/RecordReader.java
67c84ed 
>   sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/store/VectorHolder.java
PRE-CREATION 
>   sandbox/prototype/exec/java-exec/src/test/java/org/apache/drill/exec/store/JSONRecordReaderTest.java
PRE-CREATION 
>   sandbox/prototype/exec/java-exec/src/test/resources/scan_json_test_1.json PRE-CREATION

>   sandbox/prototype/exec/java-exec/src/test/resources/scan_json_test_2.json PRE-CREATION

>   sandbox/prototype/exec/java-exec/src/test/resources/scan_json_test_3.json PRE-CREATION

>   sandbox/prototype/exec/java-exec/src/test/resources/scan_json_test_4.json PRE-CREATION

>   sandbox/prototype/exec/java-exec/src/test/resources/scan_json_test_5.json PRE-CREATION

>   sandbox/prototype/exec/java-exec/src/test/resources/scan_json_test_6.json PRE-CREATION

>   sandbox/prototype/pom.xml 25f156d 
> 
> Diff: https://reviews.apache.org/r/11587/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>


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