You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@tajo.apache.org by Alvin Henrick <al...@aol.com> on 2014/03/21 03:25:01 UTC
Review Request 19513: TAJO-684 Add functions about time
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19513/
-----------------------------------------------------------
Review request for Tajo and Hyunsik Choi.
Repository: tajo
Description
-------
Add functions about time
Diffs
-----
tajo-common/src/main/java/org/apache/tajo/datum/UnixTimeStampDatum.java PRE-CREATION
tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/datetime/DateTimePartFromUnixTimeStamp.java PRE-CREATION
tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/function/TestDateTimeFunctions.java b882e84
Diff: https://reviews.apache.org/r/19513/diff/
Testing
-------
Thanks,
Alvin Henrick
Re: Review Request 19513: TAJO-684 Add functions about time
Posted by Alvin Henrick <sh...@aol.com>.
> On March 25, 2014, 4:32 p.m., Jihoon Son wrote:
> > +1.
> > This patch looks good to me.
> > Thanks for your contribution, Alvin!
Thank you Jihoon for reviewing the code.
- Alvin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19513/#review38444
-----------------------------------------------------------
On March 23, 2014, 11:42 p.m., Alvin Henrick wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19513/
> -----------------------------------------------------------
>
> (Updated March 23, 2014, 11:42 p.m.)
>
>
> Review request for Tajo and Hyunsik Choi.
>
>
> Bugs: TAJO-684
> https://issues.apache.org/jira/browse/TAJO-684
>
>
> Repository: tajo
>
>
> Description
> -------
>
> Add functions about time
>
>
> Diffs
> -----
>
> tajo-common/src/main/java/org/apache/tajo/util/TimeStampUtil.java PRE-CREATION
> tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/datetime/DateTimePartFromUnixTimeStamp.java PRE-CREATION
> tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/function/TestDateTimeFunctions.java b882e84
>
> Diff: https://reviews.apache.org/r/19513/diff/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> TAJO-684-Patch
> https://reviews.apache.org/media/uploaded/files/2014/03/21/c8117c71-5a01-495d-ae9b-eb78d43f323a__TAJO-684-With-ASF-Unit-Test.patch
>
>
> Thanks,
>
> Alvin Henrick
>
>
Re: Review Request 19513: TAJO-684 Add functions about time
Posted by Jihoon Son <ji...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19513/#review38444
-----------------------------------------------------------
Ship it!
+1.
This patch looks good to me.
Thanks for your contribution, Alvin!
- Jihoon Son
On March 24, 2014, 8:42 a.m., Alvin Henrick wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19513/
> -----------------------------------------------------------
>
> (Updated March 24, 2014, 8:42 a.m.)
>
>
> Review request for Tajo and Hyunsik Choi.
>
>
> Bugs: TAJO-684
> https://issues.apache.org/jira/browse/TAJO-684
>
>
> Repository: tajo
>
>
> Description
> -------
>
> Add functions about time
>
>
> Diffs
> -----
>
> tajo-common/src/main/java/org/apache/tajo/util/TimeStampUtil.java PRE-CREATION
> tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/datetime/DateTimePartFromUnixTimeStamp.java PRE-CREATION
> tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/function/TestDateTimeFunctions.java b882e84
>
> Diff: https://reviews.apache.org/r/19513/diff/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> TAJO-684-Patch
> https://reviews.apache.org/media/uploaded/files/2014/03/21/c8117c71-5a01-495d-ae9b-eb78d43f323a__TAJO-684-With-ASF-Unit-Test.patch
>
>
> Thanks,
>
> Alvin Henrick
>
>
Re: Review Request 19513: TAJO-684 Add functions about time
Posted by Alvin Henrick <sh...@aol.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19513/
-----------------------------------------------------------
(Updated March 23, 2014, 11:42 p.m.)
Review request for Tajo and Hyunsik Choi.
Changes
-------
The suggestions from Jihoon implemented.
Bugs: TAJO-684
https://issues.apache.org/jira/browse/TAJO-684
Repository: tajo
Description
-------
Add functions about time
Diffs (updated)
-----
tajo-common/src/main/java/org/apache/tajo/util/TimeStampUtil.java PRE-CREATION
tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/datetime/DateTimePartFromUnixTimeStamp.java PRE-CREATION
tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/function/TestDateTimeFunctions.java b882e84
Diff: https://reviews.apache.org/r/19513/diff/
Testing
-------
File Attachments
----------------
TAJO-684-Patch
https://reviews.apache.org/media/uploaded/files/2014/03/21/c8117c71-5a01-495d-ae9b-eb78d43f323a__TAJO-684-With-ASF-Unit-Test.patch
Thanks,
Alvin Henrick
Re: Review Request 19513: TAJO-684 Add functions about time
Posted by Alvin Henrick <sh...@aol.com>.
> On March 23, 2014, 5:48 a.m., Jihoon Son wrote:
> > Hi, Alvin.
> > Since I already give +1 on JIRA, I'm very sorry to say to suggest as follows.
> >
> > You proposed UnixTimeStampDatum to use in DateTimePartFromUnixTimeStamp. As you said, UnixTimeStampDaum is based on the Unix time.
> > However, TimestampDatum which is the default type for timestamps is also based on the Unix time. So, in my opinion, TimestampDatum and UnixTimeStampDatum are fundamentally same, but provide different functions.
> > For more detailed differences between them, UnixTimeStampDatum provides specific functions to approximate DateTime based on a given time unit (year, month, day, ...), while the functions of TimestampDatum extract a certain portion from DateTime (year, monthOfYear, DayOfWeek, ...).
> > I think that the approximation functions of UnixTimeStampDatum are very useful, but may be used only in DateTimePartFromUnixTimeStamp.
> >
> > So, how about moving the approximation functions of UnixTimeStampDatum to a utility class such as TimestampUtil?
> > I think that putting functions for a specific operation together will increase the readability and make users less confusing.
> >
> > Again, I truly apologize for changing my former opinion in JIRA.
Hi Jihoon,
Please don't be sorry... :) I was wondering why it was accepted in the first place because you are absolutely right.I just followed the pattern the way other Date and Time Datums were implemented to be on the safer side and I myself was debating am I doing this right because TimestampDatum already exist but the behavior of the functions were so different I created the UnixTimeStampDatum like day in TimestampDatum returns the day (MON , TUE and so on) and the day in UnixTimeStampDatum shifts it to start of the day as per Google Big Query implementation.
It is a better idea to move the function to TimestampUtil and remove UnixTimeStampDatum class.I will update the patch as per the suggestion.
Thanks!
Warm Regards,
Alvin.
- Alvin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19513/#review38249
-----------------------------------------------------------
On March 23, 2014, 5 a.m., Alvin Henrick wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19513/
> -----------------------------------------------------------
>
> (Updated March 23, 2014, 5 a.m.)
>
>
> Review request for Tajo and Hyunsik Choi.
>
>
> Bugs: TAJO-684
> https://issues.apache.org/jira/browse/TAJO-684
>
>
> Repository: tajo
>
>
> Description
> -------
>
> Add functions about time
>
>
> Diffs
> -----
>
> tajo-common/src/main/java/org/apache/tajo/datum/UnixTimeStampDatum.java PRE-CREATION
> tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/datetime/DateTimePartFromUnixTimeStamp.java PRE-CREATION
> tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/function/TestDateTimeFunctions.java b882e84
>
> Diff: https://reviews.apache.org/r/19513/diff/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> TAJO-684-Patch
> https://reviews.apache.org/media/uploaded/files/2014/03/21/c8117c71-5a01-495d-ae9b-eb78d43f323a__TAJO-684-With-ASF-Unit-Test.patch
>
>
> Thanks,
>
> Alvin Henrick
>
>
Re: Review Request 19513: TAJO-684 Add functions about time
Posted by Jihoon Son <ji...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19513/#review38249
-----------------------------------------------------------
Hi, Alvin.
Since I already give +1 on JIRA, I'm very sorry to say to suggest as follows.
You proposed UnixTimeStampDatum to use in DateTimePartFromUnixTimeStamp. As you said, UnixTimeStampDaum is based on the Unix time.
However, TimestampDatum which is the default type for timestamps is also based on the Unix time. So, in my opinion, TimestampDatum and UnixTimeStampDatum are fundamentally same, but provide different functions.
For more detailed differences between them, UnixTimeStampDatum provides specific functions to approximate DateTime based on a given time unit (year, month, day, ...), while the functions of TimestampDatum extract a certain portion from DateTime (year, monthOfYear, DayOfWeek, ...).
I think that the approximation functions of UnixTimeStampDatum are very useful, but may be used only in DateTimePartFromUnixTimeStamp.
So, how about moving the approximation functions of UnixTimeStampDatum to a utility class such as TimestampUtil?
I think that putting functions for a specific operation together will increase the readability and make users less confusing.
Again, I truly apologize for changing my former opinion in JIRA.
- Jihoon Son
On March 23, 2014, 2 p.m., Alvin Henrick wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19513/
> -----------------------------------------------------------
>
> (Updated March 23, 2014, 2 p.m.)
>
>
> Review request for Tajo and Hyunsik Choi.
>
>
> Bugs: TAJO-684
> https://issues.apache.org/jira/browse/TAJO-684
>
>
> Repository: tajo
>
>
> Description
> -------
>
> Add functions about time
>
>
> Diffs
> -----
>
> tajo-common/src/main/java/org/apache/tajo/datum/UnixTimeStampDatum.java PRE-CREATION
> tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/datetime/DateTimePartFromUnixTimeStamp.java PRE-CREATION
> tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/function/TestDateTimeFunctions.java b882e84
>
> Diff: https://reviews.apache.org/r/19513/diff/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> TAJO-684-Patch
> https://reviews.apache.org/media/uploaded/files/2014/03/21/c8117c71-5a01-495d-ae9b-eb78d43f323a__TAJO-684-With-ASF-Unit-Test.patch
>
>
> Thanks,
>
> Alvin Henrick
>
>
Re: Review Request 19513: TAJO-684 Add functions about time
Posted by Alvin Henrick <sh...@aol.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19513/
-----------------------------------------------------------
(Updated March 23, 2014, 5 a.m.)
Review request for Tajo and Hyunsik Choi.
Changes
-------
adding bug details
Bugs: TAJO-684
https://issues.apache.org/jira/browse/TAJO-684
Repository: tajo
Description
-------
Add functions about time
Diffs
-----
tajo-common/src/main/java/org/apache/tajo/datum/UnixTimeStampDatum.java PRE-CREATION
tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/datetime/DateTimePartFromUnixTimeStamp.java PRE-CREATION
tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/function/TestDateTimeFunctions.java b882e84
Diff: https://reviews.apache.org/r/19513/diff/
Testing
-------
File Attachments
----------------
TAJO-684-Patch
https://reviews.apache.org/media/uploaded/files/2014/03/21/c8117c71-5a01-495d-ae9b-eb78d43f323a__TAJO-684-With-ASF-Unit-Test.patch
Thanks,
Alvin Henrick
Re: Review Request 19513: TAJO-684 Add functions about time
Posted by Alvin Henrick <sh...@aol.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19513/
-----------------------------------------------------------
(Updated March 23, 2014, 4:59 a.m.)
Review request for Tajo and Hyunsik Choi.
Repository: tajo
Description
-------
Add functions about time
Diffs
-----
tajo-common/src/main/java/org/apache/tajo/datum/UnixTimeStampDatum.java PRE-CREATION
tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/datetime/DateTimePartFromUnixTimeStamp.java PRE-CREATION
tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/function/TestDateTimeFunctions.java b882e84
Diff: https://reviews.apache.org/r/19513/diff/
Testing
-------
File Attachments
----------------
TAJO-684-Patch
https://reviews.apache.org/media/uploaded/files/2014/03/21/c8117c71-5a01-495d-ae9b-eb78d43f323a__TAJO-684-With-ASF-Unit-Test.patch
Thanks,
Alvin Henrick
Re: Review Request 19513: TAJO-684 Add functions about time
Posted by Alvin Henrick <al...@aol.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19513/
-----------------------------------------------------------
(Updated March 21, 2014, 2:32 a.m.)
Review request for Tajo and Hyunsik Choi.
Changes
-------
Uploading Diff.
Repository: tajo
Description
-------
Add functions about time
Diffs (updated)
-----
tajo-common/src/main/java/org/apache/tajo/datum/UnixTimeStampDatum.java PRE-CREATION
tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/datetime/DateTimePartFromUnixTimeStamp.java PRE-CREATION
tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/function/TestDateTimeFunctions.java b882e84
Diff: https://reviews.apache.org/r/19513/diff/
Testing
-------
File Attachments
----------------
TAJO-684-Patch
https://reviews.apache.org/media/uploaded/files/2014/03/21/c8117c71-5a01-495d-ae9b-eb78d43f323a__TAJO-684-With-ASF-Unit-Test.patch
Thanks,
Alvin Henrick
Re: Review Request 19513: TAJO-684 Add functions about time
Posted by Alvin Henrick <al...@aol.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19513/
-----------------------------------------------------------
(Updated March 21, 2014, 2:31 a.m.)
Review request for Tajo and Hyunsik Choi.
Changes
-------
Sorry for confusion uploading again. Suggestions are welcome.
Repository: tajo
Description
-------
Add functions about time
Diffs
-----
tajo-common/src/main/java/org/apache/tajo/datum/UnixTimeStampDatum.java PRE-CREATION
tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/datetime/DateTimePartFromUnixTimeStamp.java PRE-CREATION
tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/function/TestDateTimeFunctions.java b882e84
Diff: https://reviews.apache.org/r/19513/diff/
Testing
-------
File Attachments (updated)
----------------
TAJO-684-Patch
https://reviews.apache.org/media/uploaded/files/2014/03/21/c8117c71-5a01-495d-ae9b-eb78d43f323a__TAJO-684-With-ASF-Unit-Test.patch
Thanks,
Alvin Henrick
Re: Review Request 19513: TAJO-684 Add functions about time
Posted by Alvin Henrick <al...@aol.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19513/
-----------------------------------------------------------
(Updated March 21, 2014, 2:28 a.m.)
Review request for Tajo and Hyunsik Choi.
Changes
-------
attachment not required
Repository: tajo
Description
-------
Add functions about time
Diffs
-----
tajo-common/src/main/java/org/apache/tajo/datum/UnixTimeStampDatum.java PRE-CREATION
tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/datetime/DateTimePartFromUnixTimeStamp.java PRE-CREATION
tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/function/TestDateTimeFunctions.java b882e84
Diff: https://reviews.apache.org/r/19513/diff/
Testing
-------
Thanks,
Alvin Henrick
Re: Review Request 19513: TAJO-684 Add functions about time
Posted by Alvin Henrick <al...@aol.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19513/
-----------------------------------------------------------
(Updated March 21, 2014, 2:27 a.m.)
Review request for Tajo and Hyunsik Choi.
Changes
-------
Added Patch
Repository: tajo
Description
-------
Add functions about time
Diffs
-----
tajo-common/src/main/java/org/apache/tajo/datum/UnixTimeStampDatum.java PRE-CREATION
tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/datetime/DateTimePartFromUnixTimeStamp.java PRE-CREATION
tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/function/TestDateTimeFunctions.java b882e84
Diff: https://reviews.apache.org/r/19513/diff/
Testing
-------
File Attachments (updated)
----------------
TAJO-684-Patch
https://reviews.apache.org/media/uploaded/files/2014/03/21/f21b100a-de56-4e0c-8632-10e05f796224__TAJO-684-With-ASF-Unit-Test.patch
Thanks,
Alvin Henrick