Details

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

      Description

      Both ODBC and JDBC spec mention a CONVERT scalar function, which is the equivalent of the CAST function.

      In order for JDBC and ODBC drivers to expose support for this function, calcite should support parsing it, and converting it into a CAST expression (currently, it doesn't unlike some other scalar functions).

        Activity

        Hide
        julianhyde Julian Hyde added a comment - - edited

        This would be great.

        Note that the SQL standard has a CONVERT function (see CALCITE-111) which has a different purpose (it relates to character sets). Since it is specified without the fn ... syntax, they should be able to coexist.

        Show
        julianhyde Julian Hyde added a comment - - edited This would be great. Note that the SQL standard has a CONVERT function (see CALCITE-111 ) which has a different purpose (it relates to character sets). Since it is specified without the fn ... syntax, they should be able to coexist.
        Hide
        laurentgo Laurent Goujon added a comment -
        Show
        laurentgo Laurent Goujon added a comment - Proposed change: https://github.com/apache/calcite/pull/310
        Hide
        julianhyde Julian Hyde added a comment -

        In Parser.jj, you would reduce redundancy by assigning to a SqlTypeName variable rather than a SqlIdentifier. Make it more compact, like the TimestampInterval function.

        Please add one or two tests to SqlParserTest. (SqlOperatorTest is fine, but it's more of a system test.) Also add one or two negative tests.

        Also please modify REFERENCE.md.

        Show
        julianhyde Julian Hyde added a comment - In Parser.jj, you would reduce redundancy by assigning to a SqlTypeName variable rather than a SqlIdentifier. Make it more compact, like the TimestampInterval function. Please add one or two tests to SqlParserTest. (SqlOperatorTest is fine, but it's more of a system test.) Also add one or two negative tests. Also please modify REFERENCE.md.
        Hide
        laurentgo Laurent Goujon added a comment -

        In Parser.jj, you would reduce redundancy by assigning to a SqlTypeName variable rather than a SqlIdentifier. Make it more compact, like the TimestampInterval function.

        do you mean just returning SqlTypeName for JdbcOdbcType and OdbcIntervalType. It seems there's no function to create a SqlDataTypeSpec out of a typename, nor can you use an interval type with SqlDataTypeSpec, so I still end up having two functions (one for non-interval types, and one for interval types), and I still need to create an instance of SqlDataTypeSpec out of the type name. Did I miss something?

        Please add one or two tests to SqlParserTest. (SqlOperatorTest is fine, but it's more of a system test.) Also add one or two negative tests.

        I'll do.

        Also please modify REFERENCE.md.

        I guess I missed updating the tables for the scalar functions

        Show
        laurentgo Laurent Goujon added a comment - In Parser.jj, you would reduce redundancy by assigning to a SqlTypeName variable rather than a SqlIdentifier. Make it more compact, like the TimestampInterval function. do you mean just returning SqlTypeName for JdbcOdbcType and OdbcIntervalType . It seems there's no function to create a SqlDataTypeSpec out of a typename, nor can you use an interval type with SqlDataTypeSpec , so I still end up having two functions (one for non-interval types, and one for interval types), and I still need to create an instance of SqlDataTypeSpec out of the type name. Did I miss something? Please add one or two tests to SqlParserTest. (SqlOperatorTest is fine, but it's more of a system test.) Also add one or two negative tests. I'll do. Also please modify REFERENCE.md. I guess I missed updating the tables for the scalar functions
        Hide
        julianhyde Julian Hyde added a comment - - edited

        No, JdbcOdbcType and OdbcIntervalType should still return SqlDataTypeSpec. You can just make their internal logic more concise. Note that the pattern 'typeName = new SqlIdentifier(SqlTypeName.xxx.name(), pos);' occurs many times, so refactor it by introducing an internal function that returns a SqlTypeName.

        In OdbcIntervalType, introduce an internal function that returns a TimeUnitRange. It has a start and an end TimeUnit.

        The logic will be the same as what you've written, but fewer lines of code to maintain.

        I guess I missed updating the tables for the scalar functions

        Yes, you missed https://calcite.apache.org/docs/reference.html#system

        Show
        julianhyde Julian Hyde added a comment - - edited No, JdbcOdbcType and OdbcIntervalType should still return SqlDataTypeSpec . You can just make their internal logic more concise. Note that the pattern 'typeName = new SqlIdentifier(SqlTypeName.xxx.name(), pos);' occurs many times, so refactor it by introducing an internal function that returns a SqlTypeName. In OdbcIntervalType, introduce an internal function that returns a TimeUnitRange. It has a start and an end TimeUnit. The logic will be the same as what you've written, but fewer lines of code to maintain. I guess I missed updating the tables for the scalar functions Yes, you missed https://calcite.apache.org/docs/reference.html#system
        Hide
        laurentgo Laurent Goujon added a comment -

        I just updated the pull request to address some of the comments. One question I have is regarding unparsing the query: it currently returns the legacy type identifier instead of the SQL_ ones, and I wonder if I should address this, or if this is okay...

        Show
        laurentgo Laurent Goujon added a comment - I just updated the pull request to address some of the comments. One question I have is regarding unparsing the query: it currently returns the legacy type identifier instead of the SQL_ ones, and I wonder if I should address this, or if this is okay...
        Hide
        julianhyde Julian Hyde added a comment -

        I think it's OK. When we unparse it has to produce a valid query, and a query that has the same effect, but it doesn't have to be same query.

        Show
        julianhyde Julian Hyde added a comment - I think it's OK. When we unparse it has to produce a valid query, and a query that has the same effect, but it doesn't have to be same query.
        Hide
        laurentgo Laurent Goujon added a comment -

        I missed your update, thanks for the clarification though.

        Looking at TimeUnitRange it's an avatica utility class, is it still okay to use in calcite-core?

        Show
        laurentgo Laurent Goujon added a comment - I missed your update, thanks for the clarification though. Looking at TimeUnitRange it's an avatica utility class, is it still okay to use in calcite-core?
        Hide
        laurentgo Laurent Goujon added a comment -

        sounds good. I still need to add the regular interval types to the parser I guess (or the query won't be valid).

        Show
        laurentgo Laurent Goujon added a comment - sounds good. I still need to add the regular interval types to the parser I guess (or the query won't be valid).
        Hide
        laurentgo Laurent Goujon added a comment -

        I pushed a new update to my branch

        Show
        laurentgo Laurent Goujon added a comment - I pushed a new update to my branch
        Hide
        julianhyde Julian Hyde added a comment -

        This isn't ready, and I think you know that. I tried to add some interval cases to SqlParserTest, and they broke SqlUnParserTest because you haven't implemented unparse on intervals. Can you fix, please? Do it as a separate commit (not a squash) so I can merge in changes I've made.

        Show
        julianhyde Julian Hyde added a comment - This isn't ready, and I think you know that. I tried to add some interval cases to SqlParserTest, and they broke SqlUnParserTest because you haven't implemented unparse on intervals. Can you fix, please? Do it as a separate commit (not a squash) so I can merge in changes I've made.
        Hide
        julianhyde Julian Hyde added a comment -

        Also, it will parse 'DAY TO HOUR' but not 'DAY TO MINUTE' because you have the '|' in the wrong place. I'm fixing that.

        Show
        julianhyde Julian Hyde added a comment - Also, it will parse 'DAY TO HOUR' but not 'DAY TO MINUTE' because you have the '|' in the wrong place. I'm fixing that.
        Hide
        laurentgo Laurent Goujon added a comment -

        will do, I should have mentionned it was still a bit WIP, and that I was looking more for comments regarding Parser.jj changes following your previous comments.

        Show
        laurentgo Laurent Goujon added a comment - will do, I should have mentionned it was still a bit WIP, and that I was looking more for comments regarding Parser.jj changes following your previous comments.
        Hide
        julianhyde Julian Hyde added a comment -

        Yes, you must do that. Here I am, fixing your code. I have better things to do.

        Show
        julianhyde Julian Hyde added a comment - Yes, you must do that. Here I am, fixing your code. I have better things to do.
        Hide
        laurentgo Laurent Goujon added a comment -

        sorry for that, it was not my intent. I know everybody's time is precious, and I understand that you might habe better things to do, so just feel free to to ignore my patch, or just tell it's not working...

        I also noticed that you don't comment on github so I wonder if I'm using the right workflow to contribute to Calcite.

        Show
        laurentgo Laurent Goujon added a comment - sorry for that, it was not my intent. I know everybody's time is precious, and I understand that you might habe better things to do, so just feel free to to ignore my patch, or just tell it's not working... I also noticed that you don't comment on github so I wonder if I'm using the right workflow to contribute to Calcite.
        Hide
        julianhyde Julian Hyde added a comment -

        I've pushed my changes to https://github.com/julianhyde/calcite/tree/1444-jdbc-convert. Can you build on those and make sure that SqlUnParserTest passes?

        Show
        julianhyde Julian Hyde added a comment - I've pushed my changes to https://github.com/julianhyde/calcite/tree/1444-jdbc-convert . Can you build on those and make sure that SqlUnParserTest passes?
        Hide
        julianhyde Julian Hyde added a comment -

        I don't comment on github very often. Github reviews tend to get easily lost (e.g. when the contributor rebases) so if my comments are substantive I'd prefer to have them in the permanent record in JIRA.

        Also, I tend to review by downloading the code, reading it in an IDE, and running tests. So it's not particularly convenient to go back to the github web UI to make comments.

        Show
        julianhyde Julian Hyde added a comment - I don't comment on github very often. Github reviews tend to get easily lost (e.g. when the contributor rebases) so if my comments are substantive I'd prefer to have them in the permanent record in JIRA. Also, I tend to review by downloading the code, reading it in an IDE, and running tests. So it's not particularly convenient to go back to the github web UI to make comments.
        Hide
        laurentgo Laurent Goujon added a comment -

        I updated the pull request with your commit and another commit to fix the unparse issue.

        When looking at the issue, it was not clear on how to fix the unparse output for interval types: SqlJdbcFunctionCall unparse is the same for all functions. Creating a subclass would have worked but I would have to change the jdbc parser function to not use the main class for CONVERT (with some extra logic duplication). I also thought of adding an unparse method to MakeCall which would be called by SqlJdbcFunctionCall#unparse, but some of the SqlJdbcFunctionCall instances created by calcite don't have a MakeCall instance.
        Instead (inspired by your refactoring of the parser in your patch), I decided to create a new enum/symbol SqlJdbcDataTypeName to list the possible types supported by CONVERT, and with an helper method to convert it into the proper SQL data type when converting the call to a CAST operator. The parser has been simplified a bit (because we are mapping the token to a SqlLiteral), and the lookup entry for CONVERT was changed to add the conversion. Side consequence: I can remove the legacy (and non-supported) interval types I added earlier.

        Let me know if this approach sounds reasonable.

        Show
        laurentgo Laurent Goujon added a comment - I updated the pull request with your commit and another commit to fix the unparse issue. When looking at the issue, it was not clear on how to fix the unparse output for interval types: SqlJdbcFunctionCall unparse is the same for all functions. Creating a subclass would have worked but I would have to change the jdbc parser function to not use the main class for CONVERT (with some extra logic duplication). I also thought of adding an unparse method to MakeCall which would be called by SqlJdbcFunctionCall#unparse, but some of the SqlJdbcFunctionCall instances created by calcite don't have a MakeCall instance. Instead (inspired by your refactoring of the parser in your patch), I decided to create a new enum/symbol SqlJdbcDataTypeName to list the possible types supported by CONVERT, and with an helper method to convert it into the proper SQL data type when converting the call to a CAST operator. The parser has been simplified a bit (because we are mapping the token to a SqlLiteral), and the lookup entry for CONVERT was changed to add the conversion. Side consequence: I can remove the legacy (and non-supported) interval types I added earlier. Let me know if this approach sounds reasonable.
        Hide
        julianhyde Julian Hyde added a comment -

        SqlJdbcDataTypeName is a good idea. The only change I'd make is to replace the dataType field with a SqlNode createDataType(SqlParserPos pos)) method that creates a new SqlDataTypeSpec each time (I worry about big/mutable state).

        Show
        julianhyde Julian Hyde added a comment - SqlJdbcDataTypeName is a good idea. The only change I'd make is to replace the dataType field with a SqlNode createDataType(SqlParserPos pos)) method that creates a new SqlDataTypeSpec each time (I worry about big/mutable state).
        Hide
        laurentgo Laurent Goujon added a comment -

        Easy to do. Just to avoid adding a createDataType method implementation for each enum value (depending if interval type or not), I'm keeping the dataType field private, and cloning it in SqlNode createDataType(SqlParserPos).

        The commit has been updated accordingly.

        Show
        laurentgo Laurent Goujon added a comment - Easy to do. Just to avoid adding a createDataType method implementation for each enum value (depending if interval type or not), I'm keeping the dataType field private, and cloning it in SqlNode createDataType(SqlParserPos) . The commit has been updated accordingly.
        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/9266d8ee . Thanks for the PR, Laurent Goujon !
        Hide
        julianhyde Julian Hyde added a comment -

        Resolved in release 1.11.0 (2017-01-11).

        Show
        julianhyde Julian Hyde added a comment - Resolved in release 1.11.0 (2017-01-11).

          People

          • Assignee:
            julianhyde Julian Hyde
            Reporter:
            laurentgo Laurent Goujon
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development