hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander Pivovarov" <apivova...@gmail.com>
Subject Re: Review Request 32489: HIVE-9518 Implement MONTHS_BETWEEN aligned with Oracle one
Date Fri, 27 Mar 2015 18:56:33 GMT


> On March 27, 2015, 6:08 p.m., Mohit Sabharwal wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java, line 507
> > <https://reviews.apache.org/r/32489/diff/3/?file=907259#file907259line507>
> >
> >     19...a comment on where this comes from would be great
> 
> Alexander Pivovarov wrote:
>     added comment
>           // lets be strict here and
>           // support only exact 10 char string for short date format

getTimestampValue method was added recently as part the activity to add common methods to
GenericUDF to validate/extract values.
getTimestampValue uses sql.Timestamp.getValue(str) internaly to parse input strings.
sql.Timestamp.getValue supports only string which are 19 chars and more.

BTW, getTimestampValue was not used by any UDF before. GenericUDFMonthsBetween is the first
function which uses it.
I think it's quite common situation when UDF should support both short date yyyy-MM-dd and
long date+time string formats and not skip time part
>From the other side we need to decid what to do with incorrect strings which contains
just hour and minutes for example "2015-03-27 10:30"
original getTimestampValue will return null

The change which I added yesterday - "if (dateStr.length() < 19)" will return not null
value but time part will be skipped
I think it might be confused for end users. They might expect that if function returs not
null value then it parses partial time part of the string.

This is why today I decided to return null for such incorrect strings. I think it's less confused
to end users.

So, I only left support for exact short date format which is 10 chars yyyy-MM-dd


- Alexander


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32489/#review78088
-----------------------------------------------------------


On March 27, 2015, 6:35 p.m., Alexander Pivovarov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32489/
> -----------------------------------------------------------
> 
> (Updated March 27, 2015, 6:35 p.m.)
> 
> 
> Review request for hive and Jason Dere.
> 
> 
> Bugs: HIVE-9518
>     https://issues.apache.org/jira/browse/HIVE-9518
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> HIVE-9518 Implement MONTHS_BETWEEN aligned with Oracle one
> 
> 
> Diffs
> -----
> 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java 2476e832b8b7101971ea2226368aa82633b7e7d1

>   ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java ce981232382e993c7c9d640efe9b2d21f70a0ed4

>   ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFMonthsBetween.java PRE-CREATION

>   ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFMonthsBetween.java
PRE-CREATION 
>   ql/src/test/queries/clientpositive/udf_months_between.q PRE-CREATION 
>   ql/src/test/results/clientpositive/show_functions.q.out 22091d06241218a5c0ee21d6ee6be00a71706971

>   ql/src/test/results/clientpositive/udf_months_between.q.out PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/32489/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alexander Pivovarov
> 
>


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