----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39248/#review102474 -----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFBaseNwayCompare.java (line 74) <https://reviews.apache.org/r/39248/#comment160157> should we limit the types to be numeric? From here I don't know how mixed types are compared, especially when there is no common type. ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFBaseNwayCompare.java (line 88) <https://reviews.apache.org/r/39248/#comment160165> instead of two code paths (two method), can we just always do conversions? when there is no need for conversion, the converter will be an identity converter, which doesn't incur any performance penalty. This way, the code will be simpler and cleaner in my opinion. ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLeast.java (line 30) <https://reviews.apache.org/r/39248/#comment160154> should this class inherit the base instead of "greatest" from which I don't see it gets anything. - Xuefu Zhang On Oct. 13, 2015, 12:18 a.m., Szehon Ho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39248/ > ----------------------------------------------------------- > > (Updated Oct. 13, 2015, 12:18 a.m.) > > > Review request for hive. > > > Bugs: HIVE-12082 > https://issues.apache.org/jira/browse/HIVE-12082 > > > Repository: hive-git > > > Description > ------- > > See HIVE-12070 and HIVE-12082 for discussions and findings. Refactored the > Greatest/Least UDF's to be inline with the SQL-standard spec of comparison > operators, and the mysql implementation of greatest/least functional UDF's. > > The main functional changes is that: > 1. Different types can be compared now, but used to throw an exception. > Comparison uses the same logic as binary comparison operators, ie greaterThan. > 2. If any argument is NULL, the result is null. NULLs used to be ignored in > the comparison in favor of non-null values, which violates the SQL-standard. > > Code changes: > Common logics is captured in the new class 'GenericUDFBaseNWayCompare', which > does a linear comparison in the two-cases where arguments are of same type > and different type, in the latter it uses Converters. The class that it uses > is ObjectInspectorUtils.compare(), which is the same as the binary comparison > operators. > > > Diffs > ----- > > > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFBaseNwayCompare.java > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFGreatest.java > e1eab89 > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLeast.java > 64a1b47 > > ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFGreatest.java > 55d7d5d > ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFLeast.java > 47e4801 > ql/src/test/queries/clientnegative/udf_greatest_error_2.q b270a1a > ql/src/test/queries/clientnegative/udf_greatest_error_3.q ba21748 > ql/src/test/queries/clientnegative/udf_greatest_error_4.q ae6d928 > ql/src/test/queries/clientpositive/udf_greatest.q 02c7d3c > ql/src/test/queries/clientpositive/udf_least.q a754ef0 > ql/src/test/results/clientnegative/udf_greatest_error_2.q.out 9a6348c > ql/src/test/results/clientnegative/udf_greatest_error_3.q.out 3fb3499 > ql/src/test/results/clientnegative/udf_greatest_error_4.q.out 58b4c44 > ql/src/test/results/clientpositive/udf_greatest.q.out 10f1c2d > ql/src/test/results/clientpositive/udf_least.q.out 6983137 > > Diff: https://reviews.apache.org/r/39248/diff/ > > > Testing > ------- > > Added more unit tests, and q tests. > > > Thanks, > > Szehon Ho > >
