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

Validator in Frameworks should expand identifiers

    Details

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

      Description

      Calcite supports two ways to build query plan for a SQL statement. Either submit the query through JDBC, which will use CalcitePrepareImpl class, or build a customized planner by leveraging the Frameworks/PlannerImpl class. However, the SqlValidator created in Frameworks/PlannerImpl uses a different setting in terms of identifier expanding, than the setting in the SqlValidator in CalcitePrepareImpl. This implies that one same query goes through those two SqlValidators would have different validated forms.

      SqlValidator.expandIdentifiers in by default is set to false. But CalcitePrepareImpl.prepare() will reset it to true at CalcitePrepareImpl:494:

            final SqlValidator validator =
                new CalciteSqlValidator(opTab, catalogReader, typeFactory);
            validator.setIdentifierExpansion(true);
      

      In contrast, the SqlValidator created in Frameworks/PlannerImpl will still use the default "false" setting.

      Further, Frameworks does not expose the instance of SqlValidator, so user does not have a way to change the setting for the SqlValidator created in Frameworks/PlannerImpl.

      To make behaviors consistent, I would like to propose that we make Framework's SqlValidator uses the same setting as the one in CalcitePrepareImpl.

        Activity

        Hide
        julianhyde Julian Hyde added a comment -

        +1 to make Frameworks validator consistent.

        Show
        julianhyde Julian Hyde added a comment - +1 to make Frameworks validator consistent.
        Hide
        jni Jinfeng Ni added a comment -

        Making Frameworks validator consistent will help solve a problem Drill is facing (The problem will not happen if the query uses CalcitePrepareImpl).

        On the other hand, I'm wondering if SqlValidator by default should expand identifier, in stead of the opposite. At least one place in Calcite code assumes to expand identifier,

        SqlFunction:290 has the following code:

              // REVIEW jvs 25-Mar-2005:  This is, in a sense, expanding
              // identifiers, but we ignore shouldExpandIdentifiers()
              // because otherwise later validation code will
              // choke on the unresolved function.
              ((SqlBasicCall) call).setOperator(function);
        

        That seems to always expand identifier. So, if SqlValidatorImpl set the flag to false, then some validation code might run into problem, especially when there is UDF with case insensitivity matching for function name resolution.

        Show
        jni Jinfeng Ni added a comment - Making Frameworks validator consistent will help solve a problem Drill is facing (The problem will not happen if the query uses CalcitePrepareImpl). On the other hand, I'm wondering if SqlValidator by default should expand identifier, in stead of the opposite. At least one place in Calcite code assumes to expand identifier, SqlFunction:290 has the following code: // REVIEW jvs 25-Mar-2005: This is, in a sense, expanding // identifiers, but we ignore shouldExpandIdentifiers() // because otherwise later validation code will // choke on the unresolved function. ((SqlBasicCall) call).setOperator(function); That seems to always expand identifier. So, if SqlValidatorImpl set the flag to false, then some validation code might run into problem, especially when there is UDF with case insensitivity matching for function name resolution.
        Show
        julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/f9db1ee9 .
        Hide
        julianhyde Julian Hyde added a comment -

        Closing now that 1.1.0-incubating has been released.

        Show
        julianhyde Julian Hyde added a comment - Closing now that 1.1.0-incubating has been released.

          People

          • Assignee:
            julianhyde Julian Hyde
            Reporter:
            jni Jinfeng Ni
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development