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.

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.

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?

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.

Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/84b49a9c. Thanks for the PR, Chris Baynes!

Resolved in release 1.14.0 (2017-10-01)