Details

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

      Description

      The format mssql uses looks like:
      SUBSTRING(input, start, length)

      but the default is currently to unparse to SUBSTRING(input FROM start FOR length)

        Issue Links

          Activity

          Hide
          chris-baynes Chris Baynes added a comment -

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

          I added a notOk method to the RelToSqlConverterTest, as I wanted to be able to write negative tests.
          In this case Mssql only allows the 3 argument substring, so if the query uses the 2 argument version I want to check the dialect handler will throw an exception.

          Show
          chris-baynes Chris Baynes added a comment - I've created a PR for this: https://github.com/apache/calcite/pull/504 I added a notOk method to the RelToSqlConverterTest, as I wanted to be able to write negative tests. In this case Mssql only allows the 3 argument substring, so if the query uses the 2 argument version I want to check the dialect handler will throw an exception.
          Hide
          julianhyde Julian Hyde added a comment -

          A couple of comments:

          • Can you rename notOk to throws_ (just for consistency with methods elsewhere)
          • I think you should throw IllegalArgumentException rather than AssertionError. AssertionError is (in my opinion) only for internal errors, therefore it is impossible for user code to cause it, and it should only occur in tests for private APIs.
          • Would it be straightforward to add a test that executes a query with substring on MSSQL? It's OK if not. But we're only testing SQL generation at present, and we'd get better coverage if we actually executed the SQL.
          Show
          julianhyde Julian Hyde added a comment - A couple of comments: Can you rename notOk to throws_ (just for consistency with methods elsewhere) I think you should throw IllegalArgumentException rather than AssertionError . AssertionError is (in my opinion) only for internal errors, therefore it is impossible for user code to cause it, and it should only occur in tests for private APIs. Would it be straightforward to add a test that executes a query with substring on MSSQL? It's OK if not. But we're only testing SQL generation at present, and we'd get better coverage if we actually executed the SQL.
          Hide
          chris-baynes Chris Baynes added a comment -

          I've updated the PR with the throws_ and IllegalArgumentException as you suggested (I never liked the idea of catching the AssertionError but didn't see a more appropriate exception at the time).

          As for the test I don't see support for mssql in the calcite-test-dataset, though I can run an instance on aws to try that locally.
          If I write that test in JdbcAdapterTest will it be run against Mssql on travis-ci? Is there a config of jdbc dbs that are tested against?

          Show
          chris-baynes Chris Baynes added a comment - I've updated the PR with the throws_ and IllegalArgumentException as you suggested (I never liked the idea of catching the AssertionError but didn't see a more appropriate exception at the time). As for the test I don't see support for mssql in the calcite-test-dataset, though I can run an instance on aws to try that locally. If I write that test in JdbcAdapterTest will it be run against Mssql on travis-ci? Is there a config of jdbc dbs that are tested against?
          Hide
          julianhyde Julian Hyde added a comment -

          It's tricky to add a new database to CI. But that shouldn't be the goal. I think your goal should be to run

          mvn -Dcalcite.test.db=mssql test

          from the command line. To do this, add a new value MSSQL to enum DatabaseInstance in CalciteAssert. Then populate your database manually. It's straightforward to populate the SCOTT schema (EMP, DEPT etc.)

          Maybe eventually we can get that database into Vagrant or Docker.

          I'm reviewing and testing your PR now.

          Show
          julianhyde Julian Hyde added a comment - It's tricky to add a new database to CI. But that shouldn't be the goal. I think your goal should be to run mvn -Dcalcite.test.db=mssql test from the command line. To do this, add a new value MSSQL to enum DatabaseInstance in CalciteAssert. Then populate your database manually. It's straightforward to populate the SCOTT schema (EMP, DEPT etc.) Maybe eventually we can get that database into Vagrant or Docker. I'm reviewing and testing your PR now.
          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/84b49a9c . Thanks for the PR, Chris Baynes !
          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)

            People

            • Assignee:
              chris-baynes Chris Baynes
              Reporter:
              chris-baynes Chris Baynes
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development