hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Xuefu Zhang" <xzh...@cloudera.com>
Subject Re: Review Request 14674: HIVE-3976: Support specifying scale and precision with Hive decimal type
Date Wed, 16 Oct 2013 23:35:51 GMT


> On Oct. 16, 2013, 11:05 p.m., Jason Dere wrote:
> > serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/primitive/WritableConstantHiveDecimalObjectInspector.java,
line 54
> > <https://reviews.apache.org/r/14674/diff/1/?file=365192#file365192line54>
> >
> >     But the constructor doesn't need to return null, it just needs to set this.value
to null.  Then getWritableConstantValue() can just return this.value, rather than having to
convert to HiveDecimal and back to HiveDecimalWritable on every call to getWritableConstantValue().

I got your point. Well, I believe that the way you outlined will work, but I don't see it
has any performance advantage. My understanding is that getWritableConstantValue() is only
called once per constant data. I don't see why the same value would be inspected over and
over again, unless I'm mistaken.

On the other hand, putting the logic in a nonconstructor will help error handling, in case
of hive having different error handling mode (HIVE-5438), in which different exceptions can
be thrown. Throwing an exception from a constructor seems a little unnatural and incumbersome.

Regardless, I'm open to move it to the constructor if doing so has performance gains.


- Xuefu


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14674/#review27100
-----------------------------------------------------------


On Oct. 16, 2013, 5:06 p.m., Xuefu Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14674/
> -----------------------------------------------------------
> 
> (Updated Oct. 16, 2013, 5:06 p.m.)
> 
> 
> Review request for hive and Ashutosh Chauhan.
> 
> 
> Bugs: HIVE-3976
>     https://issues.apache.org/jira/browse/HIVE-3976
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> This patch is one of the major pieces to support precision/scale for Hive decimal data
type. The following are the highlights:
> 
> 1. Grammar changes to allow optional precision/scale.
> 2. Semantical check added for decimal precision/scale.
> 3. Type info and object inspector factory changes.
> 4. UDF changes
> 5. Precision/scale enforcement in relavent object inspectors.
> 6. Test case changes/fixes.
> 7. New test cases.
> 
> 
> Diffs
> -----
> 
>   build.properties e1cd386 
>   common/src/java/org/apache/hadoop/hive/common/type/HiveDecimal.java cae8db6 
>   common/src/test/org/apache/hadoop/hive/common/type/TestHiveDecimal.java PRE-CREATION

>   data/files/kv9.txt PRE-CREATION 
>   jdbc/src/java/org/apache/hadoop/hive/jdbc/HiveResultSetMetaData.java 94b6ecd 
>   jdbc/src/java/org/apache/hadoop/hive/jdbc/Utils.java bd98274 
>   jdbc/src/test/org/apache/hadoop/hive/jdbc/TestJdbcDriver.java e1107dd 
>   jdbc/src/test/org/apache/hive/jdbc/TestJdbcDriver2.java e667aa6 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java d14bbcb 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java 41d5dd0 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/NumericOpMethodResolver.java 48dd7fd 
>   ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcStruct.java 65ee066 
>   ql/src/java/org/apache/hadoop/hive/ql/io/orc/WriterImpl.java e4ade90 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 037191a 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g 1f7b247 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/ParseUtils.java 12a0a69 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckProcFactory.java 82f3e47 
>   ql/src/java/org/apache/hadoop/hive/ql/udf/UDFOPDivide.java f6167d4 
>   ql/src/java/org/apache/hadoop/hive/ql/udf/UDFOPPlus.java 49c66cb 
>   ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFBridge.java c3c8ddc 
>   ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFToDecimal.java 60fe479

>   ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFToVarchar.java 58eca86

>   ql/src/test/org/apache/hadoop/hive/ql/exec/TestFunctionRegistry.java 50613f3 
>   ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestOrcFile.java 42bf9e4 
>   ql/src/test/org/apache/hadoop/hive/ql/parse/TestHiveDecimalParse.java PRE-CREATION

>   ql/src/test/queries/clientpositive/decimal_1.q 6c689e1 
>   ql/src/test/queries/clientpositive/decimal_2.q 4890618 
>   ql/src/test/queries/clientpositive/decimal_3.q 28211e3 
>   ql/src/test/queries/clientpositive/decimal_4.q e8a89c1 
>   ql/src/test/queries/clientpositive/decimal_5.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/decimal_6.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/decimal_join.q 589fc65 
>   ql/src/test/queries/clientpositive/decimal_precision.q 403c2be 
>   ql/src/test/queries/clientpositive/decimal_udf.q b5ff088 
>   ql/src/test/queries/clientpositive/orc_predicate_pushdown.q df89802 
>   ql/src/test/queries/clientpositive/ptf_decimal.q 03f435e 
>   ql/src/test/queries/clientpositive/serde_regex.q 2a287bd 
>   ql/src/test/queries/clientpositive/udf_pmod.q 9ff73d4 
>   ql/src/test/queries/clientpositive/udf_to_double.q b0a248a 
>   ql/src/test/queries/clientpositive/udf_to_float.q c91d18c 
>   ql/src/test/queries/clientpositive/udf_to_string.q 3b585e7 
>   ql/src/test/queries/clientpositive/windowing_expressions.q 2c33390 
>   ql/src/test/queries/clientpositive/windowing_multipartitioning.q bb371e9 
>   ql/src/test/queries/clientpositive/windowing_navfn.q 8a9d001 
>   ql/src/test/queries/clientpositive/windowing_ntile.q 505c259 
>   ql/src/test/queries/clientpositive/windowing_rank.q bf76867 
>   ql/src/test/results/clientnegative/invalid_cast_from_binary_1.q.out 015a704 
>   ql/src/test/results/clientnegative/invalid_cast_from_binary_2.q.out a8c6b88 
>   ql/src/test/results/clientnegative/invalid_cast_from_binary_3.q.out d3247e3 
>   ql/src/test/results/clientnegative/invalid_cast_from_binary_4.q.out c48186a 
>   ql/src/test/results/clientnegative/invalid_cast_from_binary_5.q.out bc3719c 
>   ql/src/test/results/clientnegative/invalid_cast_from_binary_6.q.out 19456ee 
>   ql/src/test/results/clientnegative/wrong_column_type.q.out 37a2ffc 
>   ql/src/test/results/clientpositive/decimal_1.q.out 71242eb 
>   ql/src/test/results/clientpositive/decimal_2.q.out 0b90b64 
>   ql/src/test/results/clientpositive/decimal_3.q.out 219c91a 
>   ql/src/test/results/clientpositive/decimal_4.q.out b5cc9e6 
>   ql/src/test/results/clientpositive/decimal_5.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/decimal_6.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/decimal_join.q.out 419fb7b 
>   ql/src/test/results/clientpositive/decimal_precision.q.out cf392ec 
>   ql/src/test/results/clientpositive/decimal_serde.q.out 138dbc0 
>   ql/src/test/results/clientpositive/decimal_udf.q.out 4f8f088 
>   ql/src/test/results/clientpositive/literal_decimal.q.out 1e93cd7 
>   ql/src/test/results/clientpositive/orc_predicate_pushdown.q.out ba7cf1a 
>   ql/src/test/results/clientpositive/ptf_decimal.q.out 2090829 
>   ql/src/test/results/clientpositive/serde_regex.q.out f462dfa 
>   ql/src/test/results/clientpositive/udf7.q.out 5f76d37 
>   ql/src/test/results/clientpositive/udf_pmod.q.out cc06f1d 
>   ql/src/test/results/clientpositive/udf_to_double.q.out 28e5089 
>   ql/src/test/results/clientpositive/udf_to_float.q.out b96383b 
>   ql/src/test/results/clientpositive/udf_to_string.q.out 664ff5c 
>   ql/src/test/results/clientpositive/windowing_expressions.q.out 8544879 
>   ql/src/test/results/clientpositive/windowing_multipartitioning.q.out 1953d6d 
>   ql/src/test/results/clientpositive/windowing_navfn.q.out 3272d57 
>   ql/src/test/results/clientpositive/windowing_ntile.q.out 36f738e 
>   ql/src/test/results/clientpositive/windowing_rank.q.out df06348 
>   serde/src/java/org/apache/hadoop/hive/serde2/RegexSerDe.java f2ddc73 
>   serde/src/java/org/apache/hadoop/hive/serde2/io/HiveDecimalWritable.java acab539 
>   serde/src/java/org/apache/hadoop/hive/serde2/lazy/LazyHiveDecimal.java 3be28dd 
>   serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/primitive/LazyHiveDecimalObjectInspector.java
5618d0c 
>   serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/primitive/LazyPrimitiveObjectInspectorFactory.java
6f03979 
>   serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/LazyBinaryHiveDecimal.java
a1d0e4c 
>   serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/LazyBinarySerDe.java ab4eb56

>   serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/primitive/JavaHiveDecimalObjectInspector.java
113445e 
>   serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/primitive/PrimitiveObjectInspectorFactory.java
fc0cee6 
>   serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/primitive/WritableConstantHiveDecimalObjectInspector.java
b6cb744 
>   serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/primitive/WritableHiveDecimalObjectInspector.java
dc9c8fb 
>   serde/src/java/org/apache/hadoop/hive/serde2/typeinfo/DecimalTypeInfo.java PRE-CREATION

>   serde/src/java/org/apache/hadoop/hive/serde2/typeinfo/HiveDecimalUtils.java PRE-CREATION

>   serde/src/java/org/apache/hadoop/hive/serde2/typeinfo/TypeInfo.java 36a7008 
>   serde/src/java/org/apache/hadoop/hive/serde2/typeinfo/TypeInfoFactory.java 13d1ec0

>   serde/src/java/org/apache/hadoop/hive/serde2/typeinfo/TypeInfoUtils.java d21abd4 
>   serde/src/java/org/apache/hadoop/hive/serde2/typeinfo/VarcharTypeInfo.java 5d6f3f4

> 
> Diff: https://reviews.apache.org/r/14674/diff/
> 
> 
> Testing
> -------
> 
> All unit tests passed last time when I ran them. New tests also passed when tested manually.
> 
> 
> Thanks,
> 
> Xuefu Zhang
> 
>


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