You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Peter Vary via Review Board <no...@reviews.apache.org> on 2018/05/11 10:08:12 UTC
Re: Review Request 67073: HIVE-19370 : Retain time part in add_months
function on timestamp datatype fields in hive
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67073/#review202907
-----------------------------------------------------------
Hi Bharath,
Thanks for the patch!
Few quick questions there (not too familiar with this part of the code yet)
Thanks,
Peter
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFAddMonths.java
Lines 74 (patched)
<https://reviews.apache.org/r/67073/#comment284958>
Do we think this is a better approach then the one used by GenericUDFMonthsBetween, and GenericUDFDateFormat? Are we sure that we know the exact primitive categories we can convert? My guess that we originally accepted string as an input... What happens then?
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFAddMonths.java
Lines 111 (patched)
<https://reviews.apache.org/r/67073/#comment284956>
Why do we need this conversion?
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFAddMonths.java
Lines 130 (patched)
<https://reviews.apache.org/r/67073/#comment284961>
This code line suggests to me that we accept strings as a first input:
checkArgGroups(arguments, 0, inputTypes, STRING_GROUP, DATE_GROUP, VOID_GROUP);
What happens when the first input is string?
- Peter Vary
On May 10, 2018, 9:55 p.m., Bharathkrishna Guruvayoor Murali wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67073/
> -----------------------------------------------------------
>
> (Updated May 10, 2018, 9:55 p.m.)
>
>
> Review request for hive, Peter Vary, Sahil Takiar, and Vihang Karajgaonkar.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Adding support to retain the time part (HH:mm:ss) for add_months UDF when the input is given as timestamp format.
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hive/common/util/DateUtils.java 65f3b9401916abdfa52fbf75d115ba6b61758fb0
> ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFAddMonths.java dae4b97b4a17e98122431e5fda655fd9f873fdb5
> ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFAddMonths.java af9b6c43c7dafc69c4944eab02894786af306f35
>
>
> Diff: https://reviews.apache.org/r/67073/diff/1/
>
>
> Testing
> -------
>
> Added unit tests.
>
>
> Thanks,
>
> Bharathkrishna Guruvayoor Murali
>
>
Re: Review Request 67073: HIVE-19370 : Retain time part in add_months
function on timestamp datatype fields in hive
Posted by Bharathkrishna Guruvayoor Murali via Review Board <no...@reviews.apache.org>.
> On May 11, 2018, 10:08 a.m., Peter Vary wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFAddMonths.java
> > Lines 74 (patched)
> > <https://reviews.apache.org/r/67073/diff/1/?file=2019552#file2019552line74>
> >
> > Do we think this is a better approach then the one used by GenericUDFMonthsBetween, and GenericUDFDateFormat? Are we sure that we know the exact primitive categories we can convert? My guess that we originally accepted string as an input... What happens then?
Do you mean that we need to retain the time part also in the case when input type is String?
My patch would only retain time part if input type is timestamp.
- Bharathkrishna
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67073/#review202907
-----------------------------------------------------------
On May 10, 2018, 9:55 p.m., Bharathkrishna Guruvayoor Murali wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67073/
> -----------------------------------------------------------
>
> (Updated May 10, 2018, 9:55 p.m.)
>
>
> Review request for hive, Peter Vary, Sahil Takiar, and Vihang Karajgaonkar.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Adding support to retain the time part (HH:mm:ss) for add_months UDF when the input is given as timestamp format.
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hive/common/util/DateUtils.java 65f3b9401916abdfa52fbf75d115ba6b61758fb0
> ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFAddMonths.java dae4b97b4a17e98122431e5fda655fd9f873fdb5
> ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFAddMonths.java af9b6c43c7dafc69c4944eab02894786af306f35
>
>
> Diff: https://reviews.apache.org/r/67073/diff/1/
>
>
> Testing
> -------
>
> Added unit tests.
>
>
> Thanks,
>
> Bharathkrishna Guruvayoor Murali
>
>
Re: Review Request 67073: HIVE-19370 : Retain time part in add_months
function on timestamp datatype fields in hive
Posted by Peter Vary via Review Board <no...@reviews.apache.org>.
> On May 11, 2018, 10:08 a.m., Peter Vary wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFAddMonths.java
> > Lines 74 (patched)
> > <https://reviews.apache.org/r/67073/diff/1/?file=2019552#file2019552line74>
> >
> > Do we think this is a better approach then the one used by GenericUDFMonthsBetween, and GenericUDFDateFormat? Are we sure that we know the exact primitive categories we can convert? My guess that we originally accepted string as an input... What happens then?
>
> Bharathkrishna Guruvayoor Murali wrote:
> Do you mean that we need to retain the time part also in the case when input type is String?
> My patch would only retain time part if input type is timestamp.
I mean, that GenericUDFMonthsBetween, and GenericUDFDateFormat also handles according the the comments. See:
"
// the function should support both short date and full timestamp format
// time part of the timestamp should not be skipped
Date date = getTimestampValue(arguments, 0, tsConverters);
if (date == null) {
date = getDateValue(arguments, 0, dtInputTypes, dtConverters);
if (date == null) {
return null;
}
}
"
In both cases it uses this approach to first try timestamp, and if that fails trie date. This way it does not have to create separate cases for different primitive types
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67073/#review202907
-----------------------------------------------------------
On May 10, 2018, 9:55 p.m., Bharathkrishna Guruvayoor Murali wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67073/
> -----------------------------------------------------------
>
> (Updated May 10, 2018, 9:55 p.m.)
>
>
> Review request for hive, Peter Vary, Sahil Takiar, and Vihang Karajgaonkar.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Adding support to retain the time part (HH:mm:ss) for add_months UDF when the input is given as timestamp format.
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hive/common/util/DateUtils.java 65f3b9401916abdfa52fbf75d115ba6b61758fb0
> ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFAddMonths.java dae4b97b4a17e98122431e5fda655fd9f873fdb5
> ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFAddMonths.java af9b6c43c7dafc69c4944eab02894786af306f35
>
>
> Diff: https://reviews.apache.org/r/67073/diff/1/
>
>
> Testing
> -------
>
> Added unit tests.
>
>
> Thanks,
>
> Bharathkrishna Guruvayoor Murali
>
>