You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Timothy Chen <tn...@gmail.com> on 2013/08/12 07:00:49 UTC
Review Request 13489: JsonRecordReader changes and working e2e
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13489/
-----------------------------------------------------------
Review request for drill and Jacques Nadeau.
Repository: drill-git
Description
-------
- Added JsonScanBatch and POP
- Added Repeated support for JsonRecordReader
- Support Late field type binding
- JsonRecordReader working e2e!
Diffs
-----
sandbox/prototype/common/src/main/java/org/apache/drill/common/expression/SchemaPath.java 19d1069
sandbox/prototype/common/src/main/java/org/apache/drill/common/types/Types.java e81bc89
sandbox/prototype/exec/java-exec/src/main/codegen/ValueVectors/templates/NullableValueVectors.java ca222df
sandbox/prototype/exec/java-exec/src/main/codegen/ValueVectors/templates/RepeatedValueVectors.java 1afe84b
sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/JSONScanBatchCreator.java PRE-CREATION
sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/JSONScanPOP.java PRE-CREATION
sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java c31e9e4
sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/DiffSchema.java b654a92
sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/Field.java 85bbdf3
sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/json/jackson/JacksonHelper.java 0e2c052
sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/store/JSONRecordReader.java f72b519
sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/store/VectorHolder.java d594b9e
sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/vector/AllocationHelper.java 69c17f4
sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/vector/FixedWidthVector.java 17e072b
sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedMutator.java PRE-CREATION
sandbox/prototype/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestSimpleFragmentRun.java e21289c
sandbox/prototype/exec/java-exec/src/test/java/org/apache/drill/exec/store/JSONRecordReaderTest.java 0ebb529
sandbox/prototype/exec/java-exec/src/test/resources/physical_json_scan_test1.json PRE-CREATION
sandbox/prototype/exec/java-exec/src/test/resources/scan_json_test_4.json 0fb3202
sandbox/prototype/exec/java-exec/src/test/resources/scan_json_test_5.json ae1aaf2
Diff: https://reviews.apache.org/r/13489/diff/
Testing
-------
Thanks,
Timothy Chen
Re: Review Request 13489: JsonRecordReader changes and working e2e
Posted by Timothy Chen <tn...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13489/#review25121
-----------------------------------------------------------
sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/store/JSONRecordReader.java
<https://reviews.apache.org/r/13489/#comment49361>
It's actually a valid flow that the token will attempt to create a value vector, but I don't allow that. I allow it to create a field since I want to track the time it was first encountered.
sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/store/VectorHolder.java
<https://reviews.apache.org/r/13489/#comment49362>
The count is just keeping count the number of values added.
I would love so if I don't need to keep track of this, but the vv api requires the reader to populate the total number of values at the end of the batch.
It's possible to consolidate this if we some how get vv to use its writerIndex to populate valueCount, but this is ofcourse assuming they're writing serially into the batch.
- Timothy Chen
On Aug. 12, 2013, 5 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13489/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2013, 5 a.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - Added JsonScanBatch and POP
> - Added Repeated support for JsonRecordReader
> - Support Late field type binding
> - JsonRecordReader working e2e!
>
>
> Diffs
> -----
>
> sandbox/prototype/common/src/main/java/org/apache/drill/common/expression/SchemaPath.java 19d1069
> sandbox/prototype/common/src/main/java/org/apache/drill/common/types/Types.java e81bc89
> sandbox/prototype/exec/java-exec/src/main/codegen/ValueVectors/templates/NullableValueVectors.java ca222df
> sandbox/prototype/exec/java-exec/src/main/codegen/ValueVectors/templates/RepeatedValueVectors.java 1afe84b
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/JSONScanBatchCreator.java PRE-CREATION
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/JSONScanPOP.java PRE-CREATION
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java c31e9e4
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/DiffSchema.java b654a92
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/Field.java 85bbdf3
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/json/jackson/JacksonHelper.java 0e2c052
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/store/JSONRecordReader.java f72b519
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/store/VectorHolder.java d594b9e
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/vector/AllocationHelper.java 69c17f4
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/vector/FixedWidthVector.java 17e072b
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedMutator.java PRE-CREATION
> sandbox/prototype/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestSimpleFragmentRun.java e21289c
> sandbox/prototype/exec/java-exec/src/test/java/org/apache/drill/exec/store/JSONRecordReaderTest.java 0ebb529
> sandbox/prototype/exec/java-exec/src/test/resources/physical_json_scan_test1.json PRE-CREATION
> sandbox/prototype/exec/java-exec/src/test/resources/scan_json_test_4.json 0fb3202
> sandbox/prototype/exec/java-exec/src/test/resources/scan_json_test_5.json ae1aaf2
>
> Diff: https://reviews.apache.org/r/13489/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 13489: JsonRecordReader changes and working e2e
Posted by Timothy Chen <tn...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13489/#review24980
-----------------------------------------------------------
sandbox/prototype/exec/java-exec/src/main/codegen/ValueVectors/templates/RepeatedValueVectors.java
<https://reviews.apache.org/r/13489/#comment49147>
The old codegen block doesn't generate correctly for float types.
- Timothy Chen
On Aug. 12, 2013, 5 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13489/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2013, 5 a.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - Added JsonScanBatch and POP
> - Added Repeated support for JsonRecordReader
> - Support Late field type binding
> - JsonRecordReader working e2e!
>
>
> Diffs
> -----
>
> sandbox/prototype/common/src/main/java/org/apache/drill/common/expression/SchemaPath.java 19d1069
> sandbox/prototype/common/src/main/java/org/apache/drill/common/types/Types.java e81bc89
> sandbox/prototype/exec/java-exec/src/main/codegen/ValueVectors/templates/NullableValueVectors.java ca222df
> sandbox/prototype/exec/java-exec/src/main/codegen/ValueVectors/templates/RepeatedValueVectors.java 1afe84b
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/JSONScanBatchCreator.java PRE-CREATION
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/JSONScanPOP.java PRE-CREATION
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java c31e9e4
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/DiffSchema.java b654a92
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/Field.java 85bbdf3
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/json/jackson/JacksonHelper.java 0e2c052
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/store/JSONRecordReader.java f72b519
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/store/VectorHolder.java d594b9e
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/vector/AllocationHelper.java 69c17f4
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/vector/FixedWidthVector.java 17e072b
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedMutator.java PRE-CREATION
> sandbox/prototype/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestSimpleFragmentRun.java e21289c
> sandbox/prototype/exec/java-exec/src/test/java/org/apache/drill/exec/store/JSONRecordReaderTest.java 0ebb529
> sandbox/prototype/exec/java-exec/src/test/resources/physical_json_scan_test1.json PRE-CREATION
> sandbox/prototype/exec/java-exec/src/test/resources/scan_json_test_4.json 0fb3202
> sandbox/prototype/exec/java-exec/src/test/resources/scan_json_test_5.json ae1aaf2
>
> Diff: https://reviews.apache.org/r/13489/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 13489: JsonRecordReader changes and working e2e
Posted by Ben Becker <bb...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13489/#review25052
-----------------------------------------------------------
sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/DiffSchema.java
<https://reviews.apache.org/r/13489/#comment49228>
nit: could this just be called hasChanged()?
sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/store/JSONRecordReader.java
<https://reviews.apache.org/r/13489/#comment49238>
The calls to incAndCheckLength(n + 1) seem a bit cumbersome. Given that the derived value vector class knows more about the space required for a record than the VectorHolder, would it make sense for the VectorHolder to rely on the valuevector to calculate the size?
sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/store/JSONRecordReader.java
<https://reviews.apache.org/r/13489/#comment49239>
If MAP and LATE are unsupported, perhaps this could throw a fragment-fatal exception, or at least log a message?
sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/store/VectorHolder.java
<https://reviews.apache.org/r/13489/#comment49242>
What exactly is 'count' counting; number of records or number of values?
Could it be consolidated into the VV's recordCount (thus allowing us to remove populateVectorLength())?
sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/store/VectorHolder.java
<https://reviews.apache.org/r/13489/#comment49241>
mutator.startNewGroup(++this.groupCount);
- Ben Becker
On Aug. 12, 2013, 5 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13489/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2013, 5 a.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - Added JsonScanBatch and POP
> - Added Repeated support for JsonRecordReader
> - Support Late field type binding
> - JsonRecordReader working e2e!
>
>
> Diffs
> -----
>
> sandbox/prototype/common/src/main/java/org/apache/drill/common/expression/SchemaPath.java 19d1069
> sandbox/prototype/common/src/main/java/org/apache/drill/common/types/Types.java e81bc89
> sandbox/prototype/exec/java-exec/src/main/codegen/ValueVectors/templates/NullableValueVectors.java ca222df
> sandbox/prototype/exec/java-exec/src/main/codegen/ValueVectors/templates/RepeatedValueVectors.java 1afe84b
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/JSONScanBatchCreator.java PRE-CREATION
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/JSONScanPOP.java PRE-CREATION
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java c31e9e4
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/DiffSchema.java b654a92
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/Field.java 85bbdf3
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/json/jackson/JacksonHelper.java 0e2c052
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/store/JSONRecordReader.java f72b519
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/store/VectorHolder.java d594b9e
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/vector/AllocationHelper.java 69c17f4
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/vector/FixedWidthVector.java 17e072b
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedMutator.java PRE-CREATION
> sandbox/prototype/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestSimpleFragmentRun.java e21289c
> sandbox/prototype/exec/java-exec/src/test/java/org/apache/drill/exec/store/JSONRecordReaderTest.java 0ebb529
> sandbox/prototype/exec/java-exec/src/test/resources/physical_json_scan_test1.json PRE-CREATION
> sandbox/prototype/exec/java-exec/src/test/resources/scan_json_test_4.json 0fb3202
> sandbox/prototype/exec/java-exec/src/test/resources/scan_json_test_5.json ae1aaf2
>
> Diff: https://reviews.apache.org/r/13489/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 13489: JsonRecordReader changes and working e2e
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13489/#review25082
-----------------------------------------------------------
Ship it!
sandbox/prototype/exec/java-exec/src/main/codegen/ValueVectors/templates/NullableValueVectors.java
<https://reviews.apache.org/r/13489/#comment49262>
We can probably just do concat(bits.data, values.getBuffers()) so that we don't have to fork the generation here.
- Jacques Nadeau
On Aug. 12, 2013, 5 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13489/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2013, 5 a.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - Added JsonScanBatch and POP
> - Added Repeated support for JsonRecordReader
> - Support Late field type binding
> - JsonRecordReader working e2e!
>
>
> Diffs
> -----
>
> sandbox/prototype/common/src/main/java/org/apache/drill/common/expression/SchemaPath.java 19d1069
> sandbox/prototype/common/src/main/java/org/apache/drill/common/types/Types.java e81bc89
> sandbox/prototype/exec/java-exec/src/main/codegen/ValueVectors/templates/NullableValueVectors.java ca222df
> sandbox/prototype/exec/java-exec/src/main/codegen/ValueVectors/templates/RepeatedValueVectors.java 1afe84b
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/JSONScanBatchCreator.java PRE-CREATION
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/JSONScanPOP.java PRE-CREATION
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java c31e9e4
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/DiffSchema.java b654a92
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/Field.java 85bbdf3
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/schema/json/jackson/JacksonHelper.java 0e2c052
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/store/JSONRecordReader.java f72b519
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/store/VectorHolder.java d594b9e
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/vector/AllocationHelper.java 69c17f4
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/vector/FixedWidthVector.java 17e072b
> sandbox/prototype/exec/java-exec/src/main/java/org/apache/drill/exec/vector/RepeatedMutator.java PRE-CREATION
> sandbox/prototype/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestSimpleFragmentRun.java e21289c
> sandbox/prototype/exec/java-exec/src/test/java/org/apache/drill/exec/store/JSONRecordReaderTest.java 0ebb529
> sandbox/prototype/exec/java-exec/src/test/resources/physical_json_scan_test1.json PRE-CREATION
> sandbox/prototype/exec/java-exec/src/test/resources/scan_json_test_4.json 0fb3202
> sandbox/prototype/exec/java-exec/src/test/resources/scan_json_test_5.json ae1aaf2
>
> Diff: https://reviews.apache.org/r/13489/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>