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