You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Jason Dere <jd...@hortonworks.com> on 2017/05/02 21:52:40 UTC
Review Request 58934: HIVE-16568: Support complex types in external
LLAP InputFormat
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58934/
-----------------------------------------------------------
Review request for hive, Gunther Hagleitner, Prasanth_J, and Siddharth Seth.
Bugs: HIVE-16568
https://issues.apache.org/jira/browse/HIVE-16568
Repository: hive-git
Description
-------
- Support list/map/struct types in the LLAPRowInputFormat Schema/TypeDesc
- Support list/map/struct types in the LLAPRowInputFormat Row. Changes in the Row getters/setters needed (no longer using Writable).
Diffs
-----
itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniLlap.java de47412
llap-client/src/java/org/apache/hadoop/hive/llap/LlapRowRecordReader.java ee92f3e
llap-common/src/java/org/apache/hadoop/hive/llap/Row.java a84fadc
llap-common/src/java/org/apache/hadoop/hive/llap/TypeDesc.java dda5928
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDTFGetSplits.java 9ddbd7e
Diff: https://reviews.apache.org/r/58934/diff/1/
Testing
-------
Added test to TestJdbcWithMiniLlap
Thanks,
Jason Dere
Re: Review Request 58934: HIVE-16568: Support complex types in
external LLAP InputFormat
Posted by Jason Dere <jd...@hortonworks.com>.
> On May 3, 2017, 5:34 a.m., Prasanth_J wrote:
> > llap-client/src/java/org/apache/hadoop/hive/llap/LlapRowRecordReader.java
> > Lines 154 (patched)
> > <https://reviews.apache.org/r/58934/diff/1/?file=1706163#file1706163line174>
> >
> > IIRC, there are utilities already to do this in ObjectInspectorUtils.. copyToXXX() methods.. can that be reused?
>
> Jason Dere wrote:
> I had not seen the methods in ObjectInspectorUtils, yeah we might be able to use this.
copyToXXX() ends up saving decimal/char/varchar values to their Hive equivalents (HiveDecimal/HiveChar/HiveVarchar). I'd prefer the row interface expose these values as non-Hive classes (BigDecimal/String/String) for ease of use. So I think it is better off not using the ObjectInspecterUtils.copyToXXX methods.
> On May 3, 2017, 5:34 a.m., Prasanth_J wrote:
> > llap-common/src/java/org/apache/hadoop/hive/llap/TypeDesc.java
> > Lines 154 (patched)
> > <https://reviews.apache.org/r/58934/diff/1/?file=1706165#file1706165line154>
> >
> > This also looks repetitive. TypeInfoUtils already has something like this I guess. We need to make sure TypeInfo parser can parse the string generated by this method. It's easier to reuse TypeInfoUtils or have a TypeDesc converted to TypeInfo.
> >
> > My point there is duplicacy in
> > TypeInfo
> > TypeDesc
> > TypeDescriptor (ORC has this)
> >
> > Wondering if TypeInfo or TypeDescriptor from ORC can be reused here. Thoughts?
>
> Jason Dere wrote:
> I had orginally created TypeDesc to try to avoid extra dependencies on what might be considered internal Hive types, but it is true that we are still the serde lib and TypeInfo is available from there. At this point though the TypeDesc has already been created, I'm not totally sure if I want to now go and remove this. If you feel strongly about this let me know.
>
> Prasanth_J wrote:
> May be this can be done in a follow up if it involves a lot of surgery.
> If removing hive dependency is desired or required, then there are 2 possibilities (Make TypeDescriptor in ORC a separate module or make TypeInfo in hive as separate module). But at the current state it looks like there is dependecy with hive serde and typeinfo, so it might be easier to reuse code.
> This way there won't be problem with maintaining compatibility with hive types when new one gets added.
I've tried to update the patch to remove TypeDesc and use TypeInfo instead.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58934/#review173697
-----------------------------------------------------------
On May 2, 2017, 9:52 p.m., Jason Dere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58934/
> -----------------------------------------------------------
>
> (Updated May 2, 2017, 9:52 p.m.)
>
>
> Review request for hive, Gunther Hagleitner, Prasanth_J, and Siddharth Seth.
>
>
> Bugs: HIVE-16568
> https://issues.apache.org/jira/browse/HIVE-16568
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> - Support list/map/struct types in the LLAPRowInputFormat Schema/TypeDesc
> - Support list/map/struct types in the LLAPRowInputFormat Row. Changes in the Row getters/setters needed (no longer using Writable).
>
>
> Diffs
> -----
>
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniLlap.java de47412
> llap-client/src/java/org/apache/hadoop/hive/llap/LlapRowRecordReader.java ee92f3e
> llap-common/src/java/org/apache/hadoop/hive/llap/Row.java a84fadc
> llap-common/src/java/org/apache/hadoop/hive/llap/TypeDesc.java dda5928
> ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDTFGetSplits.java 9ddbd7e
>
>
> Diff: https://reviews.apache.org/r/58934/diff/1/
>
>
> Testing
> -------
>
> Added test to TestJdbcWithMiniLlap
>
>
> Thanks,
>
> Jason Dere
>
>
Re: Review Request 58934: HIVE-16568: Support complex types in
external LLAP InputFormat
Posted by j....@gmail.com.
> On May 3, 2017, 5:34 a.m., Prasanth_J wrote:
> > llap-common/src/java/org/apache/hadoop/hive/llap/TypeDesc.java
> > Lines 154 (patched)
> > <https://reviews.apache.org/r/58934/diff/1/?file=1706165#file1706165line154>
> >
> > This also looks repetitive. TypeInfoUtils already has something like this I guess. We need to make sure TypeInfo parser can parse the string generated by this method. It's easier to reuse TypeInfoUtils or have a TypeDesc converted to TypeInfo.
> >
> > My point there is duplicacy in
> > TypeInfo
> > TypeDesc
> > TypeDescriptor (ORC has this)
> >
> > Wondering if TypeInfo or TypeDescriptor from ORC can be reused here. Thoughts?
>
> Jason Dere wrote:
> I had orginally created TypeDesc to try to avoid extra dependencies on what might be considered internal Hive types, but it is true that we are still the serde lib and TypeInfo is available from there. At this point though the TypeDesc has already been created, I'm not totally sure if I want to now go and remove this. If you feel strongly about this let me know.
May be this can be done in a follow up if it involves a lot of surgery.
If removing hive dependency is desired or required, then there are 2 possibilities (Make TypeDescriptor in ORC a separate module or make TypeInfo in hive as separate module). But at the current state it looks like there is dependecy with hive serde and typeinfo, so it might be easier to reuse code.
This way there won't be problem with maintaining compatibility with hive types when new one gets added.
- Prasanth_J
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58934/#review173697
-----------------------------------------------------------
On May 2, 2017, 9:52 p.m., Jason Dere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58934/
> -----------------------------------------------------------
>
> (Updated May 2, 2017, 9:52 p.m.)
>
>
> Review request for hive, Gunther Hagleitner, Prasanth_J, and Siddharth Seth.
>
>
> Bugs: HIVE-16568
> https://issues.apache.org/jira/browse/HIVE-16568
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> - Support list/map/struct types in the LLAPRowInputFormat Schema/TypeDesc
> - Support list/map/struct types in the LLAPRowInputFormat Row. Changes in the Row getters/setters needed (no longer using Writable).
>
>
> Diffs
> -----
>
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniLlap.java de47412
> llap-client/src/java/org/apache/hadoop/hive/llap/LlapRowRecordReader.java ee92f3e
> llap-common/src/java/org/apache/hadoop/hive/llap/Row.java a84fadc
> llap-common/src/java/org/apache/hadoop/hive/llap/TypeDesc.java dda5928
> ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDTFGetSplits.java 9ddbd7e
>
>
> Diff: https://reviews.apache.org/r/58934/diff/1/
>
>
> Testing
> -------
>
> Added test to TestJdbcWithMiniLlap
>
>
> Thanks,
>
> Jason Dere
>
>
Re: Review Request 58934: HIVE-16568: Support complex types in
external LLAP InputFormat
Posted by Jason Dere <jd...@hortonworks.com>.
> On May 3, 2017, 5:34 a.m., Prasanth_J wrote:
> > llap-client/src/java/org/apache/hadoop/hive/llap/LlapRowRecordReader.java
> > Lines 154 (patched)
> > <https://reviews.apache.org/r/58934/diff/1/?file=1706163#file1706163line174>
> >
> > IIRC, there are utilities already to do this in ObjectInspectorUtils.. copyToXXX() methods.. can that be reused?
I had not seen the methods in ObjectInspectorUtils, yeah we might be able to use this.
> On May 3, 2017, 5:34 a.m., Prasanth_J wrote:
> > llap-common/src/java/org/apache/hadoop/hive/llap/TypeDesc.java
> > Lines 154 (patched)
> > <https://reviews.apache.org/r/58934/diff/1/?file=1706165#file1706165line154>
> >
> > This also looks repetitive. TypeInfoUtils already has something like this I guess. We need to make sure TypeInfo parser can parse the string generated by this method. It's easier to reuse TypeInfoUtils or have a TypeDesc converted to TypeInfo.
> >
> > My point there is duplicacy in
> > TypeInfo
> > TypeDesc
> > TypeDescriptor (ORC has this)
> >
> > Wondering if TypeInfo or TypeDescriptor from ORC can be reused here. Thoughts?
I had orginally created TypeDesc to try to avoid extra dependencies on what might be considered internal Hive types, but it is true that we are still the serde lib and TypeInfo is available from there. At this point though the TypeDesc has already been created, I'm not totally sure if I want to now go and remove this. If you feel strongly about this let me know.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58934/#review173697
-----------------------------------------------------------
On May 2, 2017, 9:52 p.m., Jason Dere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58934/
> -----------------------------------------------------------
>
> (Updated May 2, 2017, 9:52 p.m.)
>
>
> Review request for hive, Gunther Hagleitner, Prasanth_J, and Siddharth Seth.
>
>
> Bugs: HIVE-16568
> https://issues.apache.org/jira/browse/HIVE-16568
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> - Support list/map/struct types in the LLAPRowInputFormat Schema/TypeDesc
> - Support list/map/struct types in the LLAPRowInputFormat Row. Changes in the Row getters/setters needed (no longer using Writable).
>
>
> Diffs
> -----
>
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniLlap.java de47412
> llap-client/src/java/org/apache/hadoop/hive/llap/LlapRowRecordReader.java ee92f3e
> llap-common/src/java/org/apache/hadoop/hive/llap/Row.java a84fadc
> llap-common/src/java/org/apache/hadoop/hive/llap/TypeDesc.java dda5928
> ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDTFGetSplits.java 9ddbd7e
>
>
> Diff: https://reviews.apache.org/r/58934/diff/1/
>
>
> Testing
> -------
>
> Added test to TestJdbcWithMiniLlap
>
>
> Thanks,
>
> Jason Dere
>
>
Re: Review Request 58934: HIVE-16568: Support complex types in
external LLAP InputFormat
Posted by j....@gmail.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58934/#review173697
-----------------------------------------------------------
llap-client/src/java/org/apache/hadoop/hive/llap/LlapRowRecordReader.java
Lines 154 (patched)
<https://reviews.apache.org/r/58934/#comment246743>
IIRC, there are utilities already to do this in ObjectInspectorUtils.. copyToXXX() methods.. can that be reused?
llap-common/src/java/org/apache/hadoop/hive/llap/TypeDesc.java
Lines 154 (patched)
<https://reviews.apache.org/r/58934/#comment246744>
This also looks repetitive. TypeInfoUtils already has something like this I guess. We need to make sure TypeInfo parser can parse the string generated by this method. It's easier to reuse TypeInfoUtils or have a TypeDesc converted to TypeInfo.
My point there is duplicacy in
TypeInfo
TypeDesc
TypeDescriptor (ORC has this)
Wondering if TypeInfo or TypeDescriptor from ORC can be reused here. Thoughts?
- Prasanth_J
On May 2, 2017, 9:52 p.m., Jason Dere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58934/
> -----------------------------------------------------------
>
> (Updated May 2, 2017, 9:52 p.m.)
>
>
> Review request for hive, Gunther Hagleitner, Prasanth_J, and Siddharth Seth.
>
>
> Bugs: HIVE-16568
> https://issues.apache.org/jira/browse/HIVE-16568
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> - Support list/map/struct types in the LLAPRowInputFormat Schema/TypeDesc
> - Support list/map/struct types in the LLAPRowInputFormat Row. Changes in the Row getters/setters needed (no longer using Writable).
>
>
> Diffs
> -----
>
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniLlap.java de47412
> llap-client/src/java/org/apache/hadoop/hive/llap/LlapRowRecordReader.java ee92f3e
> llap-common/src/java/org/apache/hadoop/hive/llap/Row.java a84fadc
> llap-common/src/java/org/apache/hadoop/hive/llap/TypeDesc.java dda5928
> ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDTFGetSplits.java 9ddbd7e
>
>
> Diff: https://reviews.apache.org/r/58934/diff/1/
>
>
> Testing
> -------
>
> Added test to TestJdbcWithMiniLlap
>
>
> Thanks,
>
> Jason Dere
>
>
Re: Review Request 58934: HIVE-16568: Support complex types in
external LLAP InputFormat
Posted by j....@gmail.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58934/#review174049
-----------------------------------------------------------
Ship it!
Ship It!
- Prasanth_J
On May 5, 2017, 10:30 a.m., Jason Dere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58934/
> -----------------------------------------------------------
>
> (Updated May 5, 2017, 10:30 a.m.)
>
>
> Review request for hive, Gunther Hagleitner, Prasanth_J, and Siddharth Seth.
>
>
> Bugs: HIVE-16568
> https://issues.apache.org/jira/browse/HIVE-16568
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> - Support list/map/struct types in the LLAPRowInputFormat Schema/TypeDesc
> - Support list/map/struct types in the LLAPRowInputFormat Row. Changes in the Row getters/setters needed (no longer using Writable).
>
>
> Diffs
> -----
>
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/llap/ext/TestLlapInputSplit.java 654e92b
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniLlap.java de47412
> llap-client/src/java/org/apache/hadoop/hive/llap/LlapRowRecordReader.java ee92f3e
> llap-common/src/java/org/apache/hadoop/hive/llap/FieldDesc.java 9621978
> llap-common/src/java/org/apache/hadoop/hive/llap/Row.java a84fadc
> llap-common/src/java/org/apache/hadoop/hive/llap/TypeDesc.java dda5928
> llap-common/src/test/org/apache/hadoop/hive/llap/TestRow.java d4e68f4
> ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDTFGetSplits.java 9ddbd7e
> ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestInputOutputFormat.java b003eb8
>
>
> Diff: https://reviews.apache.org/r/58934/diff/3/
>
>
> Testing
> -------
>
> Added test to TestJdbcWithMiniLlap
>
>
> Thanks,
>
> Jason Dere
>
>
Re: Review Request 58934: HIVE-16568: Support complex types in
external LLAP InputFormat
Posted by Jason Dere <jd...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58934/
-----------------------------------------------------------
(Updated May 5, 2017, 10:30 a.m.)
Review request for hive, Gunther Hagleitner, Prasanth_J, and Siddharth Seth.
Changes
-------
Check for negative list/map size per Prasanth Jayachandran's comments.
Bugs: HIVE-16568
https://issues.apache.org/jira/browse/HIVE-16568
Repository: hive-git
Description
-------
- Support list/map/struct types in the LLAPRowInputFormat Schema/TypeDesc
- Support list/map/struct types in the LLAPRowInputFormat Row. Changes in the Row getters/setters needed (no longer using Writable).
Diffs (updated)
-----
itests/hive-unit/src/test/java/org/apache/hadoop/hive/llap/ext/TestLlapInputSplit.java 654e92b
itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniLlap.java de47412
llap-client/src/java/org/apache/hadoop/hive/llap/LlapRowRecordReader.java ee92f3e
llap-common/src/java/org/apache/hadoop/hive/llap/FieldDesc.java 9621978
llap-common/src/java/org/apache/hadoop/hive/llap/Row.java a84fadc
llap-common/src/java/org/apache/hadoop/hive/llap/TypeDesc.java dda5928
llap-common/src/test/org/apache/hadoop/hive/llap/TestRow.java d4e68f4
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDTFGetSplits.java 9ddbd7e
ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestInputOutputFormat.java b003eb8
Diff: https://reviews.apache.org/r/58934/diff/3/
Changes: https://reviews.apache.org/r/58934/diff/2-3/
Testing
-------
Added test to TestJdbcWithMiniLlap
Thanks,
Jason Dere
Re: Review Request 58934: HIVE-16568: Support complex types in
external LLAP InputFormat
Posted by j....@gmail.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58934/#review174008
-----------------------------------------------------------
llap-client/src/java/org/apache/hadoop/hive/llap/LlapRowRecordReader.java
Lines 168 (patched)
<https://reviews.apache.org/r/58934/#comment247101>
if listSize <0 set the convertedVal to null?
llap-client/src/java/org/apache/hadoop/hive/llap/LlapRowRecordReader.java
Lines 181 (patched)
<https://reviews.apache.org/r/58934/#comment247100>
if mapSize is <0 then getMap() will return null. Potential NPE here.
- Prasanth_J
On May 5, 2017, 2:49 a.m., Jason Dere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58934/
> -----------------------------------------------------------
>
> (Updated May 5, 2017, 2:49 a.m.)
>
>
> Review request for hive, Gunther Hagleitner, Prasanth_J, and Siddharth Seth.
>
>
> Bugs: HIVE-16568
> https://issues.apache.org/jira/browse/HIVE-16568
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> - Support list/map/struct types in the LLAPRowInputFormat Schema/TypeDesc
> - Support list/map/struct types in the LLAPRowInputFormat Row. Changes in the Row getters/setters needed (no longer using Writable).
>
>
> Diffs
> -----
>
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/llap/ext/TestLlapInputSplit.java 654e92b
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniLlap.java de47412
> llap-client/src/java/org/apache/hadoop/hive/llap/LlapRowRecordReader.java ee92f3e
> llap-common/src/java/org/apache/hadoop/hive/llap/FieldDesc.java 9621978
> llap-common/src/java/org/apache/hadoop/hive/llap/Row.java a84fadc
> llap-common/src/java/org/apache/hadoop/hive/llap/TypeDesc.java dda5928
> llap-common/src/test/org/apache/hadoop/hive/llap/TestRow.java d4e68f4
> ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDTFGetSplits.java 9ddbd7e
> ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestInputOutputFormat.java b003eb8
>
>
> Diff: https://reviews.apache.org/r/58934/diff/2/
>
>
> Testing
> -------
>
> Added test to TestJdbcWithMiniLlap
>
>
> Thanks,
>
> Jason Dere
>
>
Re: Review Request 58934: HIVE-16568: Support complex types in
external LLAP InputFormat
Posted by Jason Dere <jd...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58934/
-----------------------------------------------------------
(Updated May 5, 2017, 2:49 a.m.)
Review request for hive, Gunther Hagleitner, Prasanth_J, and Siddharth Seth.
Changes
-------
Use TypeInfo rather than TypeDesc. Remove TypeDesc class.
Bugs: HIVE-16568
https://issues.apache.org/jira/browse/HIVE-16568
Repository: hive-git
Description
-------
- Support list/map/struct types in the LLAPRowInputFormat Schema/TypeDesc
- Support list/map/struct types in the LLAPRowInputFormat Row. Changes in the Row getters/setters needed (no longer using Writable).
Diffs (updated)
-----
itests/hive-unit/src/test/java/org/apache/hadoop/hive/llap/ext/TestLlapInputSplit.java 654e92b
itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniLlap.java de47412
llap-client/src/java/org/apache/hadoop/hive/llap/LlapRowRecordReader.java ee92f3e
llap-common/src/java/org/apache/hadoop/hive/llap/FieldDesc.java 9621978
llap-common/src/java/org/apache/hadoop/hive/llap/Row.java a84fadc
llap-common/src/java/org/apache/hadoop/hive/llap/TypeDesc.java dda5928
llap-common/src/test/org/apache/hadoop/hive/llap/TestRow.java d4e68f4
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDTFGetSplits.java 9ddbd7e
ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestInputOutputFormat.java b003eb8
Diff: https://reviews.apache.org/r/58934/diff/2/
Changes: https://reviews.apache.org/r/58934/diff/1-2/
Testing
-------
Added test to TestJdbcWithMiniLlap
Thanks,
Jason Dere