Alexander, please find my answers inline below.
On Tue, Oct 09, 2018 at 06:40:06PM +0300, Alexander Turenko wrote:
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"));
```
