Uploaded image for project: 'Calcite'
  1. Calcite
  2. CALCITE-1177

Support more time units in EXTRACT function

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.8.0
    • Component/s: core
    • Labels:
      None

      Description

      Currently extract function support following units YEAR, MONTH, DAY, HOUR, MINUTE, SECOND. Add support for more units: CENTURY, DECADE, DOW, DOY, EPOCH, MILLENNIUM, QUARTER, WEEK.

      Definitions of these units is similar to Postgres: http://www.postgresql.org/docs/9.1/static/functions-datetime.html#FUNCTIONS-DATETIME-EXTRACT

        Issue Links

          Activity

          Hide
          julianhyde Julian Hyde added a comment -

          If you extend the list of time units, please change FLOOR(datetime TO timeunit) and CEIL(datetime TO timeunit) to support the new units. There's probably not much to be done besides adding to testFloorFuncInterval].

          Show
          julianhyde Julian Hyde added a comment - If you extend the list of time units, please change FLOOR(datetime TO timeunit) and CEIL(datetime TO timeunit) to support the new units. There's probably not much to be done besides adding to testFloorFuncInterval] .
          Hide
          vkorukanti Venki Korukanti added a comment -

          Sure.

          Show
          vkorukanti Venki Korukanti added a comment - Sure.
          Hide
          julianhyde Julian Hyde added a comment -
          • Can you make the multiplier null for DOW, DOY, EPOCH?
          • s/to use it internally/to use internally/
          • Change the parser, add to SqlParserTest.testExtract and .testFloor, .testCeil, and make sure test passes.
          • Add a SqlParserTest that INTERVAL '2' MILLENNIUM is not valid.
          • Add to SqlOperatorBaseTest.testFloorFuncInterval.
          Show
          julianhyde Julian Hyde added a comment - Can you make the multiplier null for DOW, DOY, EPOCH? s/to use it internally/to use internally/ Change the parser, add to SqlParserTest.testExtract and .testFloor, .testCeil, and make sure test passes. Add a SqlParserTest that INTERVAL '2' MILLENNIUM is not valid. Add to SqlOperatorBaseTest.testFloorFuncInterval.
          Hide
          vkorukanti Venki Korukanti added a comment -

          Thanks Julian for reviewing.

          - Can you make the multiplier null for DOW, DOY, EPOCH?

          - s/to use it internally/to use internally

          Updated PR

          For others I have the wip patch here. Currently I can't even compile with 1.8.0-SNAPSHOT of avatica as there are some breaking changes in current avatica compared to 1.7.0 version. I was hoping to get the first patch in and once a new version of avatica is available and core is updated to use the new version, submit the second patch which includes parser changes and tests.

          Show
          vkorukanti Venki Korukanti added a comment - Thanks Julian for reviewing. - Can you make the multiplier null for DOW, DOY, EPOCH? - s/to use it internally/to use internally Updated PR For others I have the wip patch here . Currently I can't even compile with 1.8.0-SNAPSHOT of avatica as there are some breaking changes in current avatica compared to 1.7.0 version. I was hoping to get the first patch in and once a new version of avatica is available and core is updated to use the new version, submit the second patch which includes parser changes and tests.
          Hide
          elserj Josh Elser added a comment -

          re: breaking changes in Avatica. The only thing I can think of would be new methods added to some interfaces that Calcite implements. We could look at hiding some of this breakage by introducing a default implementation in Avatica that Calcite would extend instead.

          Either way, most likely all you would need to do to compile against 1.8.0 of avatica is stub out those new methods. I would be very interested to know if there is more that would need to be done.

          Show
          elserj Josh Elser added a comment - re: breaking changes in Avatica. The only thing I can think of would be new methods added to some interfaces that Calcite implements. We could look at hiding some of this breakage by introducing a default implementation in Avatica that Calcite would extend instead. Either way, most likely all you would need to do to compile against 1.8.0 of avatica is stub out those new methods. I would be very interested to know if there is more that would need to be done.
          Hide
          julianhyde Julian Hyde added a comment -

          Or, rebase the patch to a time when calcite and Avatica were compatible, such as just after the last release. I'd prefer to have the full PR(s) in hand before accepting.

          Show
          julianhyde Julian Hyde added a comment - Or, rebase the patch to a time when calcite and Avatica were compatible, such as just after the last release. I'd prefer to have the full PR(s) in hand before accepting.
          Hide
          vkorukanti Venki Korukanti added a comment -

          Hi Julian Hyde, Rebased the patch to just after the last release of Calcite. Here are the new changes. While writing the tests for SqlOperatorBaseTest.testFloorFuncInterval, I realized convertlet for functions extract, floor and ceil is out of sync with the supported time units. Some of the newly supported units may not rewritten in div and multiplication (ex. millennium depends on type of calendar). Should we leave the convertlet table as it is or implement for certain calendar (eg. ISO)?

          Show
          vkorukanti Venki Korukanti added a comment - Hi Julian Hyde , Rebased the patch to just after the last release of Calcite. Here are the new changes. While writing the tests for SqlOperatorBaseTest.testFloorFuncInterval , I realized convertlet for functions extract, floor and ceil is out of sync with the supported time units. Some of the newly supported units may not rewritten in div and multiplication (ex. millennium depends on type of calendar). Should we leave the convertlet table as it is or implement for certain calendar (eg. ISO)?
          Hide
          julianhyde Julian Hyde added a comment -

          Let's just use the "obvious" definition of calendar, not try to make it locale-specific. The functions will give the same answers regardless of the client's or server's locale. They can be implemented using simple div / modulo. Also remember these functions should not be using time zone in any way.

          Show
          julianhyde Julian Hyde added a comment - Let's just use the "obvious" definition of calendar, not try to make it locale-specific. The functions will give the same answers regardless of the client's or server's locale. They can be implemented using simple div / modulo. Also remember these functions should not be using time zone in any way.
          Hide
          vkorukanti Venki Korukanti added a comment -

          Hi Julian Hyde, updated the convertlet function for extract and added tests. For extract function call time units which can't be rewritten are returned without conversion. Here is the new patch. Could you please review the patch?

          Show
          vkorukanti Venki Korukanti added a comment - Hi Julian Hyde , updated the convertlet function for extract and added tests. For extract function call time units which can't be rewritten are returned without conversion. Here is the new patch. Could you please review the patch?
          Hide
          julianhyde Julian Hyde added a comment -

          Venki Korukanti, I have reviewed the PR, and it all looks good. I have committed the Avatica change, CALCITE-1179. We will have to wait until the next Avatica release before we commit the Calcite changes (omitting of course the change to make Calcite depend on an Avatica snapshot).

          Josh Elser, Any ETA for avatica-1.8.0?

          Show
          julianhyde Julian Hyde added a comment - Venki Korukanti , I have reviewed the PR, and it all looks good. I have committed the Avatica change, CALCITE-1179 . We will have to wait until the next Avatica release before we commit the Calcite changes (omitting of course the change to make Calcite depend on an Avatica snapshot). Josh Elser , Any ETA for avatica-1.8.0?
          Hide
          elserj Josh Elser added a comment -

          Any ETA for avatica-1.8.0?

          I need to finish up CALCITE-1190. CALCITE-1209 should get fixed as well. I might have some time over the weekend to work on these. After that, I'm all for an avatica-1.8.0. So, maybe as soon as next week.

          Show
          elserj Josh Elser added a comment - Any ETA for avatica-1.8.0? I need to finish up CALCITE-1190 . CALCITE-1209 should get fixed as well. I might have some time over the weekend to work on these. After that, I'm all for an avatica-1.8.0. So, maybe as soon as next week.
          Hide
          julianhyde Julian Hyde added a comment -

          Josh Elser, Ok, thanks. No particular hurry.

          Show
          julianhyde Julian Hyde added a comment - Josh Elser , Ok, thanks. No particular hurry.
          Hide
          vkorukanti Venki Korukanti added a comment -

          Thanks Julian Hyde!

          Show
          vkorukanti Venki Korukanti added a comment - Thanks Julian Hyde !
          Hide
          julianhyde Julian Hyde added a comment -
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/97945652 . Thanks for the PR, Venki Korukanti !
          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.8.0 (2016-06-13).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.8.0 (2016-06-13).

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              vkorukanti Venki Korukanti
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development