Uploaded image for project: 'Flink'
  1. Flink
  2. FLINK-5921

Adapt time mode indicator functions return custom data types

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Implemented
    • Affects Version/s: 1.3.0
    • Fix Version/s: 1.3.0
    • Component/s: Table API & SQL
    • Labels:
      None

      Description

      The functions that indicate event time (rowtime()) and processing time (proctime()) are defined to return TIMESTAMP.

      These functions should be updated to return custom types in order to ease the identification of the time semantics during optimization.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user fhueske opened a pull request:

          https://github.com/apache/flink/pull/3425

          FLINK-5921 [table] Add custom data types for rowtime and proctime.

          We add a custom data type for the result of the `rowtime()` and `proctime()` marker functions to easier identify the chosen time semantics in when translating window operations.

          Both method return a constant timestamp (0L) which is injected during constant expression reduction.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/fhueske/flink tableTimeTypes

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/flink/pull/3425.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #3425


          commit 6cc819b88c408de7b42a968d2a8cdb21ab927ffd
          Author: Fabian Hueske <fhueske@apache.org>
          Date: 2017-02-25T23:28:54Z

          FLINK-5921 [table] Add custom data types for rowtime and proctime.

          • proctime() and rowtime() return constont timestamp (0L).

          Show
          githubbot ASF GitHub Bot added a comment - GitHub user fhueske opened a pull request: https://github.com/apache/flink/pull/3425 FLINK-5921 [table] Add custom data types for rowtime and proctime. We add a custom data type for the result of the `rowtime()` and `proctime()` marker functions to easier identify the chosen time semantics in when translating window operations. Both method return a constant timestamp (0L) which is injected during constant expression reduction. You can merge this pull request into a Git repository by running: $ git pull https://github.com/fhueske/flink tableTimeTypes Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3425.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #3425 commit 6cc819b88c408de7b42a968d2a8cdb21ab927ffd Author: Fabian Hueske <fhueske@apache.org> Date: 2017-02-25T23:28:54Z FLINK-5921 [table] Add custom data types for rowtime and proctime. proctime() and rowtime() return constont timestamp (0L).
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user sunjincheng121 commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3425#discussion_r103384285

          — Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/codegen/calls/FunctionGenerator.scala —
          @@ -327,6 +329,15 @@ object FunctionGenerator {
          )
          )

          + // generate a constant for time indicator functions.
          + // this is a temporary solution and will be removed when FLINK-5884 is implemented.
          + case ProcTimeExtractor | EventTimeExtractor =>
          + Some(new CallGenerator {
          + override def generate(codeGenerator: CodeGenerator, operands: Seq[GeneratedExpression]) = {
          + GeneratedExpression("0L", "false", "", LONG_TYPE_INFO)
          — End diff –

          Can we change `LONG_TYPE_INFO` to `SqlTimeTypeInfo.TIMESTAMP`?

          Show
          githubbot ASF GitHub Bot added a comment - Github user sunjincheng121 commented on a diff in the pull request: https://github.com/apache/flink/pull/3425#discussion_r103384285 — Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/codegen/calls/FunctionGenerator.scala — @@ -327,6 +329,15 @@ object FunctionGenerator { ) ) + // generate a constant for time indicator functions. + // this is a temporary solution and will be removed when FLINK-5884 is implemented. + case ProcTimeExtractor | EventTimeExtractor => + Some(new CallGenerator { + override def generate(codeGenerator: CodeGenerator, operands: Seq [GeneratedExpression] ) = { + GeneratedExpression("0L", "false", "", LONG_TYPE_INFO) — End diff – Can we change `LONG_TYPE_INFO` to `SqlTimeTypeInfo.TIMESTAMP`?
          Hide
          wheat9 Haohui Mai added a comment -

          My understanding is that there should be no need for code generators as the indicators will be removed in the hep planner?

          Show
          wheat9 Haohui Mai added a comment - My understanding is that there should be no need for code generators as the indicators will be removed in the hep planner?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fhueske commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3425#discussion_r103399995

          — Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/codegen/calls/FunctionGenerator.scala —
          @@ -327,6 +329,15 @@ object FunctionGenerator {
          )
          )

          + // generate a constant for time indicator functions.
          + // this is a temporary solution and will be removed when FLINK-5884 is implemented.
          + case ProcTimeExtractor | EventTimeExtractor =>
          + Some(new CallGenerator {
          + override def generate(codeGenerator: CodeGenerator, operands: Seq[GeneratedExpression]) = {
          + GeneratedExpression("0L", "false", "", LONG_TYPE_INFO)
          — End diff –

          Yes, thanks for the hint

          Show
          githubbot ASF GitHub Bot added a comment - Github user fhueske commented on a diff in the pull request: https://github.com/apache/flink/pull/3425#discussion_r103399995 — Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/codegen/calls/FunctionGenerator.scala — @@ -327,6 +329,15 @@ object FunctionGenerator { ) ) + // generate a constant for time indicator functions. + // this is a temporary solution and will be removed when FLINK-5884 is implemented. + case ProcTimeExtractor | EventTimeExtractor => + Some(new CallGenerator { + override def generate(codeGenerator: CodeGenerator, operands: Seq [GeneratedExpression] ) = { + GeneratedExpression("0L", "false", "", LONG_TYPE_INFO) — End diff – Yes, thanks for the hint
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fhueske commented on the issue:

          https://github.com/apache/flink/pull/3425

          Updated

          Show
          githubbot ASF GitHub Bot added a comment - Github user fhueske commented on the issue: https://github.com/apache/flink/pull/3425 Updated
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user sunjincheng121 commented on the issue:

          https://github.com/apache/flink/pull/3425

          @fhueske Thanks for updating, the PR looks very good to me. I look forward to merge.
          Best,
          SunJincheng

          Show
          githubbot ASF GitHub Bot added a comment - Github user sunjincheng121 commented on the issue: https://github.com/apache/flink/pull/3425 @fhueske Thanks for updating, the PR looks very good to me. I look forward to merge. Best, SunJincheng
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user twalthr commented on the issue:

          https://github.com/apache/flink/pull/3425

          I'm fine with this temporary solution. +1 for merging until FLINK-5884 is fixed.

          Show
          githubbot ASF GitHub Bot added a comment - Github user twalthr commented on the issue: https://github.com/apache/flink/pull/3425 I'm fine with this temporary solution. +1 for merging until FLINK-5884 is fixed.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fhueske commented on the issue:

          https://github.com/apache/flink/pull/3425

          Thanks for the feedback @sunjincheng121 and @twalthr

          Merging this.

          Show
          githubbot ASF GitHub Bot added a comment - Github user fhueske commented on the issue: https://github.com/apache/flink/pull/3425 Thanks for the feedback @sunjincheng121 and @twalthr Merging this.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fhueske closed the pull request at:

          https://github.com/apache/flink/pull/3425

          Show
          githubbot ASF GitHub Bot added a comment - Github user fhueske closed the pull request at: https://github.com/apache/flink/pull/3425
          Hide
          fhueske Fabian Hueske added a comment -

          Implemented with 2a1a9c1e31faedb76c990a4f9405837600d770f8

          Show
          fhueske Fabian Hueske added a comment - Implemented with 2a1a9c1e31faedb76c990a4f9405837600d770f8

            People

            • Assignee:
              fhueske Fabian Hueske
              Reporter:
              fhueske Fabian Hueske
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development