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

Create handlers for JDBC dialect-specific generated SQL

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.14.0
    • Component/s: jdbc-adapter
    • Labels:
      None

      Description

      Currently the only way to generate different sql for dialects is to switch on the dialect in the unparse method. This is used quite heavily in e.g. SqlFloorFunction, but there are also switches in:

      • SUBSTRING()
      • SqlDateLiteral quoting
      • SqlTimestampLiteral quoting
      • Dialects using different interval literals (e.g. Hsqldb uses YYYY & MM rather than YEAR & MONTH)
      • limit/offset construction
      • mysql isnull function
      • type differences

      It would be great to have dialect specific handlers to deal with these, making testing & addition of new handlers (new dialects, or new overrides for a given function) much easier in the future.

      One suggested path to approach this: https://issues.apache.org/jira/browse/CALCITE-1798?focusedCommentId=16031609&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16031609

      There is a comment in SqlImplementor that reads "MySQL doesn't have a VARCHAR type, only CHAR.". Not sure if this was for a very old version of mysql, but it's certainly not true anymore.

        Issue Links

          Activity

          Hide
          michaelmior Michael Mior added a comment -

          Resolved in release 1.14.0 (2017-10-01)

          Show
          michaelmior Michael Mior added a comment - Resolved in release 1.14.0 (2017-10-01)
          Hide
          julianhyde Julian Hyde added a comment -
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/bdaa33f9 . Thanks for the PR, Chris Baynes !
          Hide
          chris-baynes Chris Baynes added a comment -

          I've had a look through and those changes make sense to me. I like the move of the unparseCall directly onto the SqlDialect.
          Thanks for the review!

          Show
          chris-baynes Chris Baynes added a comment - I've had a look through and those changes make sense to me. I like the move of the unparseCall directly onto the SqlDialect. Thanks for the review!
          Hide
          julianhyde Julian Hyde added a comment -

          Regarding handlers. I think they would be loaded from the class-path but we probably wouldn't let people add them explicitly. Anyway, I think we should defer for now. The API won't change significantly if and when we make them pluggable.

          I've just reviewed your code. I took the liberty of moving some things around. I think it improves the structure. Hope you don't mind.

          • I moved ORACLE_SUBSTR to OracleSqlOperatorTable. So we could potentially use it for parsing Calcite SQL (if people have Oracle mode enabled) in addition to generating SQL.
          • I made SqlCall call SqlDialect.unparseCall rather than DialectUnparser.unparseCall. I think SqlDialect should be the facade that all this stuff goes through.
          • I moved the unparser from DatabaseProduct to SqlDialect, and made it a private field, but nevertheless a parameter to the SqlDialect constructor. Thus someone could create a dialect that, say, deals with Oracle but has a different unparser.
          • I renamed the "unparse" package to "dialect", "DialectUnparser" to Handler, "DialectHsqldb" to "HsqldbHandler", etc.
          • In unparseFloorWithUnit, I converted an argument from Integer to int

          I've pushed my changes to https://github.com/julianhyde/calcite/1841-unparse. Please review then I'll commit.

          Show
          julianhyde Julian Hyde added a comment - Regarding handlers. I think they would be loaded from the class-path but we probably wouldn't let people add them explicitly. Anyway, I think we should defer for now. The API won't change significantly if and when we make them pluggable. I've just reviewed your code. I took the liberty of moving some things around. I think it improves the structure. Hope you don't mind. I moved ORACLE_SUBSTR to OracleSqlOperatorTable. So we could potentially use it for parsing Calcite SQL (if people have Oracle mode enabled) in addition to generating SQL. I made SqlCall call SqlDialect.unparseCall rather than DialectUnparser.unparseCall. I think SqlDialect should be the facade that all this stuff goes through. I moved the unparser from DatabaseProduct to SqlDialect, and made it a private field, but nevertheless a parameter to the SqlDialect constructor. Thus someone could create a dialect that, say, deals with Oracle but has a different unparser. I renamed the "unparse" package to "dialect", "DialectUnparser" to Handler, "DialectHsqldb" to "HsqldbHandler", etc. In unparseFloorWithUnit, I converted an argument from Integer to int I've pushed my changes to https://github.com/julianhyde/calcite/1841-unparse . Please review then I'll commit.
          Hide
          chris-baynes Chris Baynes added a comment -

          Ok, so I've moved things into an unparse subpackage and created singleton instances of each of the handlers.

          I'm not sure about changing the hard-coded handlers to database products. Would you envision being able to dynamically add a handler to a dialect, or to be able to dynamically use a handler for a specific query use case? Either way I'm not really sure where that would hook in, perhaps a SqlCall could have a handler set on it?

          Show
          chris-baynes Chris Baynes added a comment - Ok, so I've moved things into an unparse subpackage and created singleton instances of each of the handlers. I'm not sure about changing the hard-coded handlers to database products. Would you envision being able to dynamically add a handler to a dialect, or to be able to dynamically use a handler for a specific query use case? Either way I'm not really sure where that would hook in, perhaps a SqlCall could have a handler set on it?
          Hide
          julianhyde Julian Hyde added a comment -

          I did a quick review and the overall structure looks good.

          Do you think we should move the dialect handlers into a new package? org.apache.calcite.sql is rather full. Maybe org.apache.calcite.sql.dialect or org.apache.calcite.sql.unparse.

          It's worth stating in the interface that handlers are stateless, therefore immutable. We should use a singleton instance of the default handler.

          Currently, because of how they are created, handlers must correspond to a database product, and to add a handler, you have to change Calcite code (i.e. DatabaseProduct). Is that too restrictive?

          Show
          julianhyde Julian Hyde added a comment - I did a quick review and the overall structure looks good. Do you think we should move the dialect handlers into a new package? org.apache.calcite.sql is rather full. Maybe org.apache.calcite.sql.dialect or org.apache.calcite.sql.unparse. It's worth stating in the interface that handlers are stateless, therefore immutable. We should use a singleton instance of the default handler. Currently, because of how they are created, handlers must correspond to a database product, and to add a handler, you have to change Calcite code (i.e. DatabaseProduct). Is that too restrictive?
          Hide
          chris-baynes Chris Baynes added a comment -

          I've created a PR for this: https://github.com/apache/calcite/pull/501

          During the refactoring I also noticed this issue: https://issues.apache.org/jira/browse/CALCITE-1895

          Show
          chris-baynes Chris Baynes added a comment - I've created a PR for this: https://github.com/apache/calcite/pull/501 During the refactoring I also noticed this issue: https://issues.apache.org/jira/browse/CALCITE-1895
          Hide
          julianhyde Julian Hyde added a comment -

          I don't much care how the code is structured as long as there are thorough tests. If there are tests we can safely re-structure the code later.

          We all agree that we should try to organize code in a dialect, rather than scattering it around sub-classes of SqlOperator.

          Is there a case for an OracleSqlFloorFunction? I don't know; it might save a few lines or code, or it might add a few. We need to dispatch on both dialect and operator, and most direct way to do this might be to have a SqlDialect.unparseCall(SqlOperator op) method that in most cases contains a switch (op.getKind()).

          Show
          julianhyde Julian Hyde added a comment - I don't much care how the code is structured as long as there are thorough tests. If there are tests we can safely re-structure the code later. We all agree that we should try to organize code in a dialect, rather than scattering it around sub-classes of SqlOperator . Is there a case for an OracleSqlFloorFunction ? I don't know; it might save a few lines or code, or it might add a few. We need to dispatch on both dialect and operator, and most direct way to do this might be to have a SqlDialect.unparseCall(SqlOperator op) method that in most cases contains a switch (op.getKind()) .
          Hide
          chris-baynes Chris Baynes added a comment -

          I completely agree that the dialect specific translations should be together in one place. But the transformation can even change the function name (floor -> date_trunc). If that's done before unparse it means the SqlFloorFunction unparse could no longer be used, and we'd have to construct some generic SqlCall in the transformation to bypass SqlFloorFunction.

          That's why I thought it would be nicer to have something like an "OracleSqlFloorFunction extends SqlFloorFunction". The dialect version specifics could also go in that class. Also perhaps the dialect specific function classes could go into their own package.
          Though I don't know how to plug those things together. Maybe that's too tightly coupling things.

          Show
          chris-baynes Chris Baynes added a comment - I completely agree that the dialect specific translations should be together in one place. But the transformation can even change the function name (floor -> date_trunc). If that's done before unparse it means the SqlFloorFunction unparse could no longer be used, and we'd have to construct some generic SqlCall in the transformation to bypass SqlFloorFunction. That's why I thought it would be nicer to have something like an "OracleSqlFloorFunction extends SqlFloorFunction". The dialect version specifics could also go in that class. Also perhaps the dialect specific function classes could go into their own package. Though I don't know how to plug those things together. Maybe that's too tightly coupling things.
          Hide
          jbalint@gmail.com Jess Balint added a comment -

          I would much prefer to have all DB-specific translations done in one place specific to that db, not specific to the SQL function call. It's not the responsibility of the SqlCall to know how to "unparse" itself for each DB dialect.

          We could add a subclass of SqlDialect for each DB (and some specific versions, e.g. Oracle11, Oracle12, one may subclass the other). The dialect would have a visitor that would transform the sql calls. This would have to be called before unparse.

          Show
          jbalint@gmail.com Jess Balint added a comment - I would much prefer to have all DB-specific translations done in one place specific to that db, not specific to the SQL function call. It's not the responsibility of the SqlCall to know how to "unparse" itself for each DB dialect. We could add a subclass of SqlDialect for each DB (and some specific versions, e.g. Oracle11, Oracle12, one may subclass the other). The dialect would have a visitor that would transform the sql calls. This would have to be called before unparse.
          Hide
          chris-baynes Chris Baynes added a comment -

          Why not do this before unparsing by transforming the SqlNode tree?

          Since most of the dialect specific changes are related to the name of the function or things like the order of parameters, I think it would be nice to have handlers like OracleSqlFloorFunction & MysqlSqlFloorFunction. The easiest way to hook those up afaik is for them to implement the unparse method.
          Once inside the unparse method the SqlCall can of course be transformed, but doing this outside of dialect specific code gets ugly since you have to use specific strings which are of course not defined anywhere in calcite as enums or in operator tables ("DATE_TRUNC", "YYYY", "MM", etc).

          What implementation did you have in mind?

          Show
          chris-baynes Chris Baynes added a comment - Why not do this before unparsing by transforming the SqlNode tree? Since most of the dialect specific changes are related to the name of the function or things like the order of parameters, I think it would be nice to have handlers like OracleSqlFloorFunction & MysqlSqlFloorFunction. The easiest way to hook those up afaik is for them to implement the unparse method. Once inside the unparse method the SqlCall can of course be transformed, but doing this outside of dialect specific code gets ugly since you have to use specific strings which are of course not defined anywhere in calcite as enums or in operator tables ("DATE_TRUNC", "YYYY", "MM", etc). What implementation did you have in mind?
          Hide
          jbalint@gmail.com Jess Balint added a comment -

          Why not do this before unparsing by transforming the SqlNode tree?

          Show
          jbalint@gmail.com Jess Balint added a comment - Why not do this before unparsing by transforming the SqlNode tree?

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              chris-baynes Chris Baynes
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development