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

Add FrameworkContext and FrameworkConfig

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.9.0-incubating
    • Component/s: None
    • Labels:

      Description

      Allow RelOptCostFactory to be replaced. Make getPlanner() cleaner by using configuration object and builder pattern.

      ---------------- Imported from GitHub ----------------
      Url: https://github.com/julianhyde/optiq/pull/247
      Created by: jacques-n
      Labels:
      Created at: Fri Apr 11 06:20:58 CEST 2014
      State: open

        Activity

        Hide
        github-import GitHub Import added a comment -

        [Date: Fri Apr 11 06:21:25 CEST 2014, Author: jacques-n]

        This is WIP but wanted to get initial feedback. Will wrap up shortly.

        Show
        github-import GitHub Import added a comment - [Date: Fri Apr 11 06:21:25 CEST 2014, Author: jacques-n ] This is WIP but wanted to get initial feedback. Will wrap up shortly.
        Hide
        github-import GitHub Import added a comment -

        [Date: Mon Apr 21 22:22:26 CEST 2014, Author: jacques-n]

        Updated to complete patch, let me know thoughts. Thanks

        Show
        github-import GitHub Import added a comment - [Date: Mon Apr 21 22:22:26 CEST 2014, Author: jacques-n ] Updated to complete patch, let me know thoughts. Thanks
        Hide
        github-import GitHub Import added a comment -

        [Date: Thu May 08 01:15:55 CEST 2014, Author: julianhyde]

        Sorry I took so long to review this.

        `FrameworkContext` should not live in `net.hydromatic.optiq.tools`, since it is used in several packages. Can you find a better name and place for it.

        Regarding the name. The `Framework` would need to go, but you can keep `Context`. ("Context" is an over-used term, and there is already a `OptiqPrepare.Context`, but I see that `Context` is the term of art in dependency-injection, which is what this thing does.)

        I suggest you make `StdFrameworkConfig` private (or at least package protected) and move `StdFrameworkConfig.newBuilder` to `Frameworks`. We don't want people to start depending on the "standard" config class.

        And there is some crap to be cleaned up: '//' on the end of some lines; some broken javadoc (make sure `mvn site` works).

        No need to rebase master - I checked and the latest merges just fine.

        Show
        github-import GitHub Import added a comment - [Date: Thu May 08 01:15:55 CEST 2014, Author: julianhyde ] Sorry I took so long to review this. `FrameworkContext` should not live in `net.hydromatic.optiq.tools`, since it is used in several packages. Can you find a better name and place for it. Regarding the name. The `Framework` would need to go, but you can keep `Context`. ("Context" is an over-used term, and there is already a `OptiqPrepare.Context`, but I see that `Context` is the term of art in dependency-injection, which is what this thing does.) I suggest you make `StdFrameworkConfig` private (or at least package protected) and move `StdFrameworkConfig.newBuilder` to `Frameworks`. We don't want people to start depending on the "standard" config class. And there is some crap to be cleaned up: '//' on the end of some lines; some broken javadoc (make sure `mvn site` works). No need to rebase master - I checked and the latest merges just fine.
        Show
        julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/incubator-optiq/commit/31062702 .
        Hide
        julianhyde Julian Hyde added a comment -

        Close issues resolved in release 0.9.0-incubating (2014-08-25).

        Show
        julianhyde Julian Hyde added a comment - Close issues resolved in release 0.9.0-incubating (2014-08-25).

          People

          • Assignee:
            jnadeau Jacques Nadeau
            Reporter:
            github-import GitHub Import
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development