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

Flink table API defaults top level fields as nullable and all nested fields within CompositeType as non-nullable

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.3.1
    • Fix Version/s: 1.4.0, 1.3.2
    • Component/s: Table API & SQL
    • Labels:
      None

      Description

      Right now FlinkTypeFactory does conversion between Flink TypeInformation to Calcite RelDataType by assuming the following:

      All top level fields will be set to nullable and all nested fields within CompositeRelDataType and GenericRelDataType will be set to Calcite default (which is non-nullable).

      This triggers Calcite SQL optimization engine drop all `IS NOT NULL` clause on nested fields, and would not be able to optimize when top level fields were actually non-nullable.

        Issue Links

          Activity

          Hide
          fhueske Fabian Hueske added a comment -

          Thanks for reporting this issue.
          I don't think this behavior is intended and should be fixed by setting nested fields to nullable as well.

          What do you think Timo Walther?

          Show
          fhueske Fabian Hueske added a comment - Thanks for reporting this issue. I don't think this behavior is intended and should be fixed by setting nested fields to nullable as well. What do you think Timo Walther ?
          Hide
          twalthr Timo Walther added a comment -

          Yes, this should be fixed. Everything should be nullable for now.

          Show
          twalthr Timo Walther added a comment - Yes, this should be fixed. Everything should be nullable for now.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user walterddr opened a pull request:

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

          FLINK-7137 [table] Flink TableAPI supports nested CompositeType mapped to nullable calcite RelDataType by default

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

          $ git pull https://github.com/walterddr/flink FLINK-7137

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

          https://github.com/apache/flink/pull/4314.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 #4314


          commit 93c475b7da114a3022eb2b6fb8b46ffe8a1a6997
          Author: Rong Rong <rongr@uber.com>
          Date: 2017-07-13T04:29:16Z

          Fixing nested fields not mapped to nullable calcite RelDataType by default


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user walterddr opened a pull request: https://github.com/apache/flink/pull/4314 FLINK-7137 [table] Flink TableAPI supports nested CompositeType mapped to nullable calcite RelDataType by default You can merge this pull request into a Git repository by running: $ git pull https://github.com/walterddr/flink FLINK-7137 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/4314.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 #4314 commit 93c475b7da114a3022eb2b6fb8b46ffe8a1a6997 Author: Rong Rong <rongr@uber.com> Date: 2017-07-13T04:29:16Z Fixing nested fields not mapped to nullable calcite RelDataType by default
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fhueske commented on the issue:

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

          Thanks for the PR @walterddr.
          This looks good to me.
          What do you think @twalthr?

          Show
          githubbot ASF GitHub Bot added a comment - Github user fhueske commented on the issue: https://github.com/apache/flink/pull/4314 Thanks for the PR @walterddr. This looks good to me. What do you think @twalthr?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user twalthr commented on the issue:

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

          I tried to improve the stability regarding nullable types. This should fix FLINK-7177 and FLINK-7137. It ensures that all types have the correct nullability. By default, types are always nullable. If they are not nullable, than we know it for sure.

          https://github.com/apache/flink/compare/master...twalthr:FLINK-7137

          What do you think? @walterddr @fhueske

          Show
          githubbot ASF GitHub Bot added a comment - Github user twalthr commented on the issue: https://github.com/apache/flink/pull/4314 I tried to improve the stability regarding nullable types. This should fix FLINK-7177 and FLINK-7137 . It ensures that all types have the correct nullability. By default, types are always nullable. If they are not nullable, than we know it for sure. https://github.com/apache/flink/compare/master...twalthr:FLINK-7137 What do you think? @walterddr @fhueske
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user walterddr commented on the issue:

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

          @twalthr Thank you for the suggestion, yeah I think this nullable default to false issue will pop up soon somewhere else, might as well just resolve them all together. I incorporated your suggestions and also add some tests so that the problem could be captured during unitest instead of ITCases. Let me know what your thoughts on this are @fhueske @twalthr

          Show
          githubbot ASF GitHub Bot added a comment - Github user walterddr commented on the issue: https://github.com/apache/flink/pull/4314 @twalthr Thank you for the suggestion, yeah I think this nullable default to false issue will pop up soon somewhere else, might as well just resolve them all together. I incorporated your suggestions and also add some tests so that the problem could be captured during unitest instead of ITCases. Let me know what your thoughts on this are @fhueske @twalthr
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user twalthr commented on the issue:

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

          Thanks for the tests @walterddr. I will merge this now...

          Show
          githubbot ASF GitHub Bot added a comment - Github user twalthr commented on the issue: https://github.com/apache/flink/pull/4314 Thanks for the tests @walterddr. I will merge this now...
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/4314
          Hide
          twalthr Timo Walther added a comment -

          Fixed in 1.4.0: 2d1e08a02d84f8d7cb2734e09741eae72bf63b7d, 7aa115658b23c19fbcc8e3d1d83113608ebd7ce7, c0bad3b80d6fe67e43bc1a5d3bebbd98479e3d76

          Fixed in 1.3.2: be8ca8a384604a2fb2bd74886f452e4a61ce9cfb, 8c87c44692bc27fb8018adf587715a9488947799

          Show
          twalthr Timo Walther added a comment - Fixed in 1.4.0: 2d1e08a02d84f8d7cb2734e09741eae72bf63b7d, 7aa115658b23c19fbcc8e3d1d83113608ebd7ce7, c0bad3b80d6fe67e43bc1a5d3bebbd98479e3d76 Fixed in 1.3.2: be8ca8a384604a2fb2bd74886f452e4a61ce9cfb, 8c87c44692bc27fb8018adf587715a9488947799

            People

            • Assignee:
              walterddr Rong Rong
              Reporter:
              walterddr Rong Rong
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development