hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jason Dere" <jd...@hortonworks.com>
Subject Re: Review Request 31404: HIVE-9744 Move common arguments validation and value extraction code to GenericUDF
Date Thu, 26 Feb 2015 07:46:01 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message