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

Bump Calcite version to 1.12 once available

    Details

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

      Description

      Once Calcite 1.12 is release we should update to remove some copied classes.

        Issue Links

          Activity

          Hide
          twalthr Timo Walther added a comment -

          Fixed in 1.3.0: 05ceec0ace45135bc1cc17934a0b8721f4f85a03

          Show
          twalthr Timo Walther added a comment - Fixed in 1.3.0: 05ceec0ace45135bc1cc17934a0b8721f4f85a03
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

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

          Github user twalthr commented on the issue:

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

          Thanks @haohui. I will merge this.

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

          Github user fhueske commented on the issue:

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

          Thanks @haohui.
          I think this PR is good to merge.
          Since the support to unregister tables was added after Flink 1.2 was released, we are not removing a released feature.

          @twalthr, do you want to have a look as well?

          Show
          githubbot ASF GitHub Bot added a comment - Github user fhueske commented on the issue: https://github.com/apache/flink/pull/3613 Thanks @haohui. I think this PR is good to merge. Since the support to unregister tables was added after Flink 1.2 was released, we are not removing a released feature. @twalthr, do you want to have a look as well?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user haohui commented on the issue:

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

          The failed test is unrelated to the changes. @twalthr @fhueske can you please take a look?

          Show
          githubbot ASF GitHub Bot added a comment - Github user haohui commented on the issue: https://github.com/apache/flink/pull/3613 The failed test is unrelated to the changes. @twalthr @fhueske can you please take a look?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user twalthr commented on the issue:

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

          Thanks @haohui for looking into this. I think you forgot to include the modified pom.xml. Can you remove all "unregisterTable" code instead of throwing an exception? It would be great if you could have a look into the resulting flink-table jar and job jar that includes the flink-table on order to check if shading filters are still valid.

          Show
          githubbot ASF GitHub Bot added a comment - Github user twalthr commented on the issue: https://github.com/apache/flink/pull/3613 Thanks @haohui for looking into this. I think you forgot to include the modified pom.xml. Can you remove all "unregisterTable" code instead of throwing an exception? It would be great if you could have a look into the resulting flink-table jar and job jar that includes the flink-table on order to check if shading filters are still valid.
          Hide
          twalthr Timo Walther added a comment -

          If it is too complicated to implement, we can drop support for unregistering tables. It was requested by a user, I don't know for which use case.

          Show
          twalthr Timo Walther added a comment - If it is too complicated to implement, we can drop support for unregistering tables. It was requested by a user, I don't know for which use case.
          Hide
          fhueske Fabian Hueske added a comment -

          I just had a chat with Haohui Mai and we came to the conclusion that using a custom schema is not so easy. We could register a sub-schema, but then all table references in SQL queries would need to be prefixed with the schema name.

          In fact, I'm not sure how important it is to be able to remove tables from the root schema. It seems that this would be mostly relevant for long running applications that run many queries. Such applications could also register an own external catalog and manage the available tables themselves.

          What do you think, @twalthr?

          Show
          fhueske Fabian Hueske added a comment - I just had a chat with Haohui Mai and we came to the conclusion that using a custom schema is not so easy. We could register a sub-schema, but then all table references in SQL queries would need to be prefixed with the schema name. In fact, I'm not sure how important it is to be able to remove tables from the root schema. It seems that this would be mostly relevant for long running applications that run many queries. Such applications could also register an own external catalog and manage the available tables themselves. What do you think, @twalthr?
          Hide
          fhueske Fabian Hueske added a comment -

          I think we could implement our own internal default schema (similar to the ExternalCatalogSchema) and register it in Calcite.
          This would allow us to arbitrarily modify the registered tables.

          Show
          fhueske Fabian Hueske added a comment - I think we could implement our own internal default schema (similar to the ExternalCatalogSchema ) and register it in Calcite. This would allow us to arbitrarily modify the registered tables.
          Hide
          wheat9 Haohui Mai added a comment -

          Unfortunately it seems that it is infeasible to inherit and forward the calls because Calcite declares most of the methods as package local methods. It seems that Calcite intentionally keeps the class as a package local class.

          Another option is to defer the functionality of unregistering tables until Calcite provides the API. We can open a jira in Calcite and get it fixed in Calcite 1.13. However, given the release schedule, it seems to me that the functionality will be deferred to Flink 1.4.

          Timo Walther does it sound okay to you? What do you think?

          Show
          wheat9 Haohui Mai added a comment - Unfortunately it seems that it is infeasible to inherit and forward the calls because Calcite declares most of the methods as package local methods. It seems that Calcite intentionally keeps the class as a package local class. Another option is to defer the functionality of unregistering tables until Calcite provides the API. We can open a jira in Calcite and get it fixed in Calcite 1.13. However, given the release schedule, it seems to me that the functionality will be deferred to Flink 1.4. Timo Walther does it sound okay to you? What do you think?
          Hide
          twalthr Timo Walther added a comment -

          Haohui Mai if it is not much effort to extend the class in order to access the map, then it would be great. I think unregistering a table is a basic operation that should be supported.

          Show
          twalthr Timo Walther added a comment - Haohui Mai if it is not much effort to extend the class in order to access the map, then it would be great. I think unregistering a table is a basic operation that should be supported.
          Hide
          wheat9 Haohui Mai added a comment -

          I just pushed a PR. The migration process is relatively straightforward except that Calcite 1.12 seems to be conflicted with FLINK-4288.

          The tableMap field has become a protected member thus unregister table become non-trivial. There are two options here.

          1. Revert FLINK-4288. FLINK-4288 has not been released yet thus it is okay to pull it back with no concerns on backward compatibility.
          2. Implement a proxy schema which inherits from CalciteSchema to regain the access of the field.

          Fabian Hueske Timo Walther, what do you think?

          Show
          wheat9 Haohui Mai added a comment - I just pushed a PR. The migration process is relatively straightforward except that Calcite 1.12 seems to be conflicted with FLINK-4288 . The tableMap field has become a protected member thus unregister table become non-trivial. There are two options here. 1. Revert FLINK-4288 . FLINK-4288 has not been released yet thus it is okay to pull it back with no concerns on backward compatibility. 2. Implement a proxy schema which inherits from CalciteSchema to regain the access of the field. Fabian Hueske Timo Walther , what do you think?
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user haohui opened a pull request:

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

          FLINK-5829 Bump Calcite version to 1.12 once available.

          This PR bumps the Calcite version from 1.11 to 1.12.

          The main issue is that it conflicts with FLINK-4288, as the `tableMap` field becomes protected which makes unregistering table no longer trivial.

          As the first iteration I disable the related tests in this PR. We need to come up with a solution.

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

          $ git pull https://github.com/haohui/flink FLINK-5829

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

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


          commit 02533b4dbc2ba66f9ff3ce2f68c415d1460a6258
          Author: Haohui Mai <wheat9@apache.org>
          Date: 2017-03-25T08:00:50Z

          FLINK-5829 Bump Calcite version to 1.12 once available.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user haohui opened a pull request: https://github.com/apache/flink/pull/3613 FLINK-5829 Bump Calcite version to 1.12 once available. This PR bumps the Calcite version from 1.11 to 1.12. The main issue is that it conflicts with FLINK-4288 , as the `tableMap` field becomes protected which makes unregistering table no longer trivial. As the first iteration I disable the related tests in this PR. We need to come up with a solution. You can merge this pull request into a Git repository by running: $ git pull https://github.com/haohui/flink FLINK-5829 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3613.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 #3613 commit 02533b4dbc2ba66f9ff3ce2f68c415d1460a6258 Author: Haohui Mai <wheat9@apache.org> Date: 2017-03-25T08:00:50Z FLINK-5829 Bump Calcite version to 1.12 once available.
          Hide
          fhueske Fabian Hueske added a comment -

          Calcite 1.12 has been release today: http://calcite.apache.org/news/2017/03/24/release-1.12.0/

          We can upgrade and address the related issues.

          Show
          fhueske Fabian Hueske added a comment - Calcite 1.12 has been release today: http://calcite.apache.org/news/2017/03/24/release-1.12.0/ We can upgrade and address the related issues.

            People

            • Assignee:
              wheat9 Haohui Mai
              Reporter:
              fhueske Fabian Hueske
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development