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