Hive
  1. Hive
  2. HIVE-3850

hour() function returns 12 hour clock value when using timestamp datatype

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.9.0, 0.10.0
    • Fix Version/s: 0.11.0
    • Component/s: UDF
    • Labels:
      None

      Description

      Apparently UDFHour.java does have two evaluate() functions. One that does accept a Text object as parameter and one that does use a TimeStampWritable object as parameter. The first function does return the value of Calendar.HOUR_OF_DAY and the second one of Calendar.HOUR. In the documentation I couldn't find any information on the overload of the evaluation function. I did spent quite some time finding out why my statement didn't return a 24 hour clock value.

      Shouldn't both functions return the same?

      1. hive-3850_1.patch
        2 kB
        Anandha L Ranganathan
      2. HIVE-3850.patch.txt
        0.5 kB
        Frankline Jose S

        Activity

        Hide
        Arun A K added a comment -

        Pieterjan Vriends,

        I have checked the following 4 queries,
        1) select hour('1:58:59') FROM myTable;
        Result >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1
        2) select hour('13:58:59') FROM myTable;
        Result >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 13
        3) select hour('2009-07-30 10:58:59') FROM myTable;
        Result >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 10
        4) select hour('2009-07-30 14:58:59') FROM myTable;
        Result >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 14

        I have got the response in 24 hour clock time aswell.

        Have I interpretted it wrong? Please comment on the same.

        Show
        Arun A K added a comment - Pieterjan Vriends , I have checked the following 4 queries, 1) select hour('1:58:59') FROM myTable; Result >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1 2) select hour('13:58:59') FROM myTable; Result >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 13 3) select hour('2009-07-30 10:58:59') FROM myTable; Result >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 10 4) select hour('2009-07-30 14:58:59') FROM myTable; Result >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 14 I have got the response in 24 hour clock time aswell. Have I interpretted it wrong? Please comment on the same.
        Hide
        Pieterjan Vriends added a comment -

        I stumbled up on the problem when I queried using the hour() function on an existing field that was declared as a TIMESTAMP, not a STRING.

        SELECT hour(mytimstampfield) FROM mytable;.

        When the field is declared as a STRING or as in your example you provide a STRING the query will indeed return an 24 hour value as expected. The overloaded evaluation function that accepts a TIMESTAMP will return a 12 hour value.

        Show
        Pieterjan Vriends added a comment - I stumbled up on the problem when I queried using the hour() function on an existing field that was declared as a TIMESTAMP, not a STRING. SELECT hour(mytimstampfield) FROM mytable;. When the field is declared as a STRING or as in your example you provide a STRING the query will indeed return an 24 hour value as expected. The overloaded evaluation function that accepts a TIMESTAMP will return a 12 hour value.
        Hide
        Pieterjan Vriends added a comment -

        From the source code:

        public IntWritable evaluate(Text dateString) {

        if (dateString == null)

        { return null; }

        try {
        Date date = null;
        try { date = formatter1.parse(dateString.toString()); } catch (ParseException e) { date = formatter2.parse(dateString.toString()); }
        calendar.setTime(date);
        result.set(calendar.get(Calendar.HOUR_OF_DAY));
        return result;
        } catch (ParseException e) { return null; }

        }

        public IntWritable evaluate(TimestampWritable t) {
        if (t == null)

        { return null; }

        calendar.setTime(t.getTimestamp());
        result.set(calendar.get(Calendar.HOUR));
        return result;
        }

        As you can see, the second evaluate() method returns the value of Calendar.HOUR instead of Calendar.HOUR_OF_DAY.

        Show
        Pieterjan Vriends added a comment - From the source code: public IntWritable evaluate(Text dateString) { if (dateString == null) { return null; } try { Date date = null; try { date = formatter1.parse(dateString.toString()); } catch (ParseException e) { date = formatter2.parse(dateString.toString()); } calendar.setTime(date); result.set(calendar.get(Calendar.HOUR_OF_DAY)); return result; } catch (ParseException e) { return null; } } public IntWritable evaluate(TimestampWritable t) { if (t == null) { return null; } calendar.setTime(t.getTimestamp()); result.set(calendar.get(Calendar.HOUR)); return result; } As you can see, the second evaluate() method returns the value of Calendar.HOUR instead of Calendar.HOUR_OF_DAY.
        Hide
        Ashutosh Chauhan added a comment -

        Pieterjan Vriends Thanks for digging into code. Mind submitting a patch for this?

        Show
        Ashutosh Chauhan added a comment - Pieterjan Vriends Thanks for digging into code. Mind submitting a patch for this?
        Hide
        Arun A K added a comment -

        Pieterjan Vriends and Ashutosh Chauhan - May be we shall submit a patch for the same.

        Show
        Arun A K added a comment - Pieterjan Vriends and Ashutosh Chauhan - May be we shall submit a patch for the same.
        Hide
        Frankline Jose S added a comment -

        Patch for HIVE-3850

        Show
        Frankline Jose S added a comment - Patch for HIVE-3850
        Hide
        Arun A K added a comment -

        Pieterjan Vriends, Please verify the change. Guess there is no need for a test case since the change is minor.

        Show
        Arun A K added a comment - Pieterjan Vriends , Please verify the change. Guess there is no need for a test case since the change is minor.
        Hide
        Ajesh Kumar added a comment -

        @Arun, Can you use https://reviews.apache.org to create a review for the change?

        Show
        Ajesh Kumar added a comment - @Arun, Can you use https://reviews.apache.org to create a review for the change?
        Hide
        Mark Grover added a comment -

        For completeness, review is at: https://reviews.apache.org/r/9171/

        Show
        Mark Grover added a comment - For completeness, review is at: https://reviews.apache.org/r/9171/
        Hide
        Mark Grover added a comment -

        Patch looks good to me.

        Usually, I would ask for unit tests to be added with any change but given that it's a trivial change, I would be ok without new tests. We should, however, make sure we update the existing unit tests if needed.

        Did you get a chance to run the unit tests (atleast the ones that use hour UDF) and make sure no changes are required in their output?

        Show
        Mark Grover added a comment - Patch looks good to me. Usually, I would ask for unit tests to be added with any change but given that it's a trivial change, I would be ok without new tests. We should, however, make sure we update the existing unit tests if needed. Did you get a chance to run the unit tests (atleast the ones that use hour UDF) and make sure no changes are required in their output?
        Hide
        Mark Grover added a comment -

        The change wasn't committed, re-opening the JIRA.

        Show
        Mark Grover added a comment - The change wasn't committed, re-opening the JIRA.
        Hide
        Pieterjan Vriends added a comment -

        Sorry about that. I must admit I'm quite new to this.

        Show
        Pieterjan Vriends added a comment - Sorry about that. I must admit I'm quite new to this.
        Hide
        Mark Grover added a comment -

        It's all good, we were all new at some point

        Here is what I would do for the tests:
        Developer Guide is a really good link to read on how unit tests work in Hive:
        https://cwiki.apache.org/confluence/display/Hive/DeveloperGuide#DeveloperGuide-Unittestsanddebugging

        Practically, unit tests take a long time to run on especially if you run it on 1 node (like me), so for a minor change like this, I run only the tests that are impacted.

        To figure that out what's impacted, I would do something like:

        cd ql/src/test/queries
        grep -ri "hour" *
        

        For results under clientpositive, I would run something like:

        ant test -Dtestcase=TestCliDriver -Dqfile=<comma separated list of .q files>
        

        For results under clientnegative, I would run something like:

        ant test TestNegativeCliDriver -Dqfile=<comma separated list of .q files>
        

        If any of the above tests fail, it may legitimately be because of the bug fix, in which case, you would want to update the test output. You can do that by using -Doverwrite=true at the end of your commands. If the outputs, do need to get updated, please include those diffs in the patch as well.

        The committers would usually run the whole suite of tests (some times on faster clusters), so don't be paranoid about testing all tests but of course, do you due-diligence about finding all tests that may be impacted and making sure they run fine.

        Hope this makes sense. If you have any further questions, please don't hesitate to ask. And, thanks for working on this patch!

        Show
        Mark Grover added a comment - It's all good, we were all new at some point Here is what I would do for the tests: Developer Guide is a really good link to read on how unit tests work in Hive: https://cwiki.apache.org/confluence/display/Hive/DeveloperGuide#DeveloperGuide-Unittestsanddebugging Practically, unit tests take a long time to run on especially if you run it on 1 node (like me), so for a minor change like this, I run only the tests that are impacted. To figure that out what's impacted, I would do something like: cd ql/src/test/queries grep -ri "hour" * For results under clientpositive, I would run something like: ant test -Dtestcase=TestCliDriver -Dqfile=<comma separated list of .q files> For results under clientnegative, I would run something like: ant test TestNegativeCliDriver -Dqfile=<comma separated list of .q files> If any of the above tests fail, it may legitimately be because of the bug fix, in which case, you would want to update the test output. You can do that by using -Doverwrite=true at the end of your commands. If the outputs, do need to get updated, please include those diffs in the patch as well. The committers would usually run the whole suite of tests (some times on faster clusters), so don't be paranoid about testing all tests but of course, do you due-diligence about finding all tests that may be impacted and making sure they run fine. Hope this makes sense. If you have any further questions, please don't hesitate to ask. And, thanks for working on this patch!
        Hide
        Anandha L Ranganathan added a comment -

        How do I assign a ticket to myself ? The edit option in assignee is disabled when I try to assign myself.

        Show
        Anandha L Ranganathan added a comment - How do I assign a ticket to myself ? The edit option in assignee is disabled when I try to assign myself.
        Hide
        Anandha L Ranganathan added a comment -

        Attaching the patch for this ticket.

        It includes fix, .q and .q.out.

        Show
        Anandha L Ranganathan added a comment - Attaching the patch for this ticket. It includes fix, .q and .q.out.
        Hide
        Anandha L Ranganathan added a comment -

        patch submitted for this ticket.

        Show
        Anandha L Ranganathan added a comment - patch submitted for this ticket.
        Hide
        Arun A K added a comment -

        Hello Anandha L Ranganathan

        There is a patch already available with the suggested changes made, submitted by Mr. Frankline Jose S. So what is the point in submitting the same patch again ? More over there is a pending review request for the same https://reviews.apache.org/r/9171/ . Request you to justify your act? Have you tried to include something different in the previous work?

        Show
        Arun A K added a comment - Hello Anandha L Ranganathan There is a patch already available with the suggested changes made, submitted by Mr. Frankline Jose S . So what is the point in submitting the same patch again ? More over there is a pending review request for the same https://reviews.apache.org/r/9171/ . Request you to justify your act? Have you tried to include something different in the previous work?
        Hide
        Arun A K added a comment -

        Hello Anandha L Ranganathan,

        In case you have written the test cases, it would be better if you could upload the .q and .q.out files as well.

        Show
        Arun A K added a comment - Hello Anandha L Ranganathan , In case you have written the test cases, it would be better if you could upload the .q and .q.out files as well.
        Hide
        Anandha L Ranganathan added a comment -

        Hello Arun,
        Justification: You should read all the comments and the ticket was re-opened.
        Also, I added .q and .q.out in the patch.

        Show
        Anandha L Ranganathan added a comment - Hello Arun, Justification: You should read all the comments and the ticket was re-opened. Also, I added .q and .q.out in the patch.
        Hide
        Arun A K added a comment -

        Hello Anandha L Ranganathan,

        But my query was that how different is the new patch? Is that it carries the test cases? I have seen that you have added the testcase carriying the typecasted time in the query that was missing in the initial q file that Frankline Jose S had added. I guess you have misunderstood my initial question. And even now the review has not yet come up. Do you think a review request need to be raised across the new patch?

        Show
        Arun A K added a comment - Hello Anandha L Ranganathan , But my query was that how different is the new patch? Is that it carries the test cases? I have seen that you have added the testcase carriying the typecasted time in the query that was missing in the initial q file that Frankline Jose S had added. I guess you have misunderstood my initial question. And even now the review has not yet come up. Do you think a review request need to be raised across the new patch?
        Hide
        Anandha L Ranganathan added a comment -

        Arun,

        Still I didn't get it.

        If the test cases are okay, can you raise a review request. I don't have permission to raise a review request. ( I don't know how to raise a review request).

        Show
        Anandha L Ranganathan added a comment - Arun, Still I didn't get it. If the test cases are okay, can you raise a review request. I don't have permission to raise a review request. ( I don't know how to raise a review request).
        Hide
        Arun A K added a comment -

        Hello Anandha L Ranganathan, the test cases are ok. I think this need to be considered for commit. Ajesh Kumar had given me the link to raise the review request. I tried raising the review, but I am getting an error message "The selected file does not appear to be a diff." If possible could you create a new patch with the name HIVE-3850.1.patch ? Either Frankline Jose S or yourself can do that so that we can edit the current review request (https://reviews.apache.org/r/9171/) or discard this and create a new one.

        Show
        Arun A K added a comment - Hello Anandha L Ranganathan , the test cases are ok. I think this need to be considered for commit. Ajesh Kumar had given me the link to raise the review request. I tried raising the review, but I am getting an error message "The selected file does not appear to be a diff." If possible could you create a new patch with the name HIVE-3850 .1.patch ? Either Frankline Jose S or yourself can do that so that we can edit the current review request ( https://reviews.apache.org/r/9171/ ) or discard this and create a new one.
        Hide
        Anandha L Ranganathan added a comment -

        re-attaching the patch

        Show
        Anandha L Ranganathan added a comment - re-attaching the patch
        Hide
        Anandha L Ranganathan added a comment -
        Show
        Anandha L Ranganathan added a comment - review request. https://reviews.apache.org/r/9673/
        Hide
        Mark Grover added a comment -

        +1 (non-committer)

        Show
        Mark Grover added a comment - +1 (non-committer)
        Hide
        Ashutosh Chauhan added a comment -

        +1 will commit if tests pass.

        Show
        Ashutosh Chauhan added a comment - +1 will commit if tests pass.
        Hide
        Ashutosh Chauhan added a comment -

        Committed to trunk. Thanks, Anandha and Franklin!

        Show
        Ashutosh Chauhan added a comment - Committed to trunk. Thanks, Anandha and Franklin!
        Hide
        Hudson added a comment -

        Integrated in Hive-trunk-hadoop2 #138 (See https://builds.apache.org/job/Hive-trunk-hadoop2/138/)
        HIVE-3850 : hour() function returns 12 hour clock value when using timestamp datatype (Anandha and Franklin via Ashutosh Chauhan) (Revision 1462988)

        Result = FAILURE
        hashutosh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1462988
        Files :

        • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/UDFHour.java
        • /hive/trunk/ql/src/test/queries/clientpositive/udf_hour.q
        • /hive/trunk/ql/src/test/results/clientpositive/udf_hour.q.out
        Show
        Hudson added a comment - Integrated in Hive-trunk-hadoop2 #138 (See https://builds.apache.org/job/Hive-trunk-hadoop2/138/ ) HIVE-3850 : hour() function returns 12 hour clock value when using timestamp datatype (Anandha and Franklin via Ashutosh Chauhan) (Revision 1462988) Result = FAILURE hashutosh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1462988 Files : /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/UDFHour.java /hive/trunk/ql/src/test/queries/clientpositive/udf_hour.q /hive/trunk/ql/src/test/results/clientpositive/udf_hour.q.out

          People

          • Assignee:
            Unassigned
            Reporter:
            Pieterjan Vriends
          • Votes:
            2 Vote for this issue
            Watchers:
            9 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development