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

Could not resolve VIEW with SimpleCalciteSchema

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.11.0
    • Fix Version/s: 1.12.0
    • Component/s: core
    • Labels:
      None

      Description

      The current implementation (ever since its first version) of SimpleCalciteSchema.getImplicitTableBasedOnNullaryFunction() is wrong, but it was not revealed until Julian Hyde's check-in for CALCITE-1563.
      The implementation 1) did not use "tableName" in finding the functions at all; 2) was caching based.

        protected TableEntry getImplicitTableBasedOnNullaryFunction(String tableName,
            boolean caseSensitive) {
          for (String s : schema.getFunctionNames()) {
            for (Function function : schema.getFunctions(s)) {
              if (function instanceof TableMacro
                  && function.getParameters().isEmpty()) {
                final Table table = ((TableMacro) function).apply(ImmutableList.of());
                return tableEntry(tableName, table);
              }
            }
          }
          return null;
        }
      

        Activity

        Hide
        julianhyde Julian Hyde added a comment -

        Resolved in release 1.12.0 (2017-03-24).

        Show
        julianhyde Julian Hyde added a comment - Resolved in release 1.12.0 (2017-03-24).
        Hide
        maryannxue Maryann Xue added a comment -
        Show
        maryannxue Maryann Xue added a comment - Thank you, Julian Hyde , for the review! Fixed in https://git1-us-west.apache.org/repos/asf?p=calcite.git;a=commit;h=2c2b883 .
        Hide
        julianhyde Julian Hyde added a comment -

        No, don't try to fix case-insensitivity. I am working on CALCITE-1549 and it is sure to conflict.

        Show
        julianhyde Julian Hyde added a comment - No, don't try to fix case-insensitivity. I am working on CALCITE-1549 and it is sure to conflict.
        Hide
        maryannxue Maryann Xue added a comment -

        addImplicitFuncNamesToBuilder is called by CalciteSchema.getFunctionNames to get all available function names, and addImplicitFunctionsToBuilder is called by CalciteSchema.getFunctions to get matching functions for a specific function name.

        I see. Then all four functions getImplicitSubSchema, getImplicitTable, getImplicitTableBasedOnNullaryFunction and addImplicitTablesBasedOnNullaryFunctionsToBuilder need to be enhanced for case-insensitive lookup I think. Do you want me to include that fix in the PR?

        Show
        maryannxue Maryann Xue added a comment - addImplicitFuncNamesToBuilder is called by CalciteSchema.getFunctionNames to get all available function names, and addImplicitFunctionsToBuilder is called by CalciteSchema.getFunctions to get matching functions for a specific function name. I see. Then all four functions getImplicitSubSchema , getImplicitTable , getImplicitTableBasedOnNullaryFunction and addImplicitTablesBasedOnNullaryFunctionsToBuilder need to be enhanced for case-insensitive lookup I think. Do you want me to include that fix in the PR?
        Hide
        julianhyde Julian Hyde added a comment -

        The PR looks OK.

        Is addImplicitFuncNamesToBuilder still needed?

        Note that case-insensitive reads from schemas is broken. Your change doesn't make it any better, or any worse. If people want all functions named "foo", case-insensitive, we will just have to scan through all functions in the schema.

        Show
        julianhyde Julian Hyde added a comment - The PR looks OK. Is addImplicitFuncNamesToBuilder still needed? Note that case-insensitive reads from schemas is broken. Your change doesn't make it any better, or any worse. If people want all functions named "foo", case-insensitive, we will just have to scan through all functions in the schema.
        Hide
        maryannxue Maryann Xue added a comment -

        Just made another check-in to that pull request. Jinfeng Ni and Julian Hyde, could you please review it?
        I was wrong about the relationship between caching/no-caching and Schema.getFunctionNames(). Still, SimpleCalciteSchema would be better off avoiding calling methods like Schema.getFunctionNames or Schema.getTableNames when a table, sub-schema, or function name is already specified, because it's simply not necessary and it would more efficient to call just Schema.getFunctions or Schema.getTable with the specified name.
        Again, the original logic in SimpleCalciteSchema.getImplicitTableBasedOnNullaryFunction was incorrect since the parameter "name" was ignored. I found a similar issue with CalciteSchema.addImplicitFunctionsToBuilder. I updated the test case to test both functions and make sure only the functions or views with the matched name are returned.

        Show
        maryannxue Maryann Xue added a comment - Just made another check-in to that pull request. Jinfeng Ni and Julian Hyde , could you please review it? I was wrong about the relationship between caching/no-caching and Schema.getFunctionNames(). Still, SimpleCalciteSchema would be better off avoiding calling methods like Schema.getFunctionNames or Schema.getTableNames when a table, sub-schema, or function name is already specified, because it's simply not necessary and it would more efficient to call just Schema.getFunctions or Schema.getTable with the specified name. Again, the original logic in SimpleCalciteSchema.getImplicitTableBasedOnNullaryFunction was incorrect since the parameter "name" was ignored. I found a similar issue with CalciteSchema.addImplicitFunctionsToBuilder . I updated the test case to test both functions and make sure only the functions or views with the matched name are returned.
        Hide
        maryannxue Maryann Xue added a comment - - edited

        I see. Thanks, Jinfeng Ni! Anyway, if we look at the code I referred, the two things I pointed out are still true:
        1. If you cannot find it in CalciteSchema's function map (i.e., cache), and call SimpleCalciteSchema. getImplicitTableBasedOnNullaryFunction, the "tableName" parameter should be used, otherwise the Schema.getFunctionNames can return whatever functions that can be totally not related to searched function name.
        2. I assume SimpleCalciteSchema. getImplicitTableBasedOnNullaryFunction would have very similar implementation to that of SimpleCalciteSchema.getImplicitTable and SimpleCalciteSchema.getImplicitSubSchema, only that getImplicitTableBasedOnNullaryFunction is to search in functions while the latter two in tables and sub-schemas.

        Show
        maryannxue Maryann Xue added a comment - - edited I see. Thanks, Jinfeng Ni ! Anyway, if we look at the code I referred, the two things I pointed out are still true: 1. If you cannot find it in CalciteSchema 's function map (i.e., cache), and call SimpleCalciteSchema. getImplicitTableBasedOnNullaryFunction , the "tableName" parameter should be used, otherwise the Schema.getFunctionNames can return whatever functions that can be totally not related to searched function name. 2. I assume SimpleCalciteSchema. getImplicitTableBasedOnNullaryFunction would have very similar implementation to that of SimpleCalciteSchema.getImplicitTable and SimpleCalciteSchema.getImplicitSubSchema , only that getImplicitTableBasedOnNullaryFunction is to search in functions while the latter two in tables and sub-schemas.
        Hide
        jni Jinfeng Ni added a comment -

        Maryann Xue, the code in SimpleCalciteSchema.java you referred is actually not new code. It comes from an old version of CalciteSchema.java [1], and I simply moved that code from CalciteSchema to SimpleCalciteSchema.

        If you look at line 408, the tableName is searched first. Only when it does not find anything, it will come down to the code you refereed [2].

        1. https://github.com/apache/calcite/commit/ac8d04ed95589f29571b7de3c220876c3ebc3a00#diff-62a83717e802a726ec2efe30ff7cf4f1L485
        2. https://github.com/apache/calcite/commit/ac8d04ed95589f29571b7de3c220876c3ebc3a00#diff-62a83717e802a726ec2efe30ff7cf4f1R408

        Show
        jni Jinfeng Ni added a comment - Maryann Xue , the code in SimpleCalciteSchema.java you referred is actually not new code. It comes from an old version of CalciteSchema.java [1] , and I simply moved that code from CalciteSchema to SimpleCalciteSchema. If you look at line 408, the tableName is searched first. Only when it does not find anything, it will come down to the code you refereed [2] . 1. https://github.com/apache/calcite/commit/ac8d04ed95589f29571b7de3c220876c3ebc3a00#diff-62a83717e802a726ec2efe30ff7cf4f1L485 2. https://github.com/apache/calcite/commit/ac8d04ed95589f29571b7de3c220876c3ebc3a00#diff-62a83717e802a726ec2efe30ff7cf4f1R408
        Hide
        maryannxue Maryann Xue added a comment -

        The original function implementation was completely wrong since it didn't even take "tableName" into account, which means whichever ViewMacro comes first in the list of getFunctionNames() will be returned regardless of its name. I noticed that Jinfeng Ni added the initial version of this file. Was there some reason or was it just a mistake?
        I think the idea of not relying on Schema.getFunctionNames or Schema.getTableNames is the purpose of having SimpleCalciteSchema. There may or may not be a pre-loaded list of all existing tables or functions or sub-schemas. The javadoc of SimpleCalciteSchema now reads "A concrete implementation of {@link org.apache.calcite.jdbc.CalciteSchema} that maintains minimal state." I think I can add more details there. Besides there might be some other code refinement work left to do.

        Show
        maryannxue Maryann Xue added a comment - The original function implementation was completely wrong since it didn't even take "tableName" into account, which means whichever ViewMacro comes first in the list of getFunctionNames() will be returned regardless of its name. I noticed that Jinfeng Ni added the initial version of this file. Was there some reason or was it just a mistake? I think the idea of not relying on Schema.getFunctionNames or Schema.getTableNames is the purpose of having SimpleCalciteSchema. There may or may not be a pre-loaded list of all existing tables or functions or sub-schemas. The javadoc of SimpleCalciteSchema now reads "A concrete implementation of {@link org.apache.calcite.jdbc.CalciteSchema} that maintains minimal state." I think I can add more details there. Besides there might be some other code refinement work left to do.
        Hide
        julianhyde Julian Hyde added a comment -

        I gather that if Schema.getFunctionNames returns an empty collection then we are in "no-caching mode". Is this behavior documented somewhere? Does this bug only occur in "no-caching mode"?

        Show
        julianhyde Julian Hyde added a comment - I gather that if Schema.getFunctionNames returns an empty collection then we are in "no-caching mode". Is this behavior documented somewhere? Does this bug only occur in "no-caching mode"?
        Hide
        maryannxue Maryann Xue added a comment -

        Julian Hyde, could you please review this fix for me? https://github.com/apache/calcite/pull/350

        Show
        maryannxue Maryann Xue added a comment - Julian Hyde , could you please review this fix for me? https://github.com/apache/calcite/pull/350

          People

          • Assignee:
            maryannxue Maryann Xue
            Reporter:
            maryannxue Maryann Xue
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development