----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17005/#review32299 -----------------------------------------------------------
common/src/java/org/apache/hadoop/hive/common/type/Decimal128.java <https://reviews.apache.org/r/17005/#comment61044> I think it's worth having a case for signum == 0 to update the value to 0, to make correctness obvious, and for speed too, since 0 is a very common value. You can use update(0) and not have to use the updateBigInteger function. common/src/java/org/apache/hadoop/hive/common/type/UnsignedInt128.java <https://reviews.apache.org/r/17005/#comment61048> You should put a comment that behavior is undefined if the BigInteger argument is negative, and that you should only pass in positive values. common/src/java/org/apache/hadoop/hive/common/type/UnsignedInt128.java <https://reviews.apache.org/r/17005/#comment61045> The convention in this code is to overload update() based on argument type, so I think it's best to call the method "update" instead of "updateBigInteger". Also, add a comment that argument must not be negative. If it is, I think sign extension from shiftRight might cause an error. ql/src/java/org/apache/hadoop/hive/ql/io/orc/RecordReaderImpl.java <https://reviews.apache.org/r/17005/#comment61047> can you comment why you are sharing the result null vector into the scratch one? ql/src/java/org/apache/hadoop/hive/ql/io/orc/RecordReaderImpl.java <https://reviews.apache.org/r/17005/#comment61049> It seems odd that we're reading from a scaleStream because the scale should be the same for every value in the column. Is this necessary? ql/src/java/org/apache/hadoop/hive/ql/io/orc/RecordReaderImpl.java <https://reviews.apache.org/r/17005/#comment61050> If any scale values are different inside a single DecimalColumnVector, I think that could cause unpredictable or wrong results. Later operations on DecimalColumnVector take the scale from the columnvector sometimes, not each individual object. ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestVectorizedORCReader.java <https://reviews.apache.org/r/17005/#comment61051> do you want to include the printing in the final test? - Eric Hanson On Jan. 17, 2014, 12:58 a.m., Jitendra Pandey wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17005/ > ----------------------------------------------------------- > > (Updated Jan. 17, 2014, 12:58 a.m.) > > > Review request for hive and Eric Hanson. > > > Bugs: HIVE-6178 > https://issues.apache.org/jira/browse/HIVE-6178 > > > Repository: hive-git > > > Description > ------- > > vectorized reader for DECIMAL datatype for ORC format. > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/common/type/Decimal128.java 3939511 > common/src/java/org/apache/hadoop/hive/common/type/UnsignedInt128.java > d71ebb3 > common/src/test/org/apache/hadoop/hive/common/type/TestUnsignedInt128.java > fbb2aa0 > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/DecimalColumnVector.java > 23564bb > ql/src/java/org/apache/hadoop/hive/ql/io/orc/RecordReaderImpl.java 0876bf7 > ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestVectorizedORCReader.java > 0d5b7ff > > Diff: https://reviews.apache.org/r/17005/diff/ > > > Testing > ------- > > > Thanks, > > Jitendra Pandey > >
