Return-Path: X-Original-To: apmail-hive-dev-archive@www.apache.org Delivered-To: apmail-hive-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id B30B910DCE for ; Fri, 7 Feb 2014 22:41:49 +0000 (UTC) Received: (qmail 80054 invoked by uid 500); 7 Feb 2014 22:41:45 -0000 Delivered-To: apmail-hive-dev-archive@hive.apache.org Received: (qmail 80003 invoked by uid 500); 7 Feb 2014 22:41:45 -0000 Mailing-List: contact dev-help@hive.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@hive.apache.org Delivered-To: mailing list dev@hive.apache.org Received: (qmail 79993 invoked by uid 99); 7 Feb 2014 22:41:45 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 07 Feb 2014 22:41:45 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id E99521D4841; Fri, 7 Feb 2014 22:41:44 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============1173833658524144912==" MIME-Version: 1.0 Subject: Re: Review Request 17769: Generate vectorized plan for decimal expressions. From: "Eric Hanson" To: "Eric Hanson" Cc: "Jitendra Pandey" , "hive" Date: Fri, 07 Feb 2014 22:41:44 -0000 Message-ID: <20140207224144.31901.37025@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org Auto-Submitted: auto-generated Sender: "Eric Hanson" X-ReviewGroup: hive X-ReviewRequest-URL: https://reviews.apache.org/r/17769/ X-Sender: "Eric Hanson" References: <20140207023137.17020.87823@reviews.apache.org> In-Reply-To: <20140207023137.17020.87823@reviews.apache.org> Reply-To: "Eric Hanson" X-ReviewRequest-Repository: hive-git --===============1173833658524144912== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit ----------------------------------------------------------- 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 Please add a comment explaining what castExpressionUdfs is for ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java 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 Add comment before method explain what it does. ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java Hive Java coding standard says put blank line before all comments. ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java 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 add comment saying briefly what method does ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFOPMinus.java DecimalColAddDecimalScalar should be subtact ql/src/test/org/apache/hadoop/hive/ql/exec/vector/expressions/TestVectorStringExpressions.java please add brief comment saying what this test checks ql/src/test/queries/clientpositive/vector_decimal_expressions.q 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 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 > > --===============1173833658524144912==--