----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17769/#review33942 -----------------------------------------------------------
Overall this looks good. Please see my specific comments. I did find one bug (used an Add in place of Subtract in GenericUDFOpMinus), and possibly one design issue related to implicit cast precision and scale. ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java <https://reviews.apache.org/r/17769/#comment63766> Please add a comment explaining what castExpressionUdfs is for ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java <https://reviews.apache.org/r/17769/#comment63767> Expand the comment to explain the kind of situations where this is necessary. ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java <https://reviews.apache.org/r/17769/#comment63768> Add comment before method explain what it does. ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java <https://reviews.apache.org/r/17769/#comment63808> Hive Java coding standard says put blank line before all comments. ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java <https://reviews.apache.org/r/17769/#comment63769> Because TypeInfo has decimal precision/scale, the output scale is not always the same as the input scale. E.g. I've seen that decimal(18,2)*decimal(18,2) might have scale=4 or something like that. Might it be better to have integers be cast to decimal(19,0) and floats to, say, decimal(38,18) or something like that, so you never or rarely lose information during the cast, or get a NULL due to overflow? But of course, you would not change the expression result precision/scale. What you have here looks pretty good, but it may be worth more thought. ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizedRowBatchCtx.java <https://reviews.apache.org/r/17769/#comment63816> add comment saying briefly what method does ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFOPMinus.java <https://reviews.apache.org/r/17769/#comment63823> DecimalColAddDecimalScalar should be subtact ql/src/test/org/apache/hadoop/hive/ql/exec/vector/expressions/TestVectorStringExpressions.java <https://reviews.apache.org/r/17769/#comment63825> please add brief comment saying what this test checks ql/src/test/queries/clientpositive/vector_decimal_expressions.q <https://reviews.apache.org/r/17769/#comment63828> I think we need a JIRA to add unary minus for vectorized decimal, plus a test. ql/src/test/results/clientpositive/vectorization_short_regress.q.out <https://reviews.apache.org/r/17769/#comment63837> It looks like some new rows showed up in the output after you changed the test. Is this expected, or does it reveal a correctness issue? - Eric Hanson On Feb. 7, 2014, 2:31 a.m., Jitendra Pandey wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17769/ > ----------------------------------------------------------- > > (Updated Feb. 7, 2014, 2:31 a.m.) > > > Review request for hive and Eric Hanson. > > > Bugs: HIVE-6333 > https://issues.apache.org/jira/browse/HIVE-6333 > > > Repository: hive-git > > > Description > ------- > > Generate vectorized plan for decimal expressions. > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/common/type/HiveDecimal.java 29c5168 > > ql/src/gen/vectorization/ExpressionTemplates/ColumnArithmeticColumnDecimal.txt > 699b7c5 > > ql/src/gen/vectorization/ExpressionTemplates/ColumnArithmeticScalarDecimal.txt > 99366ca > ql/src/gen/vectorization/ExpressionTemplates/ColumnDivideColumnDecimal.txt > 2aa4152 > ql/src/gen/vectorization/ExpressionTemplates/ColumnDivideScalarDecimal.txt > 2e84334 > > ql/src/gen/vectorization/ExpressionTemplates/ScalarArithmeticColumnDecimal.txt > 9578d34 > ql/src/gen/vectorization/ExpressionTemplates/ScalarDivideColumnDecimal.txt > 6ee9d5f > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorExpressionDescriptor.java > 1c70387 > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java > f5ab731 > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizedRowBatchCtx.java > f513188 > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/AbstractFilterStringColLikeStringScalar.java > 4510368 > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDecimalToBoolean.java > 6a7762d > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDecimalToDecimal.java > 14b91e1 > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDecimalToDouble.java > 2ba1509 > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDecimalToLong.java > 65a804d > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDecimalToString.java > 5b2a658 > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastDoubleToDecimal.java > 14e30c3 > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastLongToDecimal.java > 1d4d84d > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastStringToDecimal.java > 41762ed > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/CastTimestampToDecimal.java > 37e92e1 > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/ConstantVectorExpression.java > cac1d80 > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/FilterStringColRegExpStringScalar.java > 93052a1 > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/FuncDoubleToDecimal.java > 8b2a6f0 > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/FuncLongToDecimal.java > 18d1dbb > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/VectorExpression.java > d00d99b > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/VectorExpressionWriter.java > e5c3aa4 > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/VectorExpressionWriterFactory.java > a242fef > ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/Vectorizer.java > ad96fa5 > ql/src/java/org/apache/hadoop/hive/ql/udf/UDFToByte.java 4f59125 > ql/src/java/org/apache/hadoop/hive/ql/udf/UDFToDouble.java e4dfcc9 > ql/src/java/org/apache/hadoop/hive/ql/udf/UDFToFloat.java 4e2d1d4 > ql/src/java/org/apache/hadoop/hive/ql/udf/UDFToInteger.java 6f9746c > ql/src/java/org/apache/hadoop/hive/ql/udf/UDFToLong.java e794e92 > ql/src/java/org/apache/hadoop/hive/ql/udf/UDFToShort.java 4e64d47 > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFOPDivide.java > 9a04e81 > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFOPEqual.java > 3479b13 > > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFOPEqualOrGreaterThan.java > edb1bf8 > > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFOPEqualOrLessThan.java > 06d9647 > > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFOPGreaterThan.java > 28bce88 > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFOPLessThan.java > 9258b43 > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFOPMinus.java > 6ee6f39 > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFOPMultiply.java > e7a2a8d > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFOPNotEqual.java > 4c11e5b > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFOPPlus.java > 26ac65c > > ql/src/test/org/apache/hadoop/hive/ql/exec/vector/TestVectorizationContext.java > 454a02d > > ql/src/test/org/apache/hadoop/hive/ql/exec/vector/expressions/TestVectorStringExpressions.java > 4f1169c > ql/src/test/queries/clientpositive/vector_decimal_expressions.q > PRE-CREATION > ql/src/test/queries/clientpositive/vectorization_decimal_date.q > PRE-CREATION > ql/src/test/results/clientpositive/vector_decimal_expressions.q.out > PRE-CREATION > ql/src/test/results/clientpositive/vectorization_decimal_date.q.out > PRE-CREATION > ql/src/test/results/clientpositive/vectorization_short_regress.q.out > 305d336 > > Diff: https://reviews.apache.org/r/17769/diff/ > > > Testing > ------- > > > Thanks, > > Jitendra Pandey > >
