Allow query syntax customization in TCK TestKit

[elefeint]

The TestKit relies on a few common queries, such as INSERT INTO test VALUES(200) to exercise functionality.

These queries could be extracted into templated/default methods, so that drivers implementing the TestKit could override just the querie , avoiding having to duplicate whole test cases that use these queries.

For context, Cloud Spanner requires that columns be enumerated, so we have to use INSERT INTO test (value) VALUES(200) instead of INSERT INTO test VALUES(200).

Not urgent; you can ignore this until after the 0.8 release. I'd just like to clean up our TestKit implementation at some point.

If you think this is a good idea, I can send a PR.

[mp911de]

We could extend the TestKit with a statement factory or the like instead of adding default methods for each statement to TestKit to keep the TestKit focused on behavior and not so much on the statements. Related is also a configuration about store capabilities as there are databases that might not support features such as BLOB/CLOB.

We've seen already folks asking for R2DBC store support that don't use SQL but rather a different query language. To verify their functionality, we would be required to abstract the statements anyway, so that sounds a decent proposal.

[meltsufin]

+1 Since the query syntax is not part of the spec, it should be customizable for the purposes of the TestKit. Otherwise, we're forced to copy the tests and modify just the SQL line, and then worry about the test method body getting out of date with the upstream.

[mp911de]

@elefeint I submitted a pull request #180 to introduce customization hooks for SQL statements in the TCK. Let me know whether the changed TCK works for you or whether you require additional changes.

[elefeint]

This helps a lot, thank you! You can see the simplification to the Cloud Spanner TCK tests in GoogleCloudPlatform/cloud-spanner-r2dbc#270.

We have to keep a few test overrides for two different reasons:

1. Spanner does not have an Integer-sized types, so the 100/200 values come back as Long. This is very Spanner-specific, so while I'd love a similar override for .expectNext(expandValue(VALUE_100)) instead of .expectNext(200L), it's probably not merited in the SPI TCK.

2. Spanner column names are case-sensitive, which makes duplicateColumnNames() and columnMetadata() fail. It may make sense to separate TCK case insensitivity tests from duplicate names/metadata tests.

[mp911de]

Thanks for having a look. I'm going to prepare the merge.

I'm not sure about 2., whether we're on the same side. The verification about duplicate column names should verify that the resulting collection of column names Collection<String> columnNames = rowMetadata.getColumnNames() is case-insensitive (although the database itself may be case-sensitive, but that's a different topic).

[elefeint]

Hmm. You think that Spanner driver should honor R2DBC contract (case insensitive retrieval by columns, first column definition wins in case Column and CoLuMn are encountered) despite underlying database having case sensitive columns? We'll have to discuss it for the new version.
My first reaction was that this will surprise the user, but then the choice is between surprising a user familiar with Cloud Spanner or surprising a user familiar with R2DBC spec. In either case, having two columns that differ only in case would be such terrible query/table design that in practical terms it'd be fine either way.

[mp911de]

Yes. JDBC's ResultSet.get…(String) is case-insensitive, too and we stick to the same convention. Another thing to consider is that if a result contains the same column (label) name twice, the first one is used when calling Row.get(String). The metadata helps to resolve ambiguity because one can inspect columns and call Row.get(int).