----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14674/#review27488 -----------------------------------------------------------
This looks really good to me! I have a number of "nits" below but after that as far as I am concerned, I am +1. common/src/test/org/apache/hadoop/hive/common/type/TestHiveDecimal.java <https://reviews.apache.org/r/14674/#comment53416> This constructor isn't used so it can be removed. common/src/test/org/apache/hadoop/hive/common/type/TestHiveDecimal.java <https://reviews.apache.org/r/14674/#comment53415> These assertTrue's should be assertTrue(String.valueOf(XXX), ...) ql/src/test/org/apache/hadoop/hive/ql/parse/TestHiveDecimalParse.java <https://reviews.apache.org/r/14674/#comment53410> Thank you for the nice unit tests! I believe we should improve the error messages so we don't have to change the code when debugging them to see what is wrong. These comments apply to many places in this test. The first assert should be: Assert.assertTrue("Got " + rc + ", expected not zero", rc != 0) second should be: Assert.assertTrue(driver.getErrorMsg(), driver.getErrorMsg().contains("Decimal precision out of allowed range [1,65]")); ql/src/test/queries/clientpositive/decimal_5.q <https://reviews.apache.org/r/14674/#comment53411> Let's trim the trailing whitespace from all the .q files. Under that is not possible for .q.out files. serde/src/java/org/apache/hadoop/hive/serde2/typeinfo/DecimalTypeInfo.java <https://reviews.apache.org/r/14674/#comment53412> If this is for Kryo only, this constructor can be protected so it's not generally available and doesn't give a warning (i.e. if it was private but not used). The same goes for JavaHiveDecimalObjectInspector and perhaps others. serde/src/java/org/apache/hadoop/hive/serde2/typeinfo/HiveDecimalUtils.java <https://reviews.apache.org/r/14674/#comment53413> It seems like IllegalArgumentException is more appropriate here and below? serde/src/java/org/apache/hadoop/hive/serde2/typeinfo/TypeInfoUtils.java <https://reviews.apache.org/r/14674/#comment53414> Is IllegalArgException more appropriate? - Brock Noland On Oct. 17, 2013, 4:50 p.m., Xuefu Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14674/ > ----------------------------------------------------------- > > (Updated Oct. 17, 2013, 4:50 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 > ----- > > 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 > >
