[tarantool-patches] Re: [PATCH] jdbc: fix primary keys meta retrieval

[Alexander Turenko]

Hi, Sergei!

The patch is okay for me. I have a couple of minor questions below, can you
please clarify them to me before we'll going to merge this patch?

WBR, Alexander Turenko.

> I have noticed another deviation from specification. The order in which columns are
> returned in the resultset is not correct. The TABLE_CAT and TABLE_SCHEM must go first.
> I have fixed this as well. Plz see the new test 'testGetPrimaryKeysOrderOfColumns'.
 
Good catch, thanks!

> > I think we should check the type to give a proper error message in the
> > case when a user give 'native' tarantool space in parameters.
> > Corresponding test case should be added.
> > 
> > Don't sure how it will be wrapped in Java, I just about the following:
> > 
> > 6th field of _vspace tuple for a 'native' space:
> > 
> > [[0, 'unsigned'], [1, 'unsigned']]
> > 
> > (Don't sure whether it is list or map in the binary protocol; likely
> > map.)
> > 
> > 6th field of _vspace tuple for a sql space:
> > 
> > [{'sort_order': 'asc', 'type': 'scalar', 'field': 0,
> >     'nullable_action': 'abort', 'is_nullable': false}]
> > 
> > What would going on if a user give us 'native' space name?
> 
> I've made some tests. For the 6th field of '_vspace' tuple, I've got the same 'List<Map>'
> in both native and sql space cases, the difference was only in map contents.
> However for the 5th field of '_vindex' tuple, it is 'List<List>' for the native space
> and 'List<Map>' for the sql space.
> 
> So, I added extensive type checking and throw an exception in case unexpected type is encountered.
> See tests 'testGetPrimaryKeyNonSQLSpace' and 'testDatabaseMetaDataGetPrimaryKeysFormatError'.
> 
> Are you certain that we should return the error in case we have been given a native space?
> I went for empty resultset instead. Hope you are OK with that.

A 'native' space is not visible in Tarantool SQL as a table, so we
likely should behave in the similar way as in no such table situation.

Don't found any information whether we should raise an error in case of
non-exists table or return empty result set from the API reference. What
other vendors do?  Postgresql seems ([1]) to return empty result set.

[1]: https://github.com/pgjdbc/pgjdbc/blob/08631ccdabdb8ba6d52f398e2b0b46a9cf0cafbf/pgjdbc/src/main/java/org/postgresql/jdbc/PgDatabaseMetaData.java#L2060

So it is ok if it is really common way to return metainformation (across API
methods as well as across vendors). Are you have any extra info / experience on
that?

> +    private void checkGetPrimaryKeysRow(ResultSet rs, String table, String colName, String pkName, int seq)
> +        throws Exception {

Why `throws Exception`, but not `SQLException`? The following patch
runs smoothly for me:

```
diff --git a/src/test/java/org/tarantool/jdbc/JdbcDatabaseMetaDataIT.java b/src/test/java/org/tarantool/jdbc/JdbcDatabaseMetaDataIT.java
index 78ff89b..8577b23 100644
--- a/src/test/java/org/tarantool/jdbc/JdbcDatabaseMetaDataIT.java
+++ b/src/test/java/org/tarantool/jdbc/JdbcDatabaseMetaDataIT.java
@@ -88,7 +88,7 @@ public class JdbcDatabaseMetaDataIT extends AbstractJdbcIT {
     }
 
     @Test
-    public void testGetPrimaryKeys() throws Exception {
+    public void testGetPrimaryKeys() throws SQLException {
         ResultSet rs = meta.getPrimaryKeys(null, null, "TEST");
 
         assertNotNull(rs);
@@ -102,7 +102,7 @@ public class JdbcDatabaseMetaDataIT extends AbstractJdbcIT {
     }
 
     @Test
-    public void testGetPrimaryKeysCompound() throws Exception {
+    public void testGetPrimaryKeysCompound() throws SQLException {
         ResultSet rs = meta.getPrimaryKeys(null, null, "TEST_COMPOUND");
 
         assertNotNull(rs);
@@ -118,7 +118,7 @@ public class JdbcDatabaseMetaDataIT extends AbstractJdbcIT {
     }
 
     @Test
-    public void testGetPrimaryKeysIgnoresCatalogSchema() throws Exception {
+    public void testGetPrimaryKeysIgnoresCatalogSchema() throws SQLException {
         String[] vals = new String[] {null, "", "IGNORE"};
         for (String cat : vals) {
             for (String schema : vals) {
@@ -168,7 +168,7 @@ public class JdbcDatabaseMetaDataIT extends AbstractJdbcIT {
     }
 
     private void checkGetPrimaryKeysRow(ResultSet rs, String table, String colName, String pkName, int seq)
-        throws Exception {
+        throws SQLException {
         assertNull(rs.getString("TABLE_CAT"));
         assertNull(rs.getString("TABLE_SCHEM"));
         assertEquals(table, rs.getString("TABLE_NAME"));
```