[tarantool-patches] Re: [PATCH] jdbc: fix primary keys meta retrieval
Alexander Turenko
alexander.turenko at tarantool.org
Wed Oct 10 13:19:38 MSK 2018
Hi!
The patch LGTM. I have the last minor concern re exception handling in a
test (see below) and then the patch can be pushed. Please, fix it and
ping me to merge the patch.
WBR, Alexander Turenko.
> > > > 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?
> >
>
> I just did a quick test with live Oracle instance and it returns empty
> resultset for unknown table. So I guess, it is correct.
>
So okay for me. Thanks for the clarification!
> > > + 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"));
> > ```
>
> Yes, it should be SQLException. Probably I was doing some experiments here and then forgot to clean it up.
> Let me know if you want me to correct this with another revision of the patch.
I think it is better to correct this thing. This is the last concern I
have.
More information about the Tarantool-patches
mailing list