hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Eric Hanson" <eh...@microsoft.com>
Subject Re: Review Request 17769: Generate vectorized plan for decimal expressions.
Date Fri, 07 Feb 2014 22:41:44 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message