> On March 28, 2014, 10:42 p.m., Eric Hanson wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorHashKeyWrapper.java, > > line 62 > > <https://reviews.apache.org/r/19718/diff/3/?file=539988#file539988line62> > > > > please add a comment to explain why we use the sum of all the counts > > here to determine the array size.
This patch modifies only one line in this file, the other code has been around for quiet some time. I agree that we should add some comments to explain these offsets, I would like to address them in a separate jira. > On March 28, 2014, 10:42 p.m., Eric Hanson wrote: > > ql/src/test/org/apache/hadoop/hive/ql/exec/vector/TestVectorizationContext.java, > > line 968 > > <https://reviews.apache.org/r/19718/diff/3/?file=539995#file539995line968> > > > > Timestamp is supposed to be represented as a long (# of nanos since > > epoch). So whey is this using a FilterStringColumnBetween? The test compares timestamp with string constants, which requires these arguments to be cast to a common type. I am using the legacy hive FunctionRegistry to get the common type which happens to be string in this case. This should be optimized to instead cast strings to timestamps, but that would mean a different logic to determine common types for comparisons, which we better take up in a separate jira. This is not a regression even though test was passing earlier, because earlier for between clause, everything was getting cast to boolean (because first argument of Between expressions is a boolean for Not) which was incorrect anyway. > On March 28, 2014, 10:42 p.m., Eric Hanson wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorHashKeyWrapper.java, > > line 226 > > <https://reviews.apache.org/r/19718/diff/3/?file=539988#file539988line226> > > > > Consider for readability/encapsulation having a function to compute > > offset, e.g. > > > > isNull[decimalOffset(index)] = false; > > > > Please add a comment to explain offset logic. > > > > Does addition of decimal affect any other offsets? I guess not. All datatypes use this pattern of offset calculation. The readability argument is valid, but it would be better to address it for all datatypes in a separate jira. Addition of decimal was done in an earlier patch and it doesn't affect other offsets, this patch only sets the isNull correctly for non-null values. > On March 28, 2014, 10:42 p.m., Eric Hanson wrote: > > ql/src/test/org/apache/hadoop/hive/ql/exec/vector/TestVectorizationContext.java, > > line 973 > > <https://reviews.apache.org/r/19718/diff/3/?file=539995#file539995line973> > > > > Again, why string and not long "not between" operator? See previous one. - Jitendra ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19718/#review38958 ----------------------------------------------------------- On March 28, 2014, 9:56 p.m., Jitendra Pandey wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19718/ > ----------------------------------------------------------- > > (Updated March 28, 2014, 9:56 p.m.) > > > Review request for hive and Eric Hanson. > > > Bugs: HIVE-6752 > https://issues.apache.org/jira/browse/HIVE-6752 > > > Repository: hive-git > > > Description > ------- > > Vectorized Between and IN expressions don't work with decimal, date types. > > > Diffs > ----- > > ant/src/org/apache/hadoop/hive/ant/GenVectorCode.java 44b0c59 > ql/src/gen/vectorization/ExpressionTemplates/FilterDecimalColumnBetween.txt > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorHashKeyWrapper.java > 2229079 > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java > 96e74a9 > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDateToString.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/DecimalColumnInList.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/FilterDecimalColumnInList.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/IDecimalInExpr.java > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/Vectorizer.java > c2240c0 > > ql/src/test/org/apache/hadoop/hive/ql/exec/vector/TestVectorizationContext.java > 5ebab70 > ql/src/test/queries/clientpositive/vector_between_in.q PRE-CREATION > ql/src/test/results/clientpositive/vector_between_in.q.out PRE-CREATION > > Diff: https://reviews.apache.org/r/19718/diff/ > > > Testing > ------- > > > Thanks, > > Jitendra Pandey > >
