Review Board 1.7.22


HIVE-5356: Move arithmatic UDFs to generic UDF implementations

Review Request #15113 - Created Oct. 31, 2013 and updated

Xuefu Zhang
trunk
HIVe-5356
Reviewers
hive
hive-git
Replace plus, minus, and so on 6 old UDFs with generic UDF implementations.
Full set of unit tests is to be run. Old testcases are also migrated.
Total:
12
Open:
12
Resolved:
0
Dropped:
0
Status:
From:
Description From Last Updated Status
This early out is incorrect if it's 2 decimal TypeInfos that are the same. decimal(10,2) * decimal(10,2) != decimal(10,2) Jason Dere Nov. 1, 2013, 1:45 a.m. Open
I think this logic may also be incorrect - decimal(10,2) * ??? would very likely be a result other than ... Jason Dere Nov. 1, 2013, 1:45 a.m. Open
Should an exception be thrown either here or by the caller? Jason Dere Nov. 1, 2013, 1:45 a.m. Open
same as comments about about string type. Jason Dere Nov. 1, 2013, 1:45 a.m. Open
I think you are over inspecting here :) convert() can use the left variable directly, no need to inspect the ... Jason Dere Nov. 1, 2013, 1:45 a.m. Open
Just a nit, but maybe consider throwing exception here rather than returning null? I don't think these base implementations are ... Jason Dere Nov. 4, 2013, 10:56 p.m. Open
This comment is no longer true, since double has been changed to return precision 15. Jason Dere Nov. 4, 2013, 10:56 p.m. Open
Same as previous comment. Jason Dere Nov. 4, 2013, 10:56 p.m. Open
Rather than creating getNumericTypePriority(), can you use FunctionRegistry.getCommonType(), which already uses the same logic for numeric types? I think it ... Jason Dere Nov. 5, 2013, 7:28 p.m. Open
This part is still incorrect for decimal types, see the following result types below, add/sub/mult is wrong when the precision/scale ... Jason Dere Nov. 14, 2013, 9:32 p.m. Open
It looks like all these methods are going to be overridden by each sub-class? If that is true then they ... Brock Noland Nov. 15, 2013, 6:12 p.m. Open
Should these be constants in common or some shared location so jdbc and serde would share the same value? Brock Noland Nov. 15, 2013, 6:12 p.m. Open
Review request changed
Updated (Nov. 14, 2013, 10:11 p.m.)
Posted (Nov. 15, 2013, 6:13 p.m.)
Nice!! Looks good to me and it appears there are new unit tests! Some minor questions and comments below as well as two minor issues.
I see there are utilities in here prior to this patch so no work to do here but Function Registry doesn't feel like the right place to have these utilities.
I assume these aren't final due to serialization. If so, it might be worth a comment indicating so.
  1. Actually this was copied from its non-UDF counterpart. It can be finaly, but personally I don't think it's a big dea.
It looks like all these methods are going to be overridden by each sub-class? If that is true then they should be abstract with no body.
  1. Actually, GenericUDFOPDivide only overrides two of them.
Should these be constants in common or some shared location so jdbc and serde would share the same value?
  1. Created followup JIRA HIVE-5838.