Review Board 1.7.22


HIVE-3976: Support specifying scale and precision with Hive decimal type

Review Request #14674 - Created Oct. 16, 2013 and updated

Xuefu Zhang
trunk
HIVE-3976
Reviewers
hive
ashutoshc
hive-git
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.
All unit tests passed last time when I ran them. New tests also passed when tested manually.
Total:
17
Open:
15
Resolved:
2
Dropped:
0
Status:
From:
Description From Last Updated Status
Can all of this be done in the constructor when this.value is initially set? Jason Dere Oct. 16, 2013, 10:21 p.m. Open
Are there ever decimal TypeInfo instances where the qualified name is "decimal"? Or is everything at the TypeInfo level fully ... Jason Dere Oct. 16, 2013, 10:21 p.m. Open
remove or convert to log statement Jason Dere Oct. 16, 2013, 10:21 p.m. Open
Yeah, but I think it's more apropriate to enforce here, as constructor has no means of return null. We do ... Xuefu Zhang Oct. 16, 2013, 10:50 p.m. Open
Yeah, it should be this.precision() - this.scale() >= dti.precision() - dti.scale(). However, it usually doesn't matter much as "this" has ... Xuefu Zhang Oct. 16, 2013, 10:50 p.m. Open
At typeinfo level, precision/scale must be existing. At user never, "decimal" may be used w/o giving precision/scale. Xuefu Zhang Oct. 16, 2013, 10:50 p.m. Open
Debugging code. Will remove. Xuefu Zhang Oct. 16, 2013, 10:50 p.m. Open
But the constructor doesn't need to return null, it just needs to set this.value to null. Then getWritableConstantValue() can just ... Jason Dere Oct. 16, 2013, 11:05 p.m. Open
This constructor isn't used so it can be removed. Brock Noland Oct. 24, 2013, 9:47 p.m. Open
These assertTrue's should be assertTrue(String.valueOf(XXX), ...) Brock Noland Oct. 24, 2013, 9:47 p.m. Open
Thank you for the nice unit tests! I believe we should improve the error messages so we don't have to ... Brock Noland Oct. 24, 2013, 9:47 p.m. Open
Let's trim the trailing whitespace from all the .q files. Under that is not possible for .q.out files. Brock Noland Oct. 24, 2013, 9:47 p.m. Open
If this is for Kryo only, this constructor can be protected so it's not generally available and doesn't give a ... Brock Noland Oct. 24, 2013, 9:47 p.m. Open
It seems like IllegalArgumentException is more appropriate here and below? Brock Noland Oct. 24, 2013, 9:47 p.m. Open
Is IllegalArgException more appropriate? Brock Noland Oct. 24, 2013, 9:47 p.m. Open
Review request changed
Updated (Oct. 25, 2013, 5:21 a.m.)