> On Oct. 13, 2015, 5:24 p.m., Xuefu Zhang wrote: > > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFBaseNwayCompare.java, > > line 74 > > <https://reviews.apache.org/r/39248/diff/1/?file=1096777#file1096777line74> > > > > 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.
I actually thought about doing that, but after some research I believe that the behavior of these UDF's should be consistent with the longstanding comparison operators (greater than, less than, greater than equal, less than equal, etc). Because the greatest udf is defined to be just running the greater than comparison on n-items, similarly for the least udf. This change makes the class consistent with those classes (see GenericUDFBaseCompare, FunctionRegistry.getCommonClassForComparison). > On Oct. 13, 2015, 5:24 p.m., Xuefu Zhang wrote: > > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFBaseNwayCompare.java, > > line 88 > > <https://reviews.apache.org/r/39248/diff/1/?file=1096777#file1096777line88> > > > > 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. There's a slight performance impact (function call), but I fixed it for code-cleaniness as you suggested. - Szehon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39248/#review102474 ----------------------------------------------------------- On Oct. 14, 2015, 1:02 a.m., Szehon Ho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39248/ > ----------------------------------------------------------- > > (Updated Oct. 14, 2015, 1:02 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 > >
