Details

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

      Description

      The ViewExpander.expandView() call already has the schema path that contains the view but not the view name itself.
      In some context it is useful to also know the name of the view being expanded.
      current call:

      RelRoot expandView(
              RelDataType rowType,
              String queryString,
              SchemaPlus rootSchema,
              List<String> schemaPath);
      

      proposed:

      RelRoot expandView(
              RelDataType rowType,
              String queryString,
              SchemaPlus rootSchema,
              List<String> schemaPath,
              String viewName);
      

        Activity

        Hide
        julianhyde Julian Hyde added a comment -

        Yes, it's reasonable, but remember that in some contexts the view name is not available. In Calcite a view, like any Table, may be anonymous. It has a path only if it was acquired from the schema SPI.

        Would it make sense to add the view name to the schema path? And let the schema path be null?

        Show
        julianhyde Julian Hyde added a comment - Yes, it's reasonable, but remember that in some contexts the view name is not available. In Calcite a view, like any Table, may be anonymous. It has a path only if it was acquired from the schema SPI. Would it make sense to add the view name to the schema path? And let the schema path be null?
        Hide
        julienledem Julien Le Dem added a comment -

        Do you mean to leave the signature unchanged and change schemaPath to have the view name as the last element?
        Wouldn't that be a change of behavior that could break existing code?
        I have a patch for adding the viewName parameter to the signature.
        I think it is fine to allow viewName to be null.

        Show
        julienledem Julien Le Dem added a comment - Do you mean to leave the signature unchanged and change schemaPath to have the view name as the last element? Wouldn't that be a change of behavior that could break existing code? I have a patch for adding the viewName parameter to the signature. I think it is fine to allow viewName to be null.
        Show
        julienledem Julien Le Dem added a comment - See: https://github.com/apache/calcite/pull/263
        Hide
        julianhyde Julian Hyde added a comment -

        Your proposal and my proposal would both break existing code. But yours would change the API, so the breakage would be more obvious.

        Now I've looked at the code, I remember what is the purpose of the List<String> schemaPath parameter. It is the schema in which to look for tables referenced in the SQL. It is not necessarily the same as the schema in which the view is being defined. So, I propose the following:

        RelRoot expandView(
                RelDataType rowType,
                String queryString,
                SchemaPlus rootSchema,
                List<String> schemaPath,
                List<String> viewPath);
        
        Show
        julianhyde Julian Hyde added a comment - Your proposal and my proposal would both break existing code. But yours would change the API, so the breakage would be more obvious. Now I've looked at the code, I remember what is the purpose of the List<String> schemaPath parameter. It is the schema in which to look for tables referenced in the SQL. It is not necessarily the same as the schema in which the view is being defined. So, I propose the following: RelRoot expandView( RelDataType rowType, String queryString, SchemaPlus rootSchema, List< String > schemaPath, List< String > viewPath);
        Hide
        julienledem Julien Le Dem added a comment -

        (obvious is good )

        Your proposal sounds good to me. I'll make it so.

        Show
        julienledem Julien Le Dem added a comment - (obvious is good ) Your proposal sounds good to me. I'll make it so.
        Hide
        julienledem Julien Le Dem added a comment -
        Show
        julienledem Julien Le Dem added a comment - PR updated: https://github.com/apache/calcite/pull/263
        Hide
        julianhyde Julian Hyde added a comment - - edited
        Show
        julianhyde Julian Hyde added a comment - - edited Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/7936185 ; thanks for the PR, Julien Le Dem !
        Hide
        jcamachorodriguez Jesus Camacho Rodriguez added a comment -

        Resolved in release 1.9.0 (2016-09-22)

        Show
        jcamachorodriguez Jesus Camacho Rodriguez added a comment - Resolved in release 1.9.0 (2016-09-22)

          People

          • Assignee:
            julianhyde Julian Hyde
            Reporter:
            julienledem Julien Le Dem
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development