Re: Review Request 31404: HIVE-9744 Move common arguments validation and value extraction code to GenericUDF
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31404/ --- (Updated Feb. 28, 2015, 1:38 a.m.) Review request for hive, Jason Dere and Thejas Nair. Changes --- rebase to the latest codebase Bugs: HIVE-9744 https://issues.apache.org/jira/browse/HIVE-9744 Repository: hive-git Description --- HIVE-9744 Move common arguments validation and value extraction code to GenericUDF Diffs (updated) - common/src/java/org/apache/hive/common/util/DateUtils.java PRE-CREATION ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java 8a0f573648c51c4945be8ffec4a0b06dfa7061c8 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFAddMonths.java c5968835a74195bea6b31a5c7b7346907fed5ce0 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFInitCap.java 406fcd608a13fadb8902bf273932acb05a0f3bbe ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLastDay.java 3a43c571ae3a83924a00413181a62ce6f4408125 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLevenshtein.java 4c9f9b1567c89e2175acd970ad12a1fd37492607 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFNextDay.java 38f08b74609a4018221ca3f5b92cf33799604d60 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFAddMonths.java 4ccae97a227257294d69f728426f425d060ef0c7 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFLastDay.java 4b233a6966bbdf6902c53f2aaf53cc0eb422b205 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFLevenshtein.java 5346e45a6befb18f63ea24212295016fe06f2ca0 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFNextDay.java e2ec551d4ae39d521680ee93c791f14f27811270 ql/src/test/results/clientnegative/udf_add_months_error_1.q.out 8226ac6fe89c38fcc14edeea215cd5cce7258683 ql/src/test/results/clientnegative/udf_add_months_error_2.q.out f00949e9a12285cc91032215372975753c1f3b4a ql/src/test/results/clientnegative/udf_last_day_error_1.q.out 6e718a0c15e84d89b1cfe7f36231e472ff03c37f ql/src/test/results/clientnegative/udf_last_day_error_2.q.out dc8e3d14f14205ce65355cd53a95cfc788f45fe0 ql/src/test/results/clientnegative/udf_next_day_error_1.q.out c67b9c42f7e7fdf20caa34d028b02fd4819e8343 ql/src/test/results/clientnegative/udf_next_day_error_2.q.out e3cb6a447bf7bd9f09648e46ace1bcba4da55339 Diff: https://reviews.apache.org/r/31404/diff/ Testing --- Thanks, Alexander Pivovarov
Re: Review Request 31404: HIVE-9744 Move common arguments validation and value extraction code to GenericUDF
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31404/#review74584 --- ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java https://reviews.apache.org/r/31404/#comment121194 Hmm, this member ends up being a part of all GenericUDFs, but is only used by the methods that use these new convertDate methods. Makes me wonder if we should package these changes (plus array of Converters) as a separate utility class. What do you think of that? Not a necessary change, we can leave it as it is and can always revisit this later. - Jason Dere On Feb. 27, 2015, 12:11 a.m., Alexander Pivovarov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31404/ --- (Updated Feb. 27, 2015, 12:11 a.m.) Review request for hive, Jason Dere and Thejas Nair. Bugs: HIVE-9744 https://issues.apache.org/jira/browse/HIVE-9744 Repository: hive-git Description --- HIVE-9744 Move common arguments validation and value extraction code to GenericUDF Diffs - ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java 8a0f573648c51c4945be8ffec4a0b06dfa7061c8 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFAddMonths.java c5968835a74195bea6b31a5c7b7346907fed5ce0 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFInitCap.java 406fcd608a13fadb8902bf273932acb05a0f3bbe ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLastDay.java 3a43c571ae3a83924a00413181a62ce6f4408125 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLevenstein.java de41793ba3925aa9e1ad9623d92881c57791f047 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFNextDay.java 38f08b74609a4018221ca3f5b92cf33799604d60 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFAddMonths.java 4ccae97a227257294d69f728426f425d060ef0c7 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFLastDay.java 4b233a6966bbdf6902c53f2aaf53cc0eb422b205 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFLevenshtein.java e674d9f38cf7b5cdffcad6eca07dba74ff1e834b ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFNextDay.java e2ec551d4ae39d521680ee93c791f14f27811270 ql/src/test/results/clientnegative/udf_add_months_error_1.q.out 8226ac6fe89c38fcc14edeea215cd5cce7258683 ql/src/test/results/clientnegative/udf_add_months_error_2.q.out f00949e9a12285cc91032215372975753c1f3b4a ql/src/test/results/clientnegative/udf_last_day_error_1.q.out 6e718a0c15e84d89b1cfe7f36231e472ff03c37f ql/src/test/results/clientnegative/udf_last_day_error_2.q.out dc8e3d14f14205ce65355cd53a95cfc788f45fe0 ql/src/test/results/clientnegative/udf_next_day_error_1.q.out c67b9c42f7e7fdf20caa34d028b02fd4819e8343 ql/src/test/results/clientnegative/udf_next_day_error_2.q.out e3cb6a447bf7bd9f09648e46ace1bcba4da55339 Diff: https://reviews.apache.org/r/31404/diff/ Testing --- Thanks, Alexander Pivovarov
Re: Review Request 31404: HIVE-9744 Move common arguments validation and value extraction code to GenericUDF
On Feb. 27, 2015, 9:01 p.m., Jason Dere wrote: ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java, line 70 https://reviews.apache.org/r/31404/diff/2-3/?file=878673#file878673line70 Hmm, this member ends up being a part of all GenericUDFs, but is only used by the methods that use these new convertDate methods. Makes me wonder if we should package these changes (plus array of Converters) as a separate utility class. What do you think of that? Not a necessary change, we can leave it as it is and can always revisit this later. Added thread-safe DateUtils class which caches SimpleDateFormat(-MM-dd) for each thread. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31404/#review74584 --- On Feb. 27, 2015, 12:11 a.m., Alexander Pivovarov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31404/ --- (Updated Feb. 27, 2015, 12:11 a.m.) Review request for hive, Jason Dere and Thejas Nair. Bugs: HIVE-9744 https://issues.apache.org/jira/browse/HIVE-9744 Repository: hive-git Description --- HIVE-9744 Move common arguments validation and value extraction code to GenericUDF Diffs - ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java 8a0f573648c51c4945be8ffec4a0b06dfa7061c8 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFAddMonths.java c5968835a74195bea6b31a5c7b7346907fed5ce0 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFInitCap.java 406fcd608a13fadb8902bf273932acb05a0f3bbe ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLastDay.java 3a43c571ae3a83924a00413181a62ce6f4408125 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLevenstein.java de41793ba3925aa9e1ad9623d92881c57791f047 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFNextDay.java 38f08b74609a4018221ca3f5b92cf33799604d60 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFAddMonths.java 4ccae97a227257294d69f728426f425d060ef0c7 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFLastDay.java 4b233a6966bbdf6902c53f2aaf53cc0eb422b205 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFLevenshtein.java e674d9f38cf7b5cdffcad6eca07dba74ff1e834b ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFNextDay.java e2ec551d4ae39d521680ee93c791f14f27811270 ql/src/test/results/clientnegative/udf_add_months_error_1.q.out 8226ac6fe89c38fcc14edeea215cd5cce7258683 ql/src/test/results/clientnegative/udf_add_months_error_2.q.out f00949e9a12285cc91032215372975753c1f3b4a ql/src/test/results/clientnegative/udf_last_day_error_1.q.out 6e718a0c15e84d89b1cfe7f36231e472ff03c37f ql/src/test/results/clientnegative/udf_last_day_error_2.q.out dc8e3d14f14205ce65355cd53a95cfc788f45fe0 ql/src/test/results/clientnegative/udf_next_day_error_1.q.out c67b9c42f7e7fdf20caa34d028b02fd4819e8343 ql/src/test/results/clientnegative/udf_next_day_error_2.q.out e3cb6a447bf7bd9f09648e46ace1bcba4da55339 Diff: https://reviews.apache.org/r/31404/diff/ Testing --- Thanks, Alexander Pivovarov
Re: Review Request 31404: HIVE-9744 Move common arguments validation and value extraction code to GenericUDF
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31404/ --- (Updated Feb. 28, 2015, 1:28 a.m.) Review request for hive, Jason Dere and Thejas Nair. Changes --- added DateUtils Bugs: HIVE-9744 https://issues.apache.org/jira/browse/HIVE-9744 Repository: hive-git Description --- HIVE-9744 Move common arguments validation and value extraction code to GenericUDF Diffs (updated) - common/src/java/org/apache/hive/common/util/DateUtils.java PRE-CREATION ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java 8a0f573648c51c4945be8ffec4a0b06dfa7061c8 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFAddMonths.java c5968835a74195bea6b31a5c7b7346907fed5ce0 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFInitCap.java 406fcd608a13fadb8902bf273932acb05a0f3bbe ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLastDay.java 3a43c571ae3a83924a00413181a62ce6f4408125 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLevenstein.java de41793ba3925aa9e1ad9623d92881c57791f047 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFNextDay.java 38f08b74609a4018221ca3f5b92cf33799604d60 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFAddMonths.java 4ccae97a227257294d69f728426f425d060ef0c7 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFLastDay.java 4b233a6966bbdf6902c53f2aaf53cc0eb422b205 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFLevenshtein.java e674d9f38cf7b5cdffcad6eca07dba74ff1e834b ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFNextDay.java e2ec551d4ae39d521680ee93c791f14f27811270 ql/src/test/results/clientnegative/udf_add_months_error_1.q.out 8226ac6fe89c38fcc14edeea215cd5cce7258683 ql/src/test/results/clientnegative/udf_add_months_error_2.q.out f00949e9a12285cc91032215372975753c1f3b4a ql/src/test/results/clientnegative/udf_last_day_error_1.q.out 6e718a0c15e84d89b1cfe7f36231e472ff03c37f ql/src/test/results/clientnegative/udf_last_day_error_2.q.out dc8e3d14f14205ce65355cd53a95cfc788f45fe0 ql/src/test/results/clientnegative/udf_next_day_error_1.q.out c67b9c42f7e7fdf20caa34d028b02fd4819e8343 ql/src/test/results/clientnegative/udf_next_day_error_2.q.out e3cb6a447bf7bd9f09648e46ace1bcba4da55339 Diff: https://reviews.apache.org/r/31404/diff/ Testing --- Thanks, Alexander Pivovarov
Re: Review Request 31404: HIVE-9744 Move common arguments validation and value extraction code to GenericUDF
On Feb. 26, 2015, 10:37 p.m., Jason Dere wrote: ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java, line 459 https://reviews.apache.org/r/31404/diff/1-2/?file=875138#file875138line459 Not a fan of these timestamp-related changes. It's causing you to have to create 2 new arrays of Converters which seems awkward. I was going to suggest simply using a DateConverter, but I see that the original behavior also allowed timestamp format strings (really, it allowed anything after the '-mm-dd' part). Rather than have 2 different converters, maybe just go back to what you had before, use a single converter to string, and then just apply whatever format logic you are going to apply to the string value. Sure I can use SimpleDateFormat(-MM-dd) to convert short and long strings to Date. That was the original behaviour in last_day, next_day, add_months. methods (obtainDateConvertor and getDateValue) I also noticed that GenericUDFFromUtcTimestamp uses TimestampConverter for all input types. To support this behaviour I created 2 methods (obtainTimestampConvertor and getTimestampValue) On Feb. 26, 2015, 10:37 p.m., Jason Dere wrote: ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFNextDay.java, line 80 https://reviews.apache.org/r/31404/diff/2/?file=878682#file878682line80 So this is actually an example of something that used to be valid input, but no longer works due to the change from SimpleDateFormat to Timestamp/Date converter. This would be ok for the UDFs you've changed since they are new ones, but I don't know if this kind of change in behavior would be something that would be a concern if we were to apply this same refactoring to some of the other date-related UDFs. fixed this. all selects in last_day, next_day, add_months q files return what they originally returned now - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31404/#review74371 --- On Feb. 26, 2015, 9:16 p.m., Alexander Pivovarov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31404/ --- (Updated Feb. 26, 2015, 9:16 p.m.) Review request for hive, Jason Dere and Thejas Nair. Bugs: HIVE-9744 https://issues.apache.org/jira/browse/HIVE-9744 Repository: hive-git Description --- HIVE-9744 Move common arguments validation and value extraction code to GenericUDF Diffs - ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java 8a0f573648c51c4945be8ffec4a0b06dfa7061c8 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFAddMonths.java c5968835a74195bea6b31a5c7b7346907fed5ce0 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFInitCap.java 406fcd608a13fadb8902bf273932acb05a0f3bbe ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLastDay.java 3a43c571ae3a83924a00413181a62ce6f4408125 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLevenstein.java de41793ba3925aa9e1ad9623d92881c57791f047 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFNextDay.java 38f08b74609a4018221ca3f5b92cf33799604d60 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFAddMonths.java 4ccae97a227257294d69f728426f425d060ef0c7 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFLastDay.java 4b233a6966bbdf6902c53f2aaf53cc0eb422b205 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFLevenshtein.java e674d9f38cf7b5cdffcad6eca07dba74ff1e834b ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFNextDay.java e2ec551d4ae39d521680ee93c791f14f27811270 ql/src/test/queries/clientpositive/udf_next_day.q db821f00c39b84c4bc59ea70b0cc0aacf69cbd18 ql/src/test/results/clientnegative/udf_add_months_error_1.q.out 8226ac6fe89c38fcc14edeea215cd5cce7258683 ql/src/test/results/clientnegative/udf_add_months_error_2.q.out f00949e9a12285cc91032215372975753c1f3b4a ql/src/test/results/clientnegative/udf_last_day_error_1.q.out 6e718a0c15e84d89b1cfe7f36231e472ff03c37f ql/src/test/results/clientnegative/udf_last_day_error_2.q.out dc8e3d14f14205ce65355cd53a95cfc788f45fe0 ql/src/test/results/clientnegative/udf_next_day_error_1.q.out c67b9c42f7e7fdf20caa34d028b02fd4819e8343 ql/src/test/results/clientnegative/udf_next_day_error_2.q.out e3cb6a447bf7bd9f09648e46ace1bcba4da55339 ql/src/test/results/clientpositive/udf_add_months.q.out 8c37fc282a25e350435ff6a4b39e02fc2d4f17df ql/src/test/results/clientpositive/udf_last_day.q.out 2d39e3897625c7651b110779b89652f0d785bc92 Diff:
Re: Review Request 31404: HIVE-9744 Move common arguments validation and value extraction code to GenericUDF
On Feb. 26, 2015, 7:46 a.m., Jason Dere wrote: ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java, line 383 https://reviews.apache.org/r/31404/diff/1/?file=875138#file875138line383 Why go away from using the formatter as it was doing before? Are there any ramifications here to switching to Timestamp.valueOf()? I think Timestamp.valueOf() is probably less forgiving when checking the format. Instead of Timestamp.valueOf and SimpleDateFormat.parse I'm going to use PrimitiveObjectInspectorConverter.DateConverters and TimestempConverters to convert string to Date/Timestamp. Internally converters use Date.valueOf (PrimitiveObjectInspectorUtils lines 999, 1008) and Timestamp.valueOf (PrimitiveObjectInspectorUtils line 1099). Timestamp.valueOf supports optional milliseconds part (from 1 to 9 digits) On Feb. 26, 2015, 7:46 a.m., Jason Dere wrote: ql/src/test/results/clientpositive/udf_last_day.q.out, line 67 https://reviews.apache.org/r/31404/diff/1/?file=875155#file875155line67 What's the cause of the output difference here? I use ObjectInspectorConverters.getConverter to get DateConverters and TimestempConverters to convert string to Date/Timestamp. I noticed that DateConverter can convert 2014-02-30 to non-null Date but 2014-02-32 is converted to null. In this particular case the function deals with 2014-01-34. DateConverter converts it to null (which is right because 2014-01-34 is not valid date). - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31404/#review74226 --- On Feb. 25, 2015, 3:57 a.m., Alexander Pivovarov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31404/ --- (Updated Feb. 25, 2015, 3:57 a.m.) Review request for hive, Jason Dere and Thejas Nair. Bugs: HIVE-9744 https://issues.apache.org/jira/browse/HIVE-9744 Repository: hive-git Description --- HIVE-9744 Move common arguments validation and value extraction code to GenericUDF Diffs - ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java 8a0f573648c51c4945be8ffec4a0b06dfa7061c8 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFAddMonths.java c5968835a74195bea6b31a5c7b7346907fed5ce0 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFInitCap.java 406fcd608a13fadb8902bf273932acb05a0f3bbe ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLastDay.java 3a43c571ae3a83924a00413181a62ce6f4408125 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLevenstein.java de41793ba3925aa9e1ad9623d92881c57791f047 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFNextDay.java 38f08b74609a4018221ca3f5b92cf33799604d60 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFAddMonths.java 4ccae97a227257294d69f728426f425d060ef0c7 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFLevenshtein.java e674d9f38cf7b5cdffcad6eca07dba74ff1e834b ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFNextDay.java e2ec551d4ae39d521680ee93c791f14f27811270 ql/src/test/queries/clientpositive/udf_next_day.q db821f00c39b84c4bc59ea70b0cc0aacf69cbd18 ql/src/test/results/clientnegative/udf_add_months_error_1.q.out 8226ac6fe89c38fcc14edeea215cd5cce7258683 ql/src/test/results/clientnegative/udf_add_months_error_2.q.out f00949e9a12285cc91032215372975753c1f3b4a ql/src/test/results/clientnegative/udf_last_day_error_1.q.out 6e718a0c15e84d89b1cfe7f36231e472ff03c37f ql/src/test/results/clientnegative/udf_last_day_error_2.q.out dc8e3d14f14205ce65355cd53a95cfc788f45fe0 ql/src/test/results/clientnegative/udf_next_day_error_1.q.out c67b9c42f7e7fdf20caa34d028b02fd4819e8343 ql/src/test/results/clientnegative/udf_next_day_error_2.q.out e3cb6a447bf7bd9f09648e46ace1bcba4da55339 ql/src/test/results/clientpositive/udf_add_months.q.out 8c37fc282a25e350435ff6a4b39e02fc2d4f17df ql/src/test/results/clientpositive/udf_last_day.q.out 2d39e3897625c7651b110779b89652f0d785bc92 ql/src/test/results/clientpositive/udf_next_day.q.out 37f5640fde267f1635d72f23e76fa89de6403ab5 Diff: https://reviews.apache.org/r/31404/diff/ Testing --- Thanks, Alexander Pivovarov
Re: Review Request 31404: HIVE-9744 Move common arguments validation and value extraction code to GenericUDF
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31404/ --- (Updated Feb. 26, 2015, 9:16 p.m.) Review request for hive, Jason Dere and Thejas Nair. Bugs: HIVE-9744 https://issues.apache.org/jira/browse/HIVE-9744 Repository: hive-git Description --- HIVE-9744 Move common arguments validation and value extraction code to GenericUDF Diffs (updated) - ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java 8a0f573648c51c4945be8ffec4a0b06dfa7061c8 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFAddMonths.java c5968835a74195bea6b31a5c7b7346907fed5ce0 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFInitCap.java 406fcd608a13fadb8902bf273932acb05a0f3bbe ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLastDay.java 3a43c571ae3a83924a00413181a62ce6f4408125 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLevenstein.java de41793ba3925aa9e1ad9623d92881c57791f047 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFNextDay.java 38f08b74609a4018221ca3f5b92cf33799604d60 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFAddMonths.java 4ccae97a227257294d69f728426f425d060ef0c7 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFLastDay.java 4b233a6966bbdf6902c53f2aaf53cc0eb422b205 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFLevenshtein.java e674d9f38cf7b5cdffcad6eca07dba74ff1e834b ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFNextDay.java e2ec551d4ae39d521680ee93c791f14f27811270 ql/src/test/queries/clientpositive/udf_next_day.q db821f00c39b84c4bc59ea70b0cc0aacf69cbd18 ql/src/test/results/clientnegative/udf_add_months_error_1.q.out 8226ac6fe89c38fcc14edeea215cd5cce7258683 ql/src/test/results/clientnegative/udf_add_months_error_2.q.out f00949e9a12285cc91032215372975753c1f3b4a ql/src/test/results/clientnegative/udf_last_day_error_1.q.out 6e718a0c15e84d89b1cfe7f36231e472ff03c37f ql/src/test/results/clientnegative/udf_last_day_error_2.q.out dc8e3d14f14205ce65355cd53a95cfc788f45fe0 ql/src/test/results/clientnegative/udf_next_day_error_1.q.out c67b9c42f7e7fdf20caa34d028b02fd4819e8343 ql/src/test/results/clientnegative/udf_next_day_error_2.q.out e3cb6a447bf7bd9f09648e46ace1bcba4da55339 ql/src/test/results/clientpositive/udf_add_months.q.out 8c37fc282a25e350435ff6a4b39e02fc2d4f17df ql/src/test/results/clientpositive/udf_last_day.q.out 2d39e3897625c7651b110779b89652f0d785bc92 Diff: https://reviews.apache.org/r/31404/diff/ Testing --- Thanks, Alexander Pivovarov
Re: Review Request 31404: HIVE-9744 Move common arguments validation and value extraction code to GenericUDF
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31404/#review74371 --- ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java https://reviews.apache.org/r/31404/#comment120934 Not a fan of these timestamp-related changes. It's causing you to have to create 2 new arrays of Converters which seems awkward. I was going to suggest simply using a DateConverter, but I see that the original behavior also allowed timestamp format strings (really, it allowed anything after the '-mm-dd' part). Rather than have 2 different converters, maybe just go back to what you had before, use a single converter to string, and then just apply whatever format logic you are going to apply to the string value. ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFNextDay.java https://reviews.apache.org/r/31404/#comment120945 So this is actually an example of something that used to be valid input, but no longer works due to the change from SimpleDateFormat to Timestamp/Date converter. This would be ok for the UDFs you've changed since they are new ones, but I don't know if this kind of change in behavior would be something that would be a concern if we were to apply this same refactoring to some of the other date-related UDFs. - Jason Dere On Feb. 26, 2015, 9:16 p.m., Alexander Pivovarov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31404/ --- (Updated Feb. 26, 2015, 9:16 p.m.) Review request for hive, Jason Dere and Thejas Nair. Bugs: HIVE-9744 https://issues.apache.org/jira/browse/HIVE-9744 Repository: hive-git Description --- HIVE-9744 Move common arguments validation and value extraction code to GenericUDF Diffs - ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java 8a0f573648c51c4945be8ffec4a0b06dfa7061c8 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFAddMonths.java c5968835a74195bea6b31a5c7b7346907fed5ce0 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFInitCap.java 406fcd608a13fadb8902bf273932acb05a0f3bbe ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLastDay.java 3a43c571ae3a83924a00413181a62ce6f4408125 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLevenstein.java de41793ba3925aa9e1ad9623d92881c57791f047 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFNextDay.java 38f08b74609a4018221ca3f5b92cf33799604d60 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFAddMonths.java 4ccae97a227257294d69f728426f425d060ef0c7 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFLastDay.java 4b233a6966bbdf6902c53f2aaf53cc0eb422b205 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFLevenshtein.java e674d9f38cf7b5cdffcad6eca07dba74ff1e834b ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFNextDay.java e2ec551d4ae39d521680ee93c791f14f27811270 ql/src/test/queries/clientpositive/udf_next_day.q db821f00c39b84c4bc59ea70b0cc0aacf69cbd18 ql/src/test/results/clientnegative/udf_add_months_error_1.q.out 8226ac6fe89c38fcc14edeea215cd5cce7258683 ql/src/test/results/clientnegative/udf_add_months_error_2.q.out f00949e9a12285cc91032215372975753c1f3b4a ql/src/test/results/clientnegative/udf_last_day_error_1.q.out 6e718a0c15e84d89b1cfe7f36231e472ff03c37f ql/src/test/results/clientnegative/udf_last_day_error_2.q.out dc8e3d14f14205ce65355cd53a95cfc788f45fe0 ql/src/test/results/clientnegative/udf_next_day_error_1.q.out c67b9c42f7e7fdf20caa34d028b02fd4819e8343 ql/src/test/results/clientnegative/udf_next_day_error_2.q.out e3cb6a447bf7bd9f09648e46ace1bcba4da55339 ql/src/test/results/clientpositive/udf_add_months.q.out 8c37fc282a25e350435ff6a4b39e02fc2d4f17df ql/src/test/results/clientpositive/udf_last_day.q.out 2d39e3897625c7651b110779b89652f0d785bc92 Diff: https://reviews.apache.org/r/31404/diff/ Testing --- Thanks, Alexander Pivovarov
Re: Review Request 31404: HIVE-9744 Move common arguments validation and value extraction code to GenericUDF
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31404/ --- (Updated Feb. 27, 2015, 12:11 a.m.) Review request for hive, Jason Dere and Thejas Nair. Bugs: HIVE-9744 https://issues.apache.org/jira/browse/HIVE-9744 Repository: hive-git Description --- HIVE-9744 Move common arguments validation and value extraction code to GenericUDF Diffs (updated) - ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java 8a0f573648c51c4945be8ffec4a0b06dfa7061c8 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFAddMonths.java c5968835a74195bea6b31a5c7b7346907fed5ce0 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFInitCap.java 406fcd608a13fadb8902bf273932acb05a0f3bbe ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLastDay.java 3a43c571ae3a83924a00413181a62ce6f4408125 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLevenstein.java de41793ba3925aa9e1ad9623d92881c57791f047 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFNextDay.java 38f08b74609a4018221ca3f5b92cf33799604d60 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFAddMonths.java 4ccae97a227257294d69f728426f425d060ef0c7 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFLastDay.java 4b233a6966bbdf6902c53f2aaf53cc0eb422b205 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFLevenshtein.java e674d9f38cf7b5cdffcad6eca07dba74ff1e834b ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFNextDay.java e2ec551d4ae39d521680ee93c791f14f27811270 ql/src/test/results/clientnegative/udf_add_months_error_1.q.out 8226ac6fe89c38fcc14edeea215cd5cce7258683 ql/src/test/results/clientnegative/udf_add_months_error_2.q.out f00949e9a12285cc91032215372975753c1f3b4a ql/src/test/results/clientnegative/udf_last_day_error_1.q.out 6e718a0c15e84d89b1cfe7f36231e472ff03c37f ql/src/test/results/clientnegative/udf_last_day_error_2.q.out dc8e3d14f14205ce65355cd53a95cfc788f45fe0 ql/src/test/results/clientnegative/udf_next_day_error_1.q.out c67b9c42f7e7fdf20caa34d028b02fd4819e8343 ql/src/test/results/clientnegative/udf_next_day_error_2.q.out e3cb6a447bf7bd9f09648e46ace1bcba4da55339 Diff: https://reviews.apache.org/r/31404/diff/ Testing --- Thanks, Alexander Pivovarov
Re: Review Request 31404: HIVE-9744 Move common arguments validation and value extraction code to GenericUDF
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31404/#review74226 --- ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java https://reviews.apache.org/r/31404/#comment120755 Why go away from using the formatter as it was doing before? Are there any ramifications here to switching to Timestamp.valueOf()? I think Timestamp.valueOf() is probably less forgiving when checking the format. ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java https://reviews.apache.org/r/31404/#comment120750 I'm not sure if all of this is actally necesary, at least at the time that you are trying to retrieve a value from a converter. If you created a Converter with PrimitiveObjectInspectorFactory.writableLongObjectInspector specified as the output type, LongConverter.convert() already has a similar switch statement that will do the necessary conversion to give you back a LongWritable object. So with a properly created Converter, you would just need to call (LongWritable) converter.convert(arguments[i].get()) It would probably be a good idea to still do this checking when creating the Converter object. ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java https://reviews.apache.org/r/31404/#comment120748 Does this need to check for null values, or does the caller check for them before calling this method? ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFAddMonths.java https://reviews.apache.org/r/31404/#comment120753 Rather than having to convert from long to int (and risk overflow), just accept byte/short/int here, and leave the caller of the UDF with the option to explicitly cast their long parameter to int if they really want it to work with the UDF. ql/src/test/results/clientpositive/udf_last_day.q.out https://reviews.apache.org/r/31404/#comment120756 What's the cause of the output difference here? - Jason Dere On Feb. 25, 2015, 3:57 a.m., Alexander Pivovarov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31404/ --- (Updated Feb. 25, 2015, 3:57 a.m.) Review request for hive, Jason Dere and Thejas Nair. Bugs: HIVE-9744 https://issues.apache.org/jira/browse/HIVE-9744 Repository: hive-git Description --- HIVE-9744 Move common arguments validation and value extraction code to GenericUDF Diffs - ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java 8a0f573648c51c4945be8ffec4a0b06dfa7061c8 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFAddMonths.java c5968835a74195bea6b31a5c7b7346907fed5ce0 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFInitCap.java 406fcd608a13fadb8902bf273932acb05a0f3bbe ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLastDay.java 3a43c571ae3a83924a00413181a62ce6f4408125 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLevenstein.java de41793ba3925aa9e1ad9623d92881c57791f047 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFNextDay.java 38f08b74609a4018221ca3f5b92cf33799604d60 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFAddMonths.java 4ccae97a227257294d69f728426f425d060ef0c7 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFLevenshtein.java e674d9f38cf7b5cdffcad6eca07dba74ff1e834b ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFNextDay.java e2ec551d4ae39d521680ee93c791f14f27811270 ql/src/test/queries/clientpositive/udf_next_day.q db821f00c39b84c4bc59ea70b0cc0aacf69cbd18 ql/src/test/results/clientnegative/udf_add_months_error_1.q.out 8226ac6fe89c38fcc14edeea215cd5cce7258683 ql/src/test/results/clientnegative/udf_add_months_error_2.q.out f00949e9a12285cc91032215372975753c1f3b4a ql/src/test/results/clientnegative/udf_last_day_error_1.q.out 6e718a0c15e84d89b1cfe7f36231e472ff03c37f ql/src/test/results/clientnegative/udf_last_day_error_2.q.out dc8e3d14f14205ce65355cd53a95cfc788f45fe0 ql/src/test/results/clientnegative/udf_next_day_error_1.q.out c67b9c42f7e7fdf20caa34d028b02fd4819e8343 ql/src/test/results/clientnegative/udf_next_day_error_2.q.out e3cb6a447bf7bd9f09648e46ace1bcba4da55339 ql/src/test/results/clientpositive/udf_add_months.q.out 8c37fc282a25e350435ff6a4b39e02fc2d4f17df ql/src/test/results/clientpositive/udf_last_day.q.out 2d39e3897625c7651b110779b89652f0d785bc92 ql/src/test/results/clientpositive/udf_next_day.q.out 37f5640fde267f1635d72f23e76fa89de6403ab5 Diff: https://reviews.apache.org/r/31404/diff/ Testing --- Thanks, Alexander Pivovarov
Review Request 31404: HIVE-9744 Move common arguments validation and value extraction code to GenericUDF
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31404/ --- Review request for hive and Jason Dere. Bugs: HIVE-9744 https://issues.apache.org/jira/browse/HIVE-9744 Repository: hive-git Description --- HIVE-9744 Move common arguments validation and value extraction code to GenericUDF Diffs - ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java 8a0f573648c51c4945be8ffec4a0b06dfa7061c8 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFAddMonths.java c5968835a74195bea6b31a5c7b7346907fed5ce0 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFInitCap.java 406fcd608a13fadb8902bf273932acb05a0f3bbe ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLastDay.java 3a43c571ae3a83924a00413181a62ce6f4408125 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFLevenstein.java de41793ba3925aa9e1ad9623d92881c57791f047 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFNextDay.java 38f08b74609a4018221ca3f5b92cf33799604d60 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFAddMonths.java 4ccae97a227257294d69f728426f425d060ef0c7 ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFLevenshtein.java e674d9f38cf7b5cdffcad6eca07dba74ff1e834b ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFNextDay.java e2ec551d4ae39d521680ee93c791f14f27811270 ql/src/test/queries/clientpositive/udf_next_day.q db821f00c39b84c4bc59ea70b0cc0aacf69cbd18 ql/src/test/results/clientnegative/udf_add_months_error_1.q.out 8226ac6fe89c38fcc14edeea215cd5cce7258683 ql/src/test/results/clientnegative/udf_add_months_error_2.q.out f00949e9a12285cc91032215372975753c1f3b4a ql/src/test/results/clientnegative/udf_last_day_error_1.q.out 6e718a0c15e84d89b1cfe7f36231e472ff03c37f ql/src/test/results/clientnegative/udf_last_day_error_2.q.out dc8e3d14f14205ce65355cd53a95cfc788f45fe0 ql/src/test/results/clientnegative/udf_next_day_error_1.q.out c67b9c42f7e7fdf20caa34d028b02fd4819e8343 ql/src/test/results/clientnegative/udf_next_day_error_2.q.out e3cb6a447bf7bd9f09648e46ace1bcba4da55339 ql/src/test/results/clientpositive/udf_add_months.q.out 8c37fc282a25e350435ff6a4b39e02fc2d4f17df ql/src/test/results/clientpositive/udf_last_day.q.out 2d39e3897625c7651b110779b89652f0d785bc92 ql/src/test/results/clientpositive/udf_next_day.q.out 37f5640fde267f1635d72f23e76fa89de6403ab5 Diff: https://reviews.apache.org/r/31404/diff/ Testing --- Thanks, Alexander Pivovarov
